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.

Sign up for GitHub

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

Jump to bottom

core: match status resolution #704

Merged
merged 1 commit into from
Oct 2, 2020
Merged

core: match status resolution #704

merged 1 commit into from
Oct 2, 2020

Conversation

buck54321
Copy link
Member

@buck54321 buck54321 commented Sep 24, 2020 •
edited
Loading

Implement match status resolution.

  1. Match status resolution is run on startup and reconnects for any status mismatches observed when comparing the connect result with our local state.
  2. Client will now begin negotiation based on the extra matches in the connect response.
  3. Differentiate between server-initiated revocations and client-initiated revocations. Required a database upgrade.

@chappjc chappjc mentioned this pull request Sep 25, 2020
Closed
@chappjc
Copy link
Member

chappjc commented Sep 25, 2020 •
edited
Loading

With your next commit, please rebase away c701f0e since that's in status-integ now.

@chappjc chappjc linked an issue Sep 28, 2020 that may be closed by this pull request
use match_status route #643
Closed
buck54321
buck54321 commented Sep 29, 2020
View reviewed changes
client/core/trade.go
@@ -804,15 +808,15 @@ func (t *trackedTrade) shouldBeginFindRedemption(match *matchTracker) bool {
}

// tick will check for and perform any match actions necessary.
func (t *trackedTrade) tick() (assetMap, error) {
func (c *Core) tick(t *trackedTrade) (assetMap, error) {
Copy link
Member Author

@buck54321 buck54321 Sep 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I've elevated a handful of methods to *Core methods. I didn't move them, because it would make the diff ridiculous, but I'll do some re-arranging in another PR.

I guess I did move runMatches though. My bad.

Copy link
Member

Choose a reason for hiding this comment

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

The main reason being that the finalize* functions now require calling (*Core).resolveMatchConflicts and on to one of the conflictResolvers? I'm prying because each one of the conflictResolvers seems to have the *Core input argument unused. I replaced (c *Core... with (_ *Core... on each one and there were no compilation issues. Perhaps the type matchConflictResolver func(*Core, *dexConnection, *trackedTrade, *matchTracker, *msgjson.MatchStatusResult) can be redefined without a *Core?

Copy link
Member Author

Choose a reason for hiding this comment

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

The main reason being that the finalize* functions now require calling (*Core).resolveMatchConflicts and on to one of the conflictResolvers?

Yep.

Perhaps the type matchConflictResolver func(*Core, *dexConnection, *trackedTrade, *matchTracker, *msgjson.MatchStatusResult) can be redefined without a *Core

Yes to that too.

Copy link
Member

@chappjc chappjc Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't this mean that redefining the receiver isn't necessary any more, noting that c.notify is replaceable because the notify function is in the trackedTrades too? Really though, I have no objection to the change. Just want to understand the considerations.

buck54321
buck54321 commented Sep 29, 2020
View reviewed changes
client/core/trade.go
Comment on lines +1234 to +1295
var msgErr *msgjson.Error
if errors.As(err, &msgErr) && msgErr.Code == msgjson.SettlementSequenceError {
// Try resolving the match status conflict.
go c.resolveMatchConflicts(t.dc, []*matchStatusConflict{{
trade: t,
match: match,
}})
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I actually don't think that we're likely to recover from this, so the call to resolveMatchConflicts is sort of a hail mary last ditch attempt to try to make sense of the situation.

@chappjc
Copy link
Member

chappjc commented Sep 30, 2020 •
edited
Loading

PR #677 is in status-integ now.

@buck54321 buck54321 force-pushed the status-resolution branch 2 times, most recently from 69bec54 to dbf3eba Compare September 30, 2020 16:53
@buck54321 buck54321 marked this pull request as ready for review September 30, 2020 20:04
chappjc
chappjc reviewed Sep 30, 2020
View reviewed changes
client/db/bolt/upgrades.go Show resolved Hide resolved
client/core/trade.go Show resolved Hide resolved
client/core/trade.go
@@ -804,15 +808,15 @@ func (t *trackedTrade) shouldBeginFindRedemption(match *matchTracker) bool {
}

// tick will check for and perform any match actions necessary.
func (t *trackedTrade) tick() (assetMap, error) {
func (c *Core) tick(t *trackedTrade) (assetMap, error) {
Copy link
Member

Choose a reason for hiding this comment

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

The main reason being that the finalize* functions now require calling (*Core).resolveMatchConflicts and on to one of the conflictResolvers? I'm prying because each one of the conflictResolvers seems to have the *Core input argument unused. I replaced (c *Core... with (_ *Core... on each one and there were no compilation issues. Perhaps the type matchConflictResolver func(*Core, *dexConnection, *trackedTrade, *matchTracker, *msgjson.MatchStatusResult) can be redefined without a *Core?

chappjc
chappjc reviewed Sep 30, 2020
View reviewed changes
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/status.go Show resolved Hide resolved
client/core/status.go Outdated Show resolved Hide resolved
client/core/status.go Show resolved Hide resolved
client/core/status.go Outdated Show resolved Hide resolved
client/core/status.go Outdated Show resolved Hide resolved
client/core/status.go Show resolved Hide resolved
client/core/trade.go Show resolved Hide resolved
@chappjc chappjc force-pushed the status-integ branch 2 times, most recently from 459a742 to 139c7fb Compare October 2, 2020 15:58
@buck54321 @chappjc
match_status resolution
5f8f43e 
1. Match status resolution is run on startup and reconnects for any
   status mismatches observed when comparing the `connect` result with
   our local state.

2. Client will now begin negotiation based on the extra matches in the
   connect response.

3. Differentiate between server-initiated revocations and
   client-initiated revocations. Required a database upgrade.

- Log error and issue notification for active orders reported by DEX
  that are unknown to the client.
- Update order statuses where server-reported statuses are different
  from statuses recorded by client.
- Set statuses to Executed, Canceled or Revoked for orders considered
  active by the client but not returned by the DEX in the connect
  response.
@chappjc chappjc merged commit d46b3d9 into decred:status-integ Oct 2, 2020
This was referenced Oct 2, 2020
Merged
Closed
@itswisdomagain itswisdomagain mentioned this pull request Oct 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chappjc chappjc chappjc approved these changes

Assignees
No one assigned
Labels
high priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

use match_status route
2 participants
@buck54321 @chappjc