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.

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

Merged
merged 35 commits into from
Jan 25, 2023

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Jan 22, 2023

When a message failed, a user can now:

  • call returnEther on the source Bridge to get his Ether back (depositAmount + callAmount)
  • call 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.

@dantaik dantaik changed the title feat(protocol): implement releaseEther in Bridge feat(protocol): implement releaseEther & releaseERC20 Jan 22, 2023
@codecov
Copy link

codecov bot commented Jan 22, 2023

Codecov Report

Merging #13008 (1710088) into main (45153d9) will decrease coverage by 0.81%.
The diff coverage is 19.23%.

@@            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              
Flag Coverage Δ *Carryforward flag
bridge-ui 92.61% <ø> (ø) Carriedforward from 6a650b1
protocol 57.01% <19.23%> (-1.34%) ⬇️
relayer 69.10% <ø> (ø) Carriedforward from 6a650b1
ui 100.00% <ø> (ø) Carriedforward from 6a650b1

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
packages/protocol/contracts/bridge/Bridge.sol 46.66% <0.00%> (-5.19%) ⬇️
...s/protocol/contracts/bridge/libs/LibBridgeData.sol 100.00% <ø> (ø)
...protocol/contracts/bridge/libs/LibBridgeInvoke.sol 100.00% <ø> (ø)
...rotocol/contracts/bridge/libs/LibBridgeProcess.sol 17.64% <0.00%> (ø)
...rotocol/contracts/bridge/libs/LibBridgeRelease.sol 0.00% <0.00%> (ø)
...contracts/test/bridge/libs/TestLibBridgeInvoke.sol 100.00% <ø> (ø)
packages/protocol/contracts/bridge/TokenVault.sol 50.57% <22.72%> (-10.30%) ⬇️
packages/protocol/contracts/bridge/EtherVault.sol 76.19% <28.57%> (-23.81%) ⬇️
.../protocol/contracts/bridge/libs/LibBridgeRetry.sol 94.44% <100.00%> (ø)
...protocol/contracts/bridge/libs/LibBridgeStatus.sol 60.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dantaik dantaik marked this pull request as ready for review January 22, 2023 07:07
@dantaik dantaik requested a review from davidtaikocha January 23, 2023 09:33
@RogerLamTd
Copy link
Contributor

Is LibBridgeRefund perhaps a better name if we're using it for refunding failed transactions?

@dantaik dantaik changed the title feat(protocol): implement returnEther & returnERC20 feat(protocol): implement releaseEther & releaseERC20 Jan 24, 2023
@cyberhorsey
Copy link
Contributor

@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.

@dantaik dantaik requested a review from RogerLamTd January 24, 2023 23:25
@dantaik dantaik enabled auto-merge (squash) January 24, 2023 23:26
@dantaik dantaik merged commit 088933e into main Jan 25, 2023
@dantaik dantaik deleted the protocol_release_tokens_step2 branch January 25, 2023 09:04
@github-actions github-actions bot mentioned this pull request Jan 25, 2023
@@ -135,17 +148,20 @@ contract TokenVault is EssentialContract {
message.refundAddress = refundAddress;
message.memo = memo;

require(message.callValue == 0, "V:callValue");
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

@dionysuzx
Copy link
Collaborator

@dantaik it is OK to merge this 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.

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 ?

@shadab-taiko
Copy link
Contributor

shadab-taiko commented Feb 2, 2023

@dantaik it is OK to merge this 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.

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 ?
Any how we are already tracking this in #12960

@cyberhorsey
Copy link
Contributor

cyberhorsey commented Feb 2, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feat(bridge): Add Failed state for Bridge and releaseTokens method on TokenVault
6 participants