XCM v0 #1657
XCM v0 #1657
Conversation
gavofyork
added
A3-in_progress
parachain/src/xcm.rs
Outdated
| } | ||
|
|
||
| #[derive(Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug)] | ||
| pub enum Junction { |
parachain/src/xcm.rs
Outdated
| } | ||
|
|
||
| #[derive(Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug)] | ||
| pub enum MultiAsset { |
There was a problem hiding this comment.
potentially need to update spec here too
xlc
left a comment
xlc
left a comment
There was a problem hiding this comment.
The current XCM implementation will failed to decode messages contains future version.
Do we want better future compatibility? i.e. Better handle VersionedXXX, decode it into some Unknown(OpaqueMessage) type. That also requires the messages to be length prefixed.
parachain/src/xcm.rs
Outdated
| AccountIndex64 { network: MultiNetwork, #[codec(compact)] index: u64 }, | ||
| AccountKey20 { network: MultiNetwork, key: [u8; 20] }, | ||
| /// An instanced Pallet on a Frame-based chain. | ||
| PalletInstance { id: u8 }, |
There was a problem hiding this comment.
The is the pallet index right? The index in construct_runtime? Not the Instance number for Instantiable pallets?
There was a problem hiding this comment.
Yes, this is the index from construct_runtime. Will see if I can rename it.
parachain/src/xcm.rs
Outdated
| pub enum AssetInstance { | ||
| Undefined, | ||
| Index8(u8), | ||
| Index16 { #[codec(compact)] id: u16 }, |
There was a problem hiding this comment.
Not sure if we want to use compact encoding for u16, it can only save up to one byte and can use two more bytes in some cases.
parachain/src/xcm.rs
Outdated
| } | ||
|
|
||
| #[derive(Clone, Eq, PartialEq, Encode, Decode)] | ||
| pub enum Ai { |
There was a problem hiding this comment.
Do we want to use the full name here? i.e. AssetInstruction
parachain/src/xcm.rs
Outdated
| AbstractNonFungible { class: Vec<u8>, instance: AssetInstance }, | ||
| ConcreteFungible { id: MultiLocation, #[codec(compact)] amount: u128 }, | ||
| ConcreteNonFungible { class: MultiLocation, instance: AssetInstance }, | ||
| Each(Vec<MultiAsset>), |
There was a problem hiding this comment.
Move this to first one to match with Ai::Each
parachain/src/xcm.rs
Outdated
| Balances { query_id: Vec<u8>, assets: Vec<MultiAsset> }, | ||
| Transact { origin_type: MultiOrigin, call: Vec<u8> }, | ||
| // these won't be staying here for long. only v0 parachains with HRMP. | ||
| ForwardToParachain { id: u32, inner: Box<VersionedXcm> }, |
There was a problem hiding this comment.
Not sure if it is possible to set explicit index here to 250 so remove those in later version won't leave a hole
There was a problem hiding this comment.
Some feedback while integrating XCM v0 to ORML.
Decimal conversion.
We might need a mechanism to handle decimal differences between chains, like DOT in Acala is 18. It might not be a problem for sending XCM, as the appropriate amounts could be set on different part of XCM while building it. (Not 100% sure, it might affect holding in some cases if different amounts in different parts?) But while executing DMP, currently there is no way to identify if it's from a chain with different decimals.
A straightforward solution might be to introduce origin parameter in deposit_asset and withdraw_asset in TransactAsset traits. So that amounts could be adjusted based on different origins.
More error handling
There is no detailed error info if something goes wrong in CurrencyAdapter. Maybe we could add a customizable variant into xcm::traits::Error, to contain details, instead of returning Error::Undefined.
Xcm/Effect execution events
It would be helpful to have events indicating what Xcm/Effects got executed.
| fn deposit_event() = default; | ||
|
|
||
| #[weight = 10] | ||
| fn execute(origin, xcm: VersionedXcm) { |
There was a problem hiding this comment.
Could we make execute public so it could be called by other modules?
One use case is in orml-xtokens, which serves as an adapter module, to adapt dispatchable calls like transfer_to_relay_chain to an xcm and execute it.
No description provided.