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: allow account conversion for Ethereum accounts #6221

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions bridges/snowbridge/primitives/router/src/inbound/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ where

let bridge_location = Location::new(2, GlobalConsensus(network));

let owner = GlobalConsensusEthereumConvertsFor::<[u8; 32]>::from_chain_id(&chain_id);
let owner = EthereumLocationsConverterFor::<[u8; 32]>::from_chain_id(&chain_id);
let asset_id = Self::convert_token_address(network, token);
let create_call_index: [u8; 2] = CreateAssetCall::get();
let inbound_queue_pallet_index = InboundQueuePalletInstance::get();
Expand Down Expand Up @@ -454,22 +454,27 @@ where
}
}

pub struct GlobalConsensusEthereumConvertsFor<AccountId>(PhantomData<AccountId>);
impl<AccountId> ConvertLocation<AccountId> for GlobalConsensusEthereumConvertsFor<AccountId>
pub struct EthereumLocationsConverterFor<AccountId>(PhantomData<AccountId>);
impl<AccountId> ConvertLocation<AccountId> for EthereumLocationsConverterFor<AccountId>
where
AccountId: From<[u8; 32]> + Clone,
{
fn convert_location(location: &Location) -> Option<AccountId> {
match location.unpack() {
(_, [GlobalConsensus(Ethereum { chain_id })]) =>
(2, [GlobalConsensus(Ethereum { chain_id })]) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why be explicit on the parents here instead? Wouldnt this limit its usage to being relative to parachains?

It would be parents 1 if relative to the relaychain. It would also be parents 1 if relative to another Ethereum chain. This wont break any existing use cases though so maybe its fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot to mention this as well in the description (edited now).

I prefer tightening the match to explicit parents: 2 since this converter is meant to be used by parachains only. I can't think of a valid (long-term) use case for the Relay chain to need to derive Ethereum locations.

It would also be parents 1 if relative to another Ethereum chain.

You mean if you use this converter in an Ethereum SC? I don't think there is such a real case, but if there will ever be I suggest adding a new entry to the match there or even using a separate converter since they would be different in output anyway (32bytes vs 20bytes).

Copy link
Contributor

@yrong yrong Oct 25, 2024

Choose a reason for hiding this comment

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

It would be parents 1 if relative to the relaychain

@alistair-singh Are you refering to another convert to convert a token location of PNA to a stable ID.

Then It's nothing to do with the above change, the above is for ENA and should be fine to set parents to 2.

Some(Self::from_chain_id(chain_id).into()),
(2, [GlobalConsensus(Ethereum { chain_id }), AccountKey20 { network: _, key }]) =>
Some(Self::from_chain_id_with_key(chain_id, *key).into()),
_ => None,
}
}
}

impl<AccountId> GlobalConsensusEthereumConvertsFor<AccountId> {
impl<AccountId> EthereumLocationsConverterFor<AccountId> {
pub fn from_chain_id(chain_id: &u64) -> [u8; 32] {
(b"ethereum-chain", chain_id).using_encoded(blake2_256)
}
pub fn from_chain_id_with_key(chain_id: &u64, key: [u8; 20]) -> [u8; 32] {
(b"ethereum-chain", chain_id, key).using_encoded(blake2_256)
}
}
24 changes: 19 additions & 5 deletions bridges/snowbridge/primitives/router/src/inbound/tests.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:
I'd squash first three tests into a single table-driven one.
Just like I did here
struct TestCase {
description,
location,
want
}

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::GlobalConsensusEthereumConvertsFor;
use super::EthereumLocationsConverterFor;
use crate::inbound::CallIndex;
use frame_support::{assert_ok, parameter_types};
use hex_literal::hex;
Expand All @@ -17,14 +17,28 @@ parameter_types! {
}

#[test]
fn test_contract_location_with_network_converts_successfully() {
fn test_ethereum_network_converts_successfully() {
let expected_account: [u8; 32] =
hex!("ce796ae65569a670d0c1cc1ac12515a3ce21b5fbf729d63d7b289baad070139d");
let contract_location = Location::new(2, [GlobalConsensus(NETWORK)]);

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let contract_location = Location::new(2, [GlobalConsensus(NETWORK)]);
let ethereum_location = Location::new(2, [GlobalConsensus(NETWORK)]);

let account =
GlobalConsensusEthereumConvertsFor::<[u8; 32]>::convert_location(&contract_location)
.unwrap();
EthereumLocationsConverterFor::<[u8; 32]>::convert_location(&contract_location).unwrap();

assert_eq!(account, expected_account);
}

#[test]
fn test_contract_location_with_network_converts_successfully() {
let expected_account: [u8; 32] =
hex!("9038d35aba0e78e072d29b2d65be9df5bb4d7d94b4609c9cf98ea8e66e544052");
let contract_location = Location::new(
2,
[GlobalConsensus(NETWORK), AccountKey20 { network: None, key: [123u8; 20] }],
);

let account =
EthereumLocationsConverterFor::<[u8; 32]>::convert_location(&contract_location).unwrap();

assert_eq!(account, expected_account);
}
Expand All @@ -34,7 +48,7 @@ fn test_contract_location_with_incorrect_location_fails_convert() {
let contract_location = Location::new(2, [GlobalConsensus(Polkadot), Parachain(1000)]);

assert_eq!(
GlobalConsensusEthereumConvertsFor::<[u8; 32]>::convert_location(&contract_location),
EthereumLocationsConverterFor::<[u8; 32]>::convert_location(&contract_location),
None,
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use snowbridge_pallet_inbound_queue_fixtures::{
};
use snowbridge_pallet_system;
use snowbridge_router_primitives::inbound::{
Command, Destination, GlobalConsensusEthereumConvertsFor, MessageV1, VersionedMessage,
Command, Destination, EthereumLocationsConverterFor, MessageV1, VersionedMessage,
};
use sp_core::H256;
use sp_runtime::{DispatchError::Token, TokenError::FundsUnavailable};
Expand Down Expand Up @@ -318,8 +318,7 @@ fn send_token_from_ethereum_to_penpal() {

// Fund ethereum sovereign on AssetHub
let ethereum_sovereign: AccountId =
GlobalConsensusEthereumConvertsFor::<AccountId>::convert_location(&origin_location)
.unwrap();
EthereumLocationsConverterFor::<AccountId>::convert_location(&origin_location).unwrap();
AssetHubRococo::fund_accounts(vec![(ethereum_sovereign.clone(), INITIAL_FUND)]);

// Create asset on the Penpal parachain.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use hex_literal::hex;
use rococo_westend_system_emulated_network::asset_hub_westend_emulated_chain::genesis::AssetHubWestendAssetOwner;
use snowbridge_core::{outbound::OperatingMode, AssetMetadata, TokenIdOf};
use snowbridge_router_primitives::inbound::{
Command, Destination, GlobalConsensusEthereumConvertsFor, MessageV1, VersionedMessage,
Command, Destination, EthereumLocationsConverterFor, MessageV1, VersionedMessage,
};
use sp_core::H256;
use testnet_parachains_constants::westend::snowbridge::EthereumNetwork;
Expand Down Expand Up @@ -297,7 +297,7 @@ fn transfer_relay_token() {
let expected_token_id = TokenIdOf::convert_location(&expected_asset_id).unwrap();

let ethereum_sovereign: AccountId =
GlobalConsensusEthereumConvertsFor::<[u8; 32]>::convert_location(&Location::new(
EthereumLocationsConverterFor::<[u8; 32]>::convert_location(&Location::new(
2,
[GlobalConsensus(EthereumNetwork::get())],
))
Expand Down Expand Up @@ -445,7 +445,7 @@ fn transfer_ah_token() {
let ethereum_destination = Location::new(2, [GlobalConsensus(Ethereum { chain_id: CHAIN_ID })]);

let ethereum_sovereign: AccountId =
GlobalConsensusEthereumConvertsFor::<[u8; 32]>::convert_location(&ethereum_destination)
EthereumLocationsConverterFor::<[u8; 32]>::convert_location(&ethereum_destination)
.unwrap()
.into();
AssetHubWestend::fund_accounts(vec![(ethereum_sovereign.clone(), INITIAL_FUND)]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ use parachains_common::{
};
use polkadot_parachain_primitives::primitives::Sibling;
use polkadot_runtime_common::xcm_sender::ExponentialPrice;
use snowbridge_router_primitives::inbound::GlobalConsensusEthereumConvertsFor;
use snowbridge_router_primitives::inbound::EthereumLocationsConverterFor;
use sp_runtime::traits::{AccountIdConversion, ConvertInto, TryConvertInto};
use testnet_parachains_constants::rococo::snowbridge::{
EthereumNetwork, INBOUND_QUEUE_PALLET_INDEX,
Expand Down Expand Up @@ -104,7 +104,7 @@ pub type LocationToAccountId = (
GlobalConsensusParachainConvertsFor<UniversalLocation, AccountId>,
// Ethereum contract sovereign account.
// (Used to get convert ethereum contract locations to sovereign account)
GlobalConsensusEthereumConvertsFor<AccountId>,
EthereumLocationsConverterFor<AccountId>,
);

/// Means for transacting the native currency on this chain.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ use parachains_common::{
};
use polkadot_parachain_primitives::primitives::Sibling;
use polkadot_runtime_common::xcm_sender::ExponentialPrice;
use snowbridge_router_primitives::inbound::GlobalConsensusEthereumConvertsFor;
use snowbridge_router_primitives::inbound::EthereumLocationsConverterFor;
use sp_runtime::traits::{AccountIdConversion, ConvertInto, TryConvertInto};
use xcm::latest::prelude::*;
use xcm_builder::{
Expand Down Expand Up @@ -100,7 +100,7 @@ pub type LocationToAccountId = (
GlobalConsensusParachainConvertsFor<UniversalLocation, AccountId>,
// Ethereum contract sovereign account.
// (Used to get convert ethereum contract locations to sovereign account)
GlobalConsensusEthereumConvertsFor<AccountId>,
EthereumLocationsConverterFor<AccountId>,
);

/// Means for transacting the native currency on this chain.
Expand Down
10 changes: 10 additions & 0 deletions prdoc/pr_6221.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
title: "snowbridge: allow account conversion for Ethereum accounts"

doc:
- audience: Runtime Dev
description: |
Replaced `GlobalConsensusEthereumConvertsFor` with `EthereumLocationsConverterFor` that allows `Location`
to `AccountId` conversion for the Ethereum network root as before, but also for Ethereum contracts and accounts.
crates:
- name: snowbridge-router-primitives
bump: major
Loading