Snowbridge: Remove snowbridge-pallet-system::NativeToForeignId #730
Conversation
yrong
changed the title
|
Just added some tests in a182c17 to demonstrate the following:
Does this mean the migration might not even be nessessary? |
…/yrong/runtimes into ron/migration-token-id-to-xcm-v5
Migration test
|
system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_config.rs
Outdated
Show resolved
Hide resolved
|
@acatangiu Which release do you think we should include this migration? As I mentioned earlier in this comment, the V4-encoded location can be decoded in V5, which makes this PR technically unnessessary. However, I’d still prefer to merge it just for consistency and to avoid confusion. |
hmm, I don't understand the question really, this is a runtime migration defined at the runtime level, so not part of any crates released in Polkadot-SDK - nothing to backport ideally same should be done for Westend and Paseo, on their repos |
Sorry for the confusion - It's not a backport. I'm just curious has version 1.5.1 been released? Is that the first release to enable XCM V5 on BH? Do we want to include this migration in it?
We already did that on Westend. Paseo is not actively maintained at the moment. |
yes, 1.5.1 is released and live on system chains including BH (spec version is 1005001)
we should get this migration in master so that it's included with next release |
|
Review required! Latest push from author must always be reviewed |
yrong
force-pushed
the
migration-token-id-to-xcm-v5
branch
from
c459e39 to
f8f1d26
Compare
|
@bkontur @franciscoaguirre Thanks for the review.
Yeah, it's techniquelly not required, as I mentioned in #730 (comment) - it's actually a no-op operation.
Nice catch. I've made changes accordingly and tested with |
yrong
changed the title
snowbridge-pallet-system::NativeToForeignId
| bridge_to_kusama_config::XcmOverBridgeHubKusamaInstance, | ||
| >, | ||
| snowbridge_pallet_system::migration::FeePerGasMigrationV0ToV1<Runtime>, | ||
| bridge_to_ethereum_config::migration::RemoveNativeToForeignId<Runtime>, |
There was a problem hiding this comment.
@yrong this PR should really contain just adding one migration and nothing else :), for example:
| bridge_to_ethereum_config::migration::RemoveNativeToForeignId<Runtime>, | |
| frame_support::migrations::RemoveStorage< | |
| "EthereumSystem", // <- define as &str param_type | |
| "NativeToForeignId", // <- define as &str param_type | |
| RocksDbWeight, | |
| >, |
for example check in polkadot-sdk:
frame_support::migrations::RemoveStorage<
BridgeWestendMessagesPalletName,
OutboundLanesCongestedSignalsKey,
RocksDbWeight,
>
There was a problem hiding this comment.
@bkontur The tests added in this PR are meant to ensure that the PNA transfer don't break in future XCM releases (e.g. XCM V6). The reason is that the tokenID - the key of ForeignToNativeId storage to identify the PNA - was generated from XCM V4 and has already been finalized and stored on Ethereum side.
So we can't allow it to change. The test will fail if there is any compatalibity issue in a future release, so I prefer to keep it as a safeguard.
Meanwhile, I've simplified the migration and made some cleaned things up as you suggested in 1ffb3a0.
|
The logs show the RemoveStorage migration running as expected. |
| } | ||
|
|
||
| #[test] | ||
| fn check_token_id_on_chain_derived_from_xcm_v4_is_same_as_value_derived_from_xcm_v5() { |
There was a problem hiding this comment.
@yrong can you add here please some description and/or what to do when this test starts to fail?
| fn check_token_id_on_chain_derived_from_xcm_v4_is_same_as_value_derived_from_xcm_v5() { | |
| fn check_compatibility_for_token_id_stored_on_ethereum() { |
| #[test] | ||
| fn check_location_encode_in_xcm_v4_can_be_decoded_by_xcm_v5() { | ||
| use xcm::v4::prelude::{ | ||
| GeneralIndex as GeneralIndexV4, GeneralKey as GeneralKeyV4, | ||
| GlobalConsensus as GlobalConsensusV4, Kusama as KusamaV4, Location as LocationV4, | ||
| PalletInstance as PalletInstanceV4, Parachain as ParachainV4, Polkadot as PolkadotV4, | ||
| }; | ||
| pub struct LocationEncodeDecodeTestCase { | ||
| pub v4: LocationV4, | ||
| pub latest: Location, | ||
| } | ||
| let test_cases = vec![ | ||
| // DOT | ||
| LocationEncodeDecodeTestCase { | ||
| v4: LocationV4::new(1, GlobalConsensusV4(PolkadotV4)), | ||
| latest: Location::new(1, GlobalConsensus(Polkadot)), | ||
| }, | ||
| // KSM | ||
| LocationEncodeDecodeTestCase { | ||
| v4: LocationV4::new(1, GlobalConsensusV4(KusamaV4)), | ||
| latest: Location::new(1, GlobalConsensus(Kusama)), | ||
| }, | ||
| // PINK | ||
| LocationEncodeDecodeTestCase { | ||
| v4: LocationV4::new( | ||
| 1, | ||
| [ | ||
| GlobalConsensusV4(PolkadotV4), | ||
| ParachainV4(1000), | ||
| PalletInstanceV4(50), | ||
| GeneralIndexV4(23), | ||
| ], | ||
| ), | ||
| latest: Location::new( | ||
| 1, | ||
| [GlobalConsensus(Polkadot), Parachain(1000), PalletInstance(50), GeneralIndex(23)], | ||
| ), | ||
| }, | ||
| // TEER | ||
| LocationEncodeDecodeTestCase { | ||
| v4: LocationV4::new(1, [GlobalConsensusV4(PolkadotV4), ParachainV4(2039)]), | ||
| latest: Location::new(1, [GlobalConsensus(Polkadot), Parachain(2039)]), | ||
| }, | ||
| // Hydration | ||
| LocationEncodeDecodeTestCase { | ||
| v4: LocationV4::new( | ||
| 1, | ||
| [GlobalConsensusV4(PolkadotV4), ParachainV4(2034), GeneralIndexV4(0)], | ||
| ), | ||
| latest: Location::new(1, [GlobalConsensus(Polkadot), Parachain(2034), GeneralIndex(0)]), | ||
| }, | ||
| // Voucher DOT | ||
| LocationEncodeDecodeTestCase { | ||
| v4: LocationV4::new( | ||
| 1, | ||
| [ | ||
| GlobalConsensusV4(PolkadotV4), | ||
| ParachainV4(2030), | ||
| GeneralKeyV4 { | ||
| length: 2, | ||
| data: hex!( | ||
| "0900000000000000000000000000000000000000000000000000000000000000" | ||
| ), | ||
| }, | ||
| ], | ||
| ), | ||
| latest: Location::new( | ||
| 1, | ||
| [ | ||
| GlobalConsensus(Polkadot), | ||
| Parachain(2030), | ||
| GeneralKey { | ||
| length: 2, | ||
| data: hex!( | ||
| "0900000000000000000000000000000000000000000000000000000000000000" | ||
| ), | ||
| }, | ||
| ], | ||
| ), | ||
| }, | ||
| ]; | ||
| for tc in test_cases.iter() { | ||
| let location: Location = Decode::decode(&mut tc.v4.encode().as_slice()) | ||
| .expect("Stored data should decode to V5 format correctly"); | ||
| assert_eq!(location, tc.latest); | ||
| } | ||
| } |
There was a problem hiding this comment.
@yrong I think we can remove this test
| #[test] | |
| fn check_location_encode_in_xcm_v4_can_be_decoded_by_xcm_v5() { | |
| use xcm::v4::prelude::{ | |
| GeneralIndex as GeneralIndexV4, GeneralKey as GeneralKeyV4, | |
| GlobalConsensus as GlobalConsensusV4, Kusama as KusamaV4, Location as LocationV4, | |
| PalletInstance as PalletInstanceV4, Parachain as ParachainV4, Polkadot as PolkadotV4, | |
| }; | |
| pub struct LocationEncodeDecodeTestCase { | |
| pub v4: LocationV4, | |
| pub latest: Location, | |
| } | |
| let test_cases = vec![ | |
| // DOT | |
| LocationEncodeDecodeTestCase { | |
| v4: LocationV4::new(1, GlobalConsensusV4(PolkadotV4)), | |
| latest: Location::new(1, GlobalConsensus(Polkadot)), | |
| }, | |
| // KSM | |
| LocationEncodeDecodeTestCase { | |
| v4: LocationV4::new(1, GlobalConsensusV4(KusamaV4)), | |
| latest: Location::new(1, GlobalConsensus(Kusama)), | |
| }, | |
| // PINK | |
| LocationEncodeDecodeTestCase { | |
| v4: LocationV4::new( | |
| 1, | |
| [ | |
| GlobalConsensusV4(PolkadotV4), | |
| ParachainV4(1000), | |
| PalletInstanceV4(50), | |
| GeneralIndexV4(23), | |
| ], | |
| ), | |
| latest: Location::new( | |
| 1, | |
| [GlobalConsensus(Polkadot), Parachain(1000), PalletInstance(50), GeneralIndex(23)], | |
| ), | |
| }, | |
| // TEER | |
| LocationEncodeDecodeTestCase { | |
| v4: LocationV4::new(1, [GlobalConsensusV4(PolkadotV4), ParachainV4(2039)]), | |
| latest: Location::new(1, [GlobalConsensus(Polkadot), Parachain(2039)]), | |
| }, | |
| // Hydration | |
| LocationEncodeDecodeTestCase { | |
| v4: LocationV4::new( | |
| 1, | |
| [GlobalConsensusV4(PolkadotV4), ParachainV4(2034), GeneralIndexV4(0)], | |
| ), | |
| latest: Location::new(1, [GlobalConsensus(Polkadot), Parachain(2034), GeneralIndex(0)]), | |
| }, | |
| // Voucher DOT | |
| LocationEncodeDecodeTestCase { | |
| v4: LocationV4::new( | |
| 1, | |
| [ | |
| GlobalConsensusV4(PolkadotV4), | |
| ParachainV4(2030), | |
| GeneralKeyV4 { | |
| length: 2, | |
| data: hex!( | |
| "0900000000000000000000000000000000000000000000000000000000000000" | |
| ), | |
| }, | |
| ], | |
| ), | |
| latest: Location::new( | |
| 1, | |
| [ | |
| GlobalConsensus(Polkadot), | |
| Parachain(2030), | |
| GeneralKey { | |
| length: 2, | |
| data: hex!( | |
| "0900000000000000000000000000000000000000000000000000000000000000" | |
| ), | |
| }, | |
| ], | |
| ), | |
| }, | |
| ]; | |
| for tc in test_cases.iter() { | |
| let location: Location = Decode::decode(&mut tc.v4.encode().as_slice()) | |
| .expect("Stored data should decode to V5 format correctly"); | |
| assert_eq!(location, tc.latest); | |
| } | |
| } |
There was a problem hiding this comment.
IMHO, this test is also useful, so I'd prefer to keep it. I've added comments in
055f9ee for more details.
There was a problem hiding this comment.
IMHO, this test is also useful, so I'd prefer to keep it.
Well, this test does not test anything special for Snowbridge or specific onchain stuff like the test before, it just tests "some" compatibility for XCMv4 vs XCMv5 - which should better go directly to the polkadot-sdk/xcm/src/v5 module and not here.
We basically agreed here that XCMv4 vs XCMv5 are compatible, so really this test is not needed here.
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
…idge.rs Co-authored-by: Branislav Kontur <bkontur@gmail.com>
|
/merge |
|
Enabled Available commands
For more information see the documentation |
|
@pandres95 can you please review this too? 🙏 |
fellowship-merge-bot
bot
merged commit d41d01f
into
polkadot-fellows:main
7 participants
No description provided.