This repository has been archived by the owner on Aug 2, 2021. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
swap, swap/chain, contracts/swap: add transaction queue #2124
base: master
Are you sure you want to change the base?
swap, swap/chain, contracts/swap: add transaction queue #2124
Changes from 8 commits
422ae58
65c5899
145097c
f6cdcab
97ce00b
eea13b6
8a3d48c
47ee895
7486071
255927d
8a9ea5e
1a59564
c872d34
a67d6ea
2deea57
67fd670
f936138
d9e54ba
40327f9
be6d28c
89ceeb5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments seem to contradict each other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't. The CashoutProcessor calls this when a CashoutRequest was successfully executed (during the
NotifyReceipt
handler). If an error is returned it will be called again, because theNotifyReceipt
will run again at some point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be a very vital feature of the queue and deserves (in my opinion) a somewhat more explicit comment here.
I was following the code to actually verify that the txqueue keeps calling
HandleCashoutResult
ifHandleCashoutResult
returns an error, but could not find this logic:processQueue
callsprocessRequest
(directly or viaprocessActveRequest
). (txqueue.go:634
)pending
,processRequest
callstxq.waitForActiveTransaction
(txqueue.go:477
)waitForActiveTransaction
blocks until the tx is mined (or txqueue/tx context times out). If tx is mined, it callstxq.finalizeRequestConfirmed
(txqueue.go:610
)finalizeRequestConfirmed
callstxq.notify
, who on it's turn returns thetrigger
function. Thetrigger
function is returned bynotify
, allowing the caller ofnotify
to tell the queue that an item is waiting.finalizeRequestConfirmed
calles thetrigger
function, returned bytxq.notify
And now I lost the track... Can you help me by explaining how the call proceeds from here? How and where is this trigger signal received and how/where can I see that indeed,
HandleCashoutResult
is retried if it failed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All notification queues are processed in their respective go routine started by the
SetHandlers
function. If a handler is registered the trigger will wake thekey, err := notifyQueue.Next(txq.ctx, &item, &txq.lock)
call on line 266.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are only setting the handler for the
NotifyReceipt
. Can you explain to me why you decided to define all other handlers (NotifyPending
,NotifyCancelled
,NotifyStatusUnkown
)?Again: consider using all handlers which you define or else add them in later PRs (I don't think it is good style to define code-components and then don't use them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are all used in tests at least. Technically all places which schedule transactions also should monitor at least
NotifyCancelled
andNotifyStatusUnkown
. Those handlers will be added soon, once we have decided what we actually want to do in those scenarios (e.g. for cashing we might want to retry onCancelled
and log onStatusUnknown
). Anyway I decided to leave this out to keep the PR smaller.Anyway those handlers are important already as otherwise we cannot properly test the error conditions during the txqueue tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Maybe, for now, we can already register the handlers for now and just write a log output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done