-
Notifications
You must be signed in to change notification settings - Fork 31
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
Remove signature from stake #548
Remove signature from stake #548
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.
Good code and nice clean-up 🚁 🚁 🎧
It looks good to me 🎉 But someone else definitely has to take a look at this (@deepesh-kn).
Is it really as easy as just removing the signature? No other side effects from that?
Is it really required to have that much code in the mock message bus, though? That looks dangerous to me
abi.encodePacked(path) | ||
) | ||
) | ||
); |
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.
Is it correct to be hashed twice? 🤔
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.
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.
Is it possibly a bug in the JS as well? What's the reason for hashing it twice?
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.
Available for review
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.
Nice 👍
Just a few comments.
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.
Available for review again.
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.
LGTM 🏆
This pr fixes #539
The following features are implemented as a part of this PR.
EIP20Gateway.stake
is called by Staker without providing staker signature.MessageBus.declareMessage
also modified to not accept the signature.As bounty is transferred from staker/redeemer, the gateway only knows about one actor.
Gateway doesn't care about how staker and facilitator shares hash lock, unlock secret and bounty.
Note: This PR didn't touch unit test cases for
ProgressOutboxWithProof
as those are covered in #526. Also, one test forProgressInboxWithProof
which tests that transaction is reverted if Merkel proof verification failed should be covered after #526.