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

core: handle epoch order messages. #188

Merged
merged 2 commits into from
Mar 10, 2020
Merged

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Feb 22, 2020

This adds the epoch_order notification handler to the client core.

This updates the epoch queue to track the
current epoch and implements the associated
epoch order handler.
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

It looks like the EpochQueue methods may need to lock for most or all of the function.

client/core/core.go Outdated Show resolved Hide resolved
client/order/epochqueue.go Outdated Show resolved Hide resolved
client/order/epochqueue.go Show resolved Hide resolved
match := false
for _, mkt := range dc.markets {
if mkt.Display() == note.MarketID {
match = true
Copy link
Member

@chappjc chappjc Feb 26, 2020

Choose a reason for hiding this comment

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

Could just return here and get rid of the match variable. Regarding it being an error, let me see if I understand: If Core is only setup for market X, but messages for market Y come in, that means the client has somehow registered on the orderbook route (which include the epoch order notes) for market Y or the server is misbehaving and sending messages that weren't what the client registered for. Does that sound right? In the first case, that's clearly an Core error, but in the second that's the server messing up.

Copy link
Member Author

@dnldd dnldd Feb 26, 2020

Choose a reason for hiding this comment

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

I'd say both cases outlined apply, and they're both errors, no?

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 should be breaking after setting match however.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right, the check below is for !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.

updated the commit, would like to squash it if it's all good now.

Copy link
Member

@chappjc chappjc Feb 26, 2020

Choose a reason for hiding this comment

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

TBH, I prefer not squashing until the review is approved, if at all. It helps reviewers follow changes, and github squashes for us on merge.

I just remove commit descriptions that don't describe a net effect of the PR, although I welcome squash commit messages in comments.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/order/epochqueue.go Outdated Show resolved Hide resolved
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

OK. We will probably scrutinize some more in #189, so I'm good with this for now.

@chappjc chappjc requested a review from buck54321 March 9, 2020 16:21
@chappjc chappjc merged commit 8aa9ceb into decred:master Mar 10, 2020
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