-
Notifications
You must be signed in to change notification settings - Fork 502
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
Fix auction resolution during a tie #2971
Conversation
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.
This looks like it matches the contract now, nice work. The test is also very good, I just have one question about it, but I'll approve this for now and you can fix if there's something to be fixed.
system_tests/timeboost_test.go
Outdated
|
||
} | ||
|
||
var expectedWinner common.Address |
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.
Does the bidHash change between runs of the test so that both alice and bob being the expected winner are tested? Could we run this test a few times then?
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.
Since hashing is pseudo-random its hard to deterministically say one should win out during a tie!
I agree with you and that we can have another test that would keep running multiple auctions until both alice and bob winning a tie is tested out- but this test would need to be skipped in CI as it would take a lot of time
This PR fixes how we broke tie on the GO side and matches it to how the auction contract breaks it.
Resolves NIT-3132