Skip to content
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: consider contract expiry before acting #1548

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Mar 24, 2022

Complementary to #1541, this includes client-side checks.

In particular, (*trackedTrade).counterPartyConfirms checks if the contract is expired and returns an expired bool. The callers (isSwappable and isRedeemable) consider the expired status in the same way they already consider spent status -- self-revoke and return false to the spendable/redeemable check. Specifically, this happens only in the order.TakerSwapCast && order.Maker condition of isRedeemable, and in the order.MakerSwapCast && order.Taker condition of isSwappable.

Note that in the first scenario above of the maker deciding if the taker's contract is redeemable, it is quite important not to redeem an expired contract. This is true for either actor since the contract could also be refunded at any time, but it's absolutely critical for maker since they are revealing the secret key to the taker. That must not happen if the taker's contract is expired.

Another check added is for taker at MakerSwapCast when checking swappability, where it now also ensures that the taker's own contract that it would publish would not have a lock time that is already in the past.

To ease readability, some order status if statements are converted to switch statements. Also, an initial order status filter is added to isSwappable and isRedeemable for efficiency.

Finally, an minor bug with wg.Done() being called not at the end of a defer stack is fixed.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some intermittent failures in core harness tests, specifically TestNoMakerRedeem, but I'm not convinced it has anything to do with this PR.

client/core/trade.go Outdated Show resolved Hide resolved
@chappjc
Copy link
Member Author

chappjc commented Apr 6, 2022

Holding off merging this until #1541 is in since testing that without this client change is potentially more informative. On the other hand, stacking that PR on top of this one is potentially helpful as well.

@chappjc chappjc force-pushed the client-check-expiry branch from cec3699 to c758a54 Compare April 6, 2022 20:44
@chappjc chappjc merged commit f39971b into decred:master Apr 8, 2022
@chappjc chappjc deleted the client-check-expiry branch April 8, 2022 03:31
@chappjc chappjc added this to the 0.4.3 milestone Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants