Added foreign locations to local accounts converter to all the parachains #5765
Conversation
There was a problem hiding this comment.
Request changes
Do not remove existing converters to maintain backwards compatibility (see following comments)
Testing
For each chain/runtime, please add unit tests (faster/easier than emulated tests) that cover the newly supported conversions.
E.g. you can add new test for asset-hub-westend here, then run it with cargo test -r -p asset-hub-westend-runtime. Do the same for other runtimes too.
In terms of how to test, very easy way is using LocationToAccountHelper::convert_location()` and pass it locations that use to fail conversion, but with your change now succeed.
You can see an example of how to call LocationToAccountHelper here.
cumulus/parachains/runtimes/people/people-rococo/src/xcm_config.rs
Outdated
Show resolved
Hide resolved
cumulus/parachains/runtimes/people/people-westend/src/xcm_config.rs
Outdated
Show resolved
Hide resolved
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs
Show resolved
Hide resolved
cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs
Outdated
Show resolved
Hide resolved
|
bot fmt |
|
Review required! Latest push from author must always be reviewed |
…xcm_config.rs Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Added the tests. Additionally removed my change and made sure that the tests fail. |
x3c41a
added
A0-needs_burnin
| let alice_on_bh = Location::new( | ||
| 1, | ||
| [ | ||
| Parachain(1111), | ||
| Junction::AccountId32 { network: None, id: AccountId::from(ALICE).into() }, | ||
| ], | ||
| ); |
There was a problem hiding this comment.
alice_on_bh implies BH para id will be used, so either the right para id (not 1111) or change name to sibling_alice or something similar.
| assert_ok!(LocationToAccountHelper::<AccountId, LocationToAccountId>::convert_location( | ||
| alice_on_bh.into() | ||
| )); |
There was a problem hiding this comment.
This location_conversion_works test currently checks one single location conversion: that of an AccountId32 on a sibling parachain.
The test should verify multiple location types conversions. At the very least to completely cover regression testing for the changes in this PR it should cover all branches of DescribeFamily<DescribeAllTerminal>.
That means: 3 valid branches in DescribeFamiliy x 6 valid types of location terminals defined in DescribeAllTerminal = 18 valid locations types to verify.
Also, instead of assert_ok!(), you should check assert_eq!() against the hardcoded converted account, so that we know in the future this conversion doesn't accidentally change (breaking existing derived accounts).
Same comment applies for all the other runtimes tests.
There was a problem hiding this comment.
Applied requested changes to a single runtime.
PTAL.
Added
HashedDescription<AccountId, DescribeFamily<DescribeAllTerminal>>foreign locations to local accounts converter to all the parachains.