snowbridge: allow account conversion for Ethereum accounts#6221
Conversation
acatangiu
force-pushed
the
enhance-ethereum-account-derivation
branch
from
c06253a to
bebb62f
Compare
| fn convert_location(location: &Location) -> Option<AccountId> { | ||
| match location.unpack() { | ||
| (_, [GlobalConsensus(Ethereum { chain_id })]) => | ||
| (2, [GlobalConsensus(Ethereum { chain_id })]) => |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
(btw: I am using this new converter in XCMv5 tests to Transact on parachain from Ethereum/Bob) |
acatangiu
force-pushed
the
enhance-ethereum-account-derivation
branch
from
bebb62f to
1123e78
Compare
| fn test_ethereum_network_converts_successfully() { | ||
| let expected_account: [u8; 32] = | ||
| hex!("ce796ae65569a670d0c1cc1ac12515a3ce21b5fbf729d63d7b289baad070139d"); | ||
| let contract_location = Location::new(2, [GlobalConsensus(NETWORK)]); |
There was a problem hiding this comment.
| let contract_location = Location::new(2, [GlobalConsensus(NETWORK)]); | |
| let ethereum_location = Location::new(2, [GlobalConsensus(NETWORK)]); |
There was a problem hiding this comment.
Nit:
I'd squash first three tests into a single table-driven one.
Just like I did here
struct TestCase {
description,
location,
want
}
* master: (129 commits) pallet-revive: Use `RUSTUP_TOOLCHAIN` if set (#6365) [eth-rpc] proxy /health (#6360) [Release|CI/CD] adjust release pipelines (#6366) Bump the known_good_semver group across 1 directory with 3 updates (#6339) Run check semver in MQ (#6287) [Deprecation] deprecate treasury `spend_local` call and related items (#6169) refactor and harden check_core_index (#6217) litep2p: Update litep2p to v0.8.0 (#6353) [pallet-staking] Additional check for virtual stakers (#5985) migrate pallet-remarks to v2 bench syntax (#6291) Remove leftover references of Wococo (#6361) snowbridge: allow account conversion for Ethereum accounts (#6221) authority-discovery: Populate DHT records with public listen addresses (#6298) Bounty Pallet: add `approve_bounty_with_curator` call to `bounties` pallet (#5961) Silent annoying log (#6351) [pallet-revive] rework balance transfers (#6187) `statement-distribution`: RFC103 implementation (#5883) Disable flaky tests reported in #6343 / #6345 (#6346) migrate pallet-recovery to benchmark V2 syntax (#6299) inclusion emulator: correctly handle UMP signals (#6178) ...
acatangiu
moved this to SDK Released - Needs Integration
in fellowship/runtimes integrations queue
acatangiu
moved this from SDK Released - Needs Integration
to Integrated - Done
in fellowship/runtimes integrations queue
github-project-automation
bot
moved this from Todo
to Done
in [preview] release tracker
7 participants
Replace
GlobalConsensusEthereumConvertsForwithEthereumLocationsConverterForthat allowsLocationtoAccountIdconversion for the Ethereum network root as before, but also for Ethereum contracts and accounts.The new converter only matches explicit
parents: 2Ethereum locations, meaning it should be used only on/by parachains.