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

fix(protocol): fix TokenVault.sendERC20 #420

Merged
merged 2 commits into from
Dec 13, 2022
Merged

fix(protocol): fix TokenVault.sendERC20 #420

merged 2 commits into from
Dec 13, 2022

Conversation

davidtaikocha
Copy link
Contributor

@davidtaikocha davidtaikocha commented Dec 12, 2022

When I re-read bridge's protocol today, i thought the depositValue when sending ERC-20 tokens should be msg.value - processingFee, otherwise, this message can not be sent unless the processingFee is 0? Because of the following check in LibBridgeSend.sendMessage:

uint256 expectedAmount = message.depositValue +
            message.callValue +
            message.processingFee;
require(expectedAmount == msg.value, "B:value");

@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #420 (d7fff0a) into main (e7ef53e) will decrease coverage by 0.23%.
The diff coverage is 48.88%.

@@            Coverage Diff             @@
##             main     #420      +/-   ##
==========================================
- Coverage   65.18%   64.95%   -0.24%     
==========================================
  Files          96       96              
  Lines        2605     2605              
  Branches      313      313              
==========================================
- Hits         1698     1692       -6     
- Misses        840      846       +6     
  Partials       67       67              
Flag Coverage Δ *Carryforward flag
bridge-ui 100.00% <ø> (ø) Carriedforward from e7ef53e
protocol 58.11% <0.00%> (-0.43%) ⬇️
relayer 68.34% <50.00%> (ø) Carriedforward from e7ef53e
ui 100.00% <ø> (ø) Carriedforward from e7ef53e

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

Impacted Files Coverage Δ
packages/protocol/contracts/bridge/TokenVault.sol 26.08% <0.00%> (ø)
packages/relayer/indexer/service.go 75.00% <0.00%> (ø)
packages/relayer/cli/cli.go 61.18% <23.07%> (ø)
packages/relayer/repo/event.go 75.00% <33.33%> (ø)
packages/relayer/http/get_events_by_address.go 50.00% <57.14%> (ø)
packages/relayer/indexer/filter_then_subscribe.go 19.75% <57.14%> (ø)
packages/relayer/message/process_message.go 67.54% <75.00%> (ø)
packages/relayer/message/processor.go 100.00% <100.00%> (ø)
...es/protocol/contracts/thirdparty/LibMerkleTrie.sol 83.13% <0.00%> (-7.23%) ⬇️
... and 6 more

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

@dantaik
Copy link
Contributor

dantaik commented Dec 13, 2022

I think this is indeed a bug, but I'll let @cyberhorsey to double-confirm and maybe add a test for it.
@cyberhorsey A nice bridge feature is when people deposit ERC20 tokens, they can also deposit Ether if they want in the same transaction. The bridge can achieve that.

@cyberhorsey cyberhorsey merged commit d42b953 into main Dec 13, 2022
@cyberhorsey cyberhorsey deleted the fix-sendERC20 branch December 13, 2022 22:07
@gitpoap-bot
Copy link

gitpoap-bot bot commented Dec 13, 2022

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 Taiko Contributor:

GitPOAP: 2022 Taiko Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@github-actions github-actions bot mentioned this pull request Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants