-
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
Return penalty to staker if target chain has already progressed #576
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.
Please resolve the conflicts.
@sarvesh-ost Done! |
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 👍
I have added a few inline comments.
We also similar changes in progressRedeemWithProof
flow
@deepesh-kn I incorporated all your feedback, execpt for the one I left a comment for. |
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 work 👍 🙌
Some minor comments.
@sarvesh-ost I moved the |
@sarvesh-ost @deepesh-kn Ready 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.
Changes looks good for progressStakeWithProof
. 🙌
As requested by @deepesh-kn , Similar feature is required for progressRedeemWithProof
.
Refer : #409
Oh wow, completely overlooked that 🙈 |
@sarvesh-ost @deepesh-kn I added the version for |
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 🐼 🚀
Minor typo needs correction and also one question on gas consumption in the unit test. 👍
contracts/gateway/EIP20CoGateway.sol
Outdated
@@ -546,8 +546,9 @@ contract EIP20CoGateway is GatewayBase { | |||
); | |||
|
|||
MessageBus.Message storage message = messages[_messageHash]; | |||
MessageBus.MessageStatus outboxMessageStatus = | |||
messageBox.outbox[_messageHash]; |
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.
messageBox.outbox[_messageHash]; | |
messageBox.outbox[_messageHash]; |
contracts/gateway/EIP20CoGateway.sol
Outdated
return; | ||
} | ||
|
||
// Penalty charged to staker for revert redeem. |
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.
// Penalty charged to staker for revert redeem. | |
// Penalty charged to redeemer for revert redeem. |
|
||
const expectedFinalFacilitatorETHBalance = initialFacilitatorEthBalance | ||
.add(bountyAmount) | ||
.subn(tx.receipt.gasUsed); |
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.
Should this not be multiplied with gas price?
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.
The gas price is set to 1
in truffle.js
, which I think is the reason that it works. But yeah, in theory it would need to be multiplied with the gas price.
- Align with inbox/outbox naming - Moved returning penalty after progressStakeInternal and collect relevant values before that
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 🚢
FIXES #409
Waiting for #578 being merged.