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

Snowbridge: Support bridging native ETH #6855

Merged
merged 20 commits into from
Jan 7, 2025

Conversation

alistair-singh
Copy link
Contributor

@alistair-singh alistair-singh commented Dec 12, 2024

Changes:

  1. Use the 0x0000000000000000000000000000000000000000 token address as Native ETH.
  2. Convert it to/from { parents: 2, interior: X1(GlobalConsensus(Ethereum{chain_id: 1})) } when encountered.

Onchain changes:
This will require a governance request to register native ETH (with the above location) in the foreign assets pallet and make it sufficient.

Related solidity changes: Snowfork/snowbridge#1354

TODO:

  • Emulated Tests

@alistair-singh alistair-singh changed the base branch from stable2409 to master December 17, 2024 19:39
@vgeddes
Copy link
Contributor

vgeddes commented Dec 18, 2024

The approach looks good!

@vgeddes
Copy link
Contributor

vgeddes commented Dec 20, 2024

Will approve once you've implemented the emulated test that verifies the full BH->AH path.

@alistair-singh alistair-singh marked this pull request as ready for review December 21, 2024 21:50
@paritytech-review-bot paritytech-review-bot bot requested a review from a team December 21, 2024 21:51
kku9706

This comment was marked as spam.

Comment on lines +518 to +522
/// Tests the full cycle of eth transfers:
/// - sending a token to AssetHub
/// - returning the token to Ethereum
#[test]
fn send_eth_asset_from_asset_hub_to_ethereum_and_back() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Rococo deprecated so we may need to add new features/tests to Westend instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah agree, I did not want to change all our test boilerplate in this PR to keep it focused. But I think we need to in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this test (and others) needs to move to westend - can be followup PR, but pls do it

Copy link
Contributor

@yrong yrong left a comment

Choose a reason for hiding this comment

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

+1

type RuntimeEvent = <AssetHubRococo as Chain>::RuntimeEvent;
type RuntimeOrigin = <AssetHubRococo as Chain>::RuntimeOrigin;

let _issued_event = RuntimeEvent::ForeignAssets(pallet_assets::Event::Issued {
Copy link
Contributor

Choose a reason for hiding this comment

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

why the the underscore prepended to the variable name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assert_expected_events! macro does not "use" the variable from the perspective of the compiler. I think the token is copied into the macro body, so it leaves this variable unused. Using underscore silences the warning.

)
.unwrap();

let _burned_event = RuntimeEvent::ForeignAssets(pallet_assets::Event::Burned {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, why the underscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commented above.

@@ -291,6 +291,7 @@ where
match inner_location.unpack() {
(0, [AccountKey20 { network, key }]) if self.network_matches(network) =>
Some((H160(*key), *amount)),
(0, []) => Some((H160([0; 20]), *amount)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment for this case explaining why what we are trying to do here, for example, UnlockNativeToken command supports unlocking ether if token address is NULL.

Copy link
Contributor Author

@alistair-singh alistair-singh Dec 23, 2024

Choose a reason for hiding this comment

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

Address in 17d98b7.

Copy link
Contributor

@yrong yrong Jan 1, 2025

Choose a reason for hiding this comment

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

Convert it to/from { parents: 2, interior: X1(GlobalConsensus(Ethereum{chain_id: 1})) } when encountered

Could we use location { parents: 2, interior: X2[GlobalConsensus(Ethereum{chain_id: 1}),AccountKey20 { network: None, key: H160::zero().into() }] } to represent Ether?

To be consistent with other ERC20 assets, then I would assume there is no need for this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point. But I think using the base { parents: 2, interior: X1(GlobalConsensus(Ethereum{chain_id: 1})) } is more intuitive so I prefer the current version. But lets discuss @vgeddes

2,
[GlobalConsensus(network), AccountKey20 { network: None, key: token.into() }],
)
if token == H160([0; 20]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment here explaining that a token address of 0x00... is equivalent to native ether.

Copy link
Contributor Author

@alistair-singh alistair-singh Dec 23, 2024

Choose a reason for hiding this comment

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

Address in 17d98b7.

@acatangiu acatangiu added the T15-bridges This PR/Issue is related to bridges. label Jan 7, 2025
Comment on lines +518 to +522
/// Tests the full cycle of eth transfers:
/// - sending a token to AssetHub
/// - returning the token to Ethereum
#[test]
fn send_eth_asset_from_asset_hub_to_ethereum_and_back() {
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this test (and others) needs to move to westend - can be followup PR, but pls do it

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Not a XCM expert, left one DQ otherwise it looks reasonable.

bridges/snowbridge/primitives/router/Cargo.toml Outdated Show resolved Hide resolved
// If the token is `0x0000000000000000000000000000000000000000` then return the location of
// native Ether.
if token == H160([0; 20]) {
Location::new(2, [GlobalConsensus(network)])
Copy link
Member

Choose a reason for hiding this comment

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

DQ: Don't we need to check here that network == Ethereum(chain_id: 1)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that too and checked if it needs validation, it doesn't: convert_token_address() is internal fn called by convert_register_token() or convert_send_token() which are converting Ethereum commands into XCM programs, somewhere above on the callstack the source was already validated as being Ethereum and the only thing that varies here is the Ethereum chain_id (testnet or prod).

Maybe for clarity and hardening this helper function should take only chain_id and hardcode Ethereum as network:

- 	fn convert_token_address(network: NetworkId, token: H160) -> Location {
+	fn convert_token_address(chain_id: u64, token: H160) -> Location {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey thanks for setting the label. So the chain_id comes directly from the message encoded on the solidity side, where it is taken from the on-chain variable block.chainid

  1. Passed in here:
  2. From message body here:
    V1(MessageV1 { chain_id, command: SendToken { token, destination, amount, fee } }) =>
  3. Decoded here:
    let message = VersionedMessage::decode_all(&mut envelope.payload.as_ref())
    .map_err(|_| Error::<T>::InvalidPayload)?;
  4. Encoded in solidity here, taken directly from the block:
https://github.com/Snowfork/snowbridge/blob/20a804669e0da59cff01b09588e16f1267d98862/contracts/src/SubstrateTypes.sol#L81

So it does not need to be validated explicitly. It will be validated implicitly by message verification through the light client.

@bkchr bkchr added this pull request to the merge queue Jan 7, 2025
Merged via the queue into paritytech:master with commit 4059282 Jan 7, 2025
200 of 203 checks passed
@acatangiu acatangiu added the A4-needs-backport Pull request must be backported to all maintained releases. label Jan 8, 2025
@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2407:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-6855-to-stable2407
git worktree add --checkout .worktree/backport-6855-to-stable2407 backport-6855-to-stable2407
cd .worktree/backport-6855-to-stable2407
git reset --hard HEAD^
git cherry-pick -x 4059282fc7b6ec965cc22a9a0df5920a4f3a4101
git push --force-with-lease

@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2409:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-6855-to-stable2409
git worktree add --checkout .worktree/backport-6855-to-stable2409 backport-6855-to-stable2409
cd .worktree/backport-6855-to-stable2409
git reset --hard HEAD^
git cherry-pick -x 4059282fc7b6ec965cc22a9a0df5920a4f3a4101
git push --force-with-lease

@acatangiu
Copy link
Contributor

acatangiu commented Jan 8, 2025

hmm, any way to trigger the backport bot after the PR was merged? cc @EgorPopelyaev

L.E.: Nevermind, bot picked it up automatically 💪

github-actions bot pushed a commit that referenced this pull request Jan 8, 2025
Changes:
1. Use the 0x0000000000000000000000000000000000000000 token address as
Native ETH.
2. Convert it to/from `{ parents: 2, interior:
X1(GlobalConsensus(Ethereum{chain_id: 1})) }` when encountered.

Onchain changes:
This will require a governance request to register native ETH (with the
above location) in the foreign assets pallet and make it sufficient.

Related solidity changes:
Snowfork/snowbridge#1354

TODO:
- [x] Emulated Tests

---------

Co-authored-by: Vincent Geddes <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
(cherry picked from commit 4059282)
@paritytech-cmd-bot-polkadot-sdk

Successfully created backport PR for stable2412:

use sp_core::U256;
use sp_std::vec;

pub fn make_send_native_eth_message() -> InboundQueueFixture {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a const fn.

@alistair-singh alistair-singh deleted the alistair/support-eth branch January 9, 2025 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A4-needs-backport Pull request must be backported to all maintained releases. T15-bridges This PR/Issue is related to bridges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants