chore: AH as reserve #423
chore: AH as reserve #423
Conversation
| fn contains(asset: &Asset, origin: &Location) -> bool { | ||
| let loc = T::get(); | ||
| let loc = AssetHub::get(); | ||
| &loc == origin && | ||
| matches!(asset, Asset { id: AssetId(asset_loc), fun: Fungible(_a) } | ||
| if *asset_loc == Location::from(Parent)) |
There was a problem hiding this comment.
Didn't see value in having this generic as it always looks for the asset location of the relay and we won't use it for a different chain.
There was a problem hiding this comment.
Ok given that having multiple reserves for the same asset is not a good practice. And we most likely won't want to have many, for instance, system chains acting as the reserve of DOT.
But I liked better NativeAssetFrom<AssetHub> than RelayAssetFromAssetHub :)
| @@ -270,7 +270,9 @@ fn fund_pop_from_system_para( | |||
| } | |||
|
|
|||
| /// Reserve Transfers of native asset from Relay to Parachain should work | |||
| #[cfg(not(feature = "testnet"))] | |||
There was a problem hiding this comment.
Can be removed in the release where we set the reserve of dot to asset hub on testnet as well.
| #[test] | ||
| #[should_panic] | ||
| fn reserve_transfer_native_asset_from_para_to_relay() { |
There was a problem hiding this comment.
This test can be removed in the next release as well as it should not be possible to do a reserve transfer to the relay with only AH as reserve.
Daanvdplas
force-pushed
the
daan/chore-ah_as_reserve
branch
from
44f77a2 to
65393c8
Compare
There was a problem hiding this comment.
Left a few nits, but the changes are good and tests run green!
Some easy unit testing for NativeAssetExceptRelay and RelayAssetFromAssetHub showing than the results are the expected given a list of locations would have been nice!
Or, a very small integration test checking the reserves if the above is not really something we want given we have 3x runtimes.
| @@ -0,0 +1,8 @@ | |||
| In order to run the integration tests you have to specify what Pop and Relay runtime to use. The latter also determines the Asset Hub runtime. | |||
There was a problem hiding this comment.
| In order to run the integration tests you have to specify what Pop and Relay runtime to use. The latter also determines the Asset Hub runtime. | |
| In order to run the integration tests you have to specify what Relay and Pop runtime to use. The latter also determines the Asset Hub runtime. |
There was a problem hiding this comment.
Just so that the doc reads in the same order as the code.
| } | ||
|
|
||
| /// Combinations of (Asset, Location) pairs which we trust as reserves. | ||
| pub type TrustedReserves = (NativeAssetExceptRelay, RelayAssetFromAssetHub); |
There was a problem hiding this comment.
While I don't think this presents a problem, I'm have some hesitations around including more reserves on mainnet than relay native's for now.
Channels are still required for any transfer to occur. But maybe for that same reason we expand the reserves when that moment comes ?
I'd like to hear your considerations as I can think of opinions for both, keeping it and removing it. :)
| fn contains(asset: &Asset, origin: &Location) -> bool { | ||
| let loc = T::get(); | ||
| let loc = AssetHub::get(); | ||
| &loc == origin && | ||
| matches!(asset, Asset { id: AssetId(asset_loc), fun: Fungible(_a) } | ||
| if *asset_loc == Location::from(Parent)) |
There was a problem hiding this comment.
Ok given that having multiple reserves for the same asset is not a good practice. And we most likely won't want to have many, for instance, system chains acting as the reserve of DOT.
But I liked better NativeAssetFrom<AssetHub> than RelayAssetFromAssetHub :)
Changes the xcm config of devnet and mainnet to only accept reserve transfers of the dot token from AH.
A small refactor of the structure of the features in the integration tests have been applied to simplify things, supported with a small readme.
The CI has received some optimisations to exclude the integration tests. Only the testnet integration tests are added to the CI for now. This can also be changed to all pop's runtimes with westend / paseo runtime. Adding them all could also be done but it feels like an overkill to run for each PR. Open to suggestions.