-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat(protocol): implement releaseEther & releaseERC20 #13008
Conversation
Codecov Report
@@ Coverage Diff @@
## main #13008 +/- ##
==========================================
- Coverage 65.89% 65.08% -0.81%
==========================================
Files 112 113 +1
Lines 3064 3105 +41
Branches 368 386 +18
==========================================
+ Hits 2019 2021 +2
- Misses 969 1008 +39
Partials 76 76
*This pull request uses carry forward flags. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Co-authored-by: David <[email protected]>
Co-authored-by: David <[email protected]>
Is LibBridgeRefund perhaps a better name if we're using it for refunding failed transactions? |
@dantaik it is OK to merge thsi in before alpha-2 testnet because we wont deploy this onto the current public testnet, so any protocol changes do not effect relayer. I have made an issue for met o catch up the relayer with the protocol and bridge changes. |
packages/website/pages/docs/reference/contract-documentation/bridge/EtherVault.md
Show resolved
Hide resolved
@@ -135,17 +148,20 @@ contract TokenVault is EssentialContract { | |||
message.refundAddress = refundAddress; | |||
message.memo = memo; | |||
|
|||
require(message.callValue == 0, "V:callValue"); |
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.
@dantaik can you explain why this check is needed? from my understanding, the memory is initialized, and the uint256 fields callValue
is initialized as 0
. And between the initialization and this require
statement, there is no modification to callValue
.
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.
That's true, I added this check to prevent future PRs to change the value of callValue
without realizing it must be 0.
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.
ok ill add a comment for that.
i think we also need an issue to update the bridge-ui to generate the proof from the signal service contract instead of the bridge, right @shadab-taiko ? |
Not sure if it has already been covered in https://github.com/taikoxyz/taiko-mono/pull/13059/files @cyberhorsey ? |
I think @d1onys1us is actually correct I will need to change where the eth_getProof call lands on both relayer and bridge ui, but I will do it after alpha testnet 2 is live so I can make sure it works as right now I would just be guessing. |
When a message failed, a user can now:
returnEther
on the source Bridge to get his Ether back (depositAmount + callAmount)returnERC20
on TokenVault to get his ERC20 token back@shadab-taiko @cyberhorsey Merging this PR will likely make the Bridge relayer no long work due to the change of events. Please let me know if we should postpone this PR and only merge it after the alpha-2 testnet.