-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client/core: ensure redeem is accepted before confirm/complete match #2405
Conversation
b63a264
to
b98a2ef
Compare
c170795
to
029b257
Compare
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.
LGTM. shouldConfirmRedemption
could return false if match.redemptionConfs > match.redemptionConfsReq
, but nbd.
Will you walk me through this? I might be missing a detail. Summary of the changes in current PR:
|
Yes. I guess it's better this way though.. the UI will be kept up to date. |
I'm not sure but I think inside of Also, I guess setting |
The idea was to just keep updating the confirmation count, but you and @martonp both found this notable, so will change it. I had removed a short-circuit condition from a previous iteration: https://github.com/decred/dcrdex/compare/d0530fddea72201fe2cbe1ed01155823b0de356d..c17079563ceeaf401bd6c89399dfe88cff402d83
I was trying to skip having to wait for another tick, but I think it'll do it in the same tick anyway. Updating. EDIT: Nope, won't be the same tick because the meat of sendRedeemAsync is async in a goroutine. Oh well, still changing it so it notifies. |
029b257
to
a21fa90
Compare
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.
Thanks for answering our nits.
Not nits. I repeatedly missed some important details on this one. Thanks for reviewing. |
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.
Just a rebase needed.
a21fa90
to
2d178ce
Compare
In certain cases, the redeem confirmation status was skipping the retries of the
'redeem'
request. This was happening becauseconfirmRedemption
would advance the status to confirmed regardless of whether there was a storedRedeemSig
, which indicates that the redeem request was accepted by the server.This fixes the issue by
adding theonly advancing toRedeemSig
check toshouldConfirmRedemption
, which allowsresendPendingRequests
when they are needed.MatchConfirmed
if both (a) match is confirmed, and (b) server has received and ack'd ourredeem
request, which is necessary to complete our role in the swap dance.