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

Ensure wrapped ERC20 transfers are correctly authorized and add end-to-end tests for them #1112

Merged

Conversation

james-chf
Copy link
Contributor

@james-chf james-chf commented Feb 1, 2023

Closes #432 and relates to #508

This PR

  • ensures we check the relevant VPs are being executed for wrapped ERC20 transfers
  • adds end-to-end tests covering
    • transfers of DAI from Ethereum to implicit and established Namada accounts
    • authorized transfers of wrapped DAI between implicit <-> established accounts once on Namada
    • rejecting transactions that attempt to transfer wrapped DAI from implicit/established accounts, if the transactions aren't signed by the key which controls the sending account

NB: some helper fns added/used in this PR are also in https://github.com/anoma/namada/pull/886/files#diff-b26bbd416d9d500f8f12aee8f7b3182f35b5cacb748898d9c07fe2cdd41e69b8

@james-chf james-chf force-pushed the james/ethbridge/e2e-wrapped-erc20-transfers branch 5 times, most recently from 9c20e94 to 949421c Compare February 6, 2023 13:45
@james-chf james-chf marked this pull request as ready for review February 6, 2023 13:47
@james-chf james-chf requested review from batconjurer and sug0 February 6, 2023 13:47
@james-chf james-chf marked this pull request as draft February 6, 2023 13:48
@james-chf james-chf force-pushed the james/ethbridge/e2e-wrapped-erc20-transfers branch from 949421c to 4751573 Compare February 6, 2023 13:59
@james-chf james-chf marked this pull request as ready for review February 6, 2023 13:59
Copy link
Collaborator

@sug0 sug0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I'd change some of the INFO to DEBUG logs, but otherwise lgtm

verifiers: &BTreeSet<Address>,
sender: &Address,
) -> bool {
verifiers.contains(sender)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honest question, I've always wondered if we trigger the receiver vp as well. Or can people not refuse money from certain accounts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we should trigger the receiver account as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add this, will require some small refactoring.

Copy link
Member

@batconjurer batconjurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, I'm just waiting on an answer as to whether we trigger receiver vps for token transfers.

@batconjurer batconjurer self-requested a review February 7, 2023 09:28
Copy link
Member

@batconjurer batconjurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure the receiver vp also gets triggered.

@james-chf james-chf marked this pull request as draft February 7, 2023 12:43
…d-erc20-transfers

* eth-bridge-integration:
  Fix the validator set hashes signed over by secp keys
@james-chf james-chf marked this pull request as ready for review February 7, 2023 14:06
@james-chf james-chf requested a review from batconjurer February 7, 2023 14:06
@james-chf james-chf linked an issue Feb 8, 2023 that may be closed by this pull request
4 tasks
…d-erc20-transfers

* eth-bridge-integration:
  In tests crate, use eth bridge types via shared crate
  Use namada_ethereum_bridge via shared crate
  Use (not(feature = "abcipp"))
  Revert genesis.rs changes
  Fix up the e2e test
  Move oracle Config struct to ethereum_bridge crate
  Rename test
  Simplify setup::network call slightly
  Don't set any Ethereum bridge params in dev mode
  Revert "Remove e2e test until expectrl issue solved"

# Conflicts:
#	tests/src/e2e/eth_bridge_tests.rs
@batconjurer batconjurer merged commit 6919f4e into eth-bridge-integration Feb 16, 2023
@batconjurer batconjurer deleted the james/ethbridge/e2e-wrapped-erc20-transfers branch February 16, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

End-to-end tests for wrapped ERC20 transfers between accounts
3 participants