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

fix(p2p): order invalidation packet #774

Merged
merged 3 commits into from
Dec 26, 2018
Merged

fix(p2p): order invalidation packet #774

merged 3 commits into from
Dec 26, 2018

Conversation

moshababo
Copy link
Collaborator

  • fix typing mismatch in order invalidation conversion return type
  • fix the tests to be able to find such problems
  • adding data integrity errors logging/penalization (taken from feat(p2p): encryption & authentication #756)

Still need to check if it actually solves #767 or that we have another problem.

@moshababo moshababo requested a review from offerm December 26, 2018 10:31
@ghost ghost assigned moshababo Dec 26, 2018
@ghost ghost added the in progress label Dec 26, 2018
sangaman
sangaman approved these changes Dec 26, 2018
View reviewed changes
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

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

Looks good, and good catch on the type being returned for OrderInvalidationPackets. I'm just not sure the type check in the tests makes a difference, and it'd be nice to reword the commits to follow the commit template.

test/unit/Parser.spec.ts Show resolved Hide resolved
@moshababo
Copy link
Collaborator Author

moshababo commented Dec 26, 2018 •
edited
Loading

I was thinking to squash the commits, but i'll see if I can easily rename them although they were pushed, and merge without squash.

offerm
offerm approved these changes Dec 26, 2018
View reviewed changes
Copy link
Contributor

@offerm offerm left a comment

Choose a reason for hiding this comment

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

Based on testing it looks good.

@sangaman
Copy link
Collaborator

Squash merging is fine too, however you think is best but just to keep the template format. Thanks!

@moshababo moshababo merged commit 962124f into master Dec 26, 2018
@ghost ghost removed the in progress label Dec 26, 2018
@michael1011 michael1011 deleted the order_invalidation_fix branch March 14, 2019 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sangaman sangaman sangaman approved these changes

@offerm offerm offerm approved these changes

Assignees

@moshababo moshababo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@moshababo @sangaman @offerm