From ab431fed3227973417317d4e645dc8f89e39d1fd Mon Sep 17 00:00:00 2001 From: Gav Date: Wed, 18 Jan 2023 13:11:04 -0300 Subject: [PATCH 1/7] Introduce ExpectTransactStatus instruction --- Cargo.lock | 1 + xcm/Cargo.toml | 1 + xcm/src/v3/mod.rs | 37 ++++++++++++++++++++++++++++--------- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6f3d27f88841..758e544e1338 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13555,6 +13555,7 @@ name = "xcm" version = "0.9.33" dependencies = [ "derivative", + "hex-literal", "impl-trait-for-tuples", "log", "parity-scale-codec", diff --git a/xcm/Cargo.toml b/xcm/Cargo.toml index d66e25748bce..bb3c1022988a 100644 --- a/xcm/Cargo.toml +++ b/xcm/Cargo.toml @@ -17,6 +17,7 @@ serde = { version = "1.0.136", optional = true, features = ["derive"] } xcm-procedural = { path = "procedural" } [dev-dependencies] +hex-literal = "0.3.4" sp-io = { git = "https://github.com/paritytech/substrate", branch = "master" } [features] diff --git a/xcm/src/v3/mod.rs b/xcm/src/v3/mod.rs index 7f55f27fad28..8c687301fc7c 100644 --- a/xcm/src/v3/mod.rs +++ b/xcm/src/v3/mod.rs @@ -480,7 +480,7 @@ pub enum Instruction { /// Withdraw asset(s) (`assets`) from the ownership of `origin` and place equivalent assets /// under the ownership of `dest` within this consensus system (i.e. its sovereign account). /// - /// Send an onward XCM message to `dest` of `ReserveAssetDeposited` with the wantn + /// Send an onward XCM message to `dest` of `ReserveAssetDeposited` with the given /// `xcm`. /// /// - `assets`: The asset(s) to be withdrawn. @@ -588,7 +588,7 @@ pub enum Instruction { /// Errors: DescendOrigin(InteriorMultiLocation), - /// Immediately report the contents of the Error Register to the wantn destination via XCM. + /// Immediately report the contents of the Error Register to the given destination via XCM. /// /// A `QueryResponse` message of type `ExecutionOutcome` is sent to the described destination. /// @@ -614,7 +614,7 @@ pub enum Instruction { /// the ownership of `dest` within this consensus system (i.e. deposit them into its sovereign /// account). /// - /// Send an onward XCM message to `dest` of `ReserveAssetDeposited` with the wantn `effects`. + /// Send an onward XCM message to `dest` of `ReserveAssetDeposited` with the given `effects`. /// /// - `assets`: The asset(s) to remove from holding. /// - `dest`: The location whose sovereign account will own the assets and thus the effective @@ -652,7 +652,7 @@ pub enum Instruction { /// - `reserve`: A valid location that acts as a reserve for all asset(s) in `assets`. The /// sovereign account of this consensus system *on the reserve location* will have appropriate /// assets withdrawn and `effects` will be executed on them. There will typically be only one - /// valid location on any wantn asset/chain combination. + /// valid location on any given asset/chain combination. /// - `xcm`: The instructions to execute on the assets once withdrawn *on the reserve /// location*. /// @@ -677,7 +677,7 @@ pub enum Instruction { /// Errors: InitiateTeleport { assets: MultiAssetFilter, dest: MultiLocation, xcm: Xcm<()> }, - /// Report to a wantn destination the contents of the Holding Register. + /// Report to a given destination the contents of the Holding Register. /// /// A `QueryResponse` message of type `Assets` is sent to the described destination. /// @@ -795,7 +795,7 @@ pub enum Instruction { /// Errors: *Fallible* UnsubscribeVersion, - /// Reduce Holding by up to the wantn assets. + /// Reduce Holding by up to the given assets. /// /// Holding is reduced by as much as possible up to the assets in the parameter. It is not an /// error if the Holding does not contain the assets (to make this an error, use `ExpectAsset` @@ -806,7 +806,7 @@ pub enum Instruction { /// Errors: *Infallible* BurnAsset(MultiAssets), - /// Throw an error if Holding does not contain at least the wantn assets. + /// Throw an error if Holding does not contain at least the given assets. /// /// Kind: *Instruction* /// @@ -814,7 +814,7 @@ pub enum Instruction { /// - `ExpectationFalse`: If Holding Register does not contain the assets in the parameter. ExpectAsset(MultiAssets), - /// Ensure that the Origin Register equals some wantn value and throw an error if not. + /// Ensure that the Origin Register equals some given value and throw an error if not. /// /// Kind: *Instruction* /// @@ -822,7 +822,7 @@ pub enum Instruction { /// - `ExpectationFalse`: If Origin Register is not equal to the parameter. ExpectOrigin(Option), - /// Ensure that the Error Register equals some wantn value and throw an error if not. + /// Ensure that the Error Register equals some given value and throw an error if not. /// /// Kind: *Instruction* /// @@ -830,6 +830,15 @@ pub enum Instruction { /// - `ExpectationFalse`: If the value of the Error Register is not equal to the parameter. ExpectError(Option<(u32, Error)>), + /// Ensure that the Transact StatusError Register equals some given value and throw an error if + /// not. + /// + /// Kind: *Instruction* + /// + /// Errors: + /// - `ExpectationFalse`: If the value of the Error Register is not equal to the parameter. + ExpectTransactStatus(MaybeErrorCode), + /// Query the existence of a particular pallet type. /// /// - `module_name`: The module name of the pallet to query. @@ -1325,6 +1334,16 @@ mod tests { WildMultiAsset as OldWildMultiAsset, }; + #[test] + fn message_from_v3_works() { + use crate::VersionedXcm; + use hex_literal::hex; + let message = hex!["0204060202286bee3d010a020052bc71c1eca5353749542dfdf0af97bf764f9c2f44e860cd485f1cd86400f649009e6eb74b0a6b39de36fb58d1fab20bc2b3fea96023ce5a47941c20480d99f92e1b000080f64ae1c7022d15"]; + let xcm = VersionedXcm::<()>::decode(&mut &message[..]).unwrap(); + assert_eq!(xcm, VersionedXcm::<()>::V2(OldXcm::<()>(vec![]))); + panic!(); + } + #[test] fn basic_roundtrip_works() { let xcm = Xcm::<()>(vec![TransferAsset { From fe962f0e8279c60badfb427998fa754fa96f5832 Mon Sep 17 00:00:00 2001 From: Gav Date: Wed, 18 Jan 2023 13:12:11 -0300 Subject: [PATCH 2/7] Remove other changes --- Cargo.lock | 1 - xcm/Cargo.toml | 1 - xcm/src/v3/mod.rs | 10 ---------- 3 files changed, 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 758e544e1338..6f3d27f88841 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13555,7 +13555,6 @@ name = "xcm" version = "0.9.33" dependencies = [ "derivative", - "hex-literal", "impl-trait-for-tuples", "log", "parity-scale-codec", diff --git a/xcm/Cargo.toml b/xcm/Cargo.toml index bb3c1022988a..d66e25748bce 100644 --- a/xcm/Cargo.toml +++ b/xcm/Cargo.toml @@ -17,7 +17,6 @@ serde = { version = "1.0.136", optional = true, features = ["derive"] } xcm-procedural = { path = "procedural" } [dev-dependencies] -hex-literal = "0.3.4" sp-io = { git = "https://github.com/paritytech/substrate", branch = "master" } [features] diff --git a/xcm/src/v3/mod.rs b/xcm/src/v3/mod.rs index 8c687301fc7c..1b0e044549f9 100644 --- a/xcm/src/v3/mod.rs +++ b/xcm/src/v3/mod.rs @@ -1334,16 +1334,6 @@ mod tests { WildMultiAsset as OldWildMultiAsset, }; - #[test] - fn message_from_v3_works() { - use crate::VersionedXcm; - use hex_literal::hex; - let message = hex!["0204060202286bee3d010a020052bc71c1eca5353749542dfdf0af97bf764f9c2f44e860cd485f1cd86400f649009e6eb74b0a6b39de36fb58d1fab20bc2b3fea96023ce5a47941c20480d99f92e1b000080f64ae1c7022d15"]; - let xcm = VersionedXcm::<()>::decode(&mut &message[..]).unwrap(); - assert_eq!(xcm, VersionedXcm::<()>::V2(OldXcm::<()>(vec![]))); - panic!(); - } - #[test] fn basic_roundtrip_works() { let xcm = Xcm::<()>(vec![TransferAsset { From f2d6c707b4bc312dbb8e339569b037748c122381 Mon Sep 17 00:00:00 2001 From: Gav Date: Wed, 18 Jan 2023 13:20:26 -0300 Subject: [PATCH 3/7] Implement --- runtime/kusama/src/weights/xcm/mod.rs | 3 +++ xcm/src/v3/mod.rs | 2 ++ 2 files changed, 5 insertions(+) diff --git a/runtime/kusama/src/weights/xcm/mod.rs b/runtime/kusama/src/weights/xcm/mod.rs index 74c67c175785..ac45a98bcfe6 100644 --- a/runtime/kusama/src/weights/xcm/mod.rs +++ b/runtime/kusama/src/weights/xcm/mod.rs @@ -198,6 +198,9 @@ impl XcmWeightInfo for KusamaXcmWeight { fn expect_error(_error: &Option<(u32, XcmError)>) -> Weight { XcmGeneric::::expect_error() } + fn expect_transact_status(_transact_status: &MaybeErrorCode) -> Weight { + XcmGeneric::::expect_transact_status() + } fn query_pallet(_module_name: &Vec, _response_info: &QueryResponseInfo) -> Weight { XcmGeneric::::query_pallet() } diff --git a/xcm/src/v3/mod.rs b/xcm/src/v3/mod.rs index 1b0e044549f9..d1cec0867174 100644 --- a/xcm/src/v3/mod.rs +++ b/xcm/src/v3/mod.rs @@ -1097,6 +1097,7 @@ impl Instruction { ExpectAsset(assets) => ExpectAsset(assets), ExpectOrigin(origin) => ExpectOrigin(origin), ExpectError(error) => ExpectError(error), + ExpectTransactStatus(transact_status) => ExpectTransactStatus(transact_status), QueryPallet { module_name, response_info } => QueryPallet { module_name, response_info }, ExpectPallet { index, name, module_name, crate_major, min_crate_minor } => @@ -1165,6 +1166,7 @@ impl> GetWeight for Instruction { ExpectAsset(assets) => W::expect_asset(assets), ExpectOrigin(origin) => W::expect_origin(origin), ExpectError(error) => W::expect_error(error), + ExpectTransactStatus(transact_status) => W::expect_transact_status(transact_status), QueryPallet { module_name, response_info } => W::query_pallet(module_name, response_info), ExpectPallet { index, name, module_name, crate_major, min_crate_minor } => From 028f4d01fb837d9c04a81851f0e6e340fecb2afa Mon Sep 17 00:00:00 2001 From: Gav Date: Wed, 18 Jan 2023 13:30:22 -0300 Subject: [PATCH 4/7] Implement rest --- .../xcm/pallet_xcm_benchmarks_generic.rs | 3 + runtime/rococo/src/weights/xcm/mod.rs | 3 + .../xcm/pallet_xcm_benchmarks_generic.rs | 3 + runtime/westend/src/weights/xcm/mod.rs | 3 + .../xcm/pallet_xcm_benchmarks_generic.rs | 3 + xcm/xcm-builder/src/tests/transacting.rs | 62 +++++++++++++++++++ xcm/xcm-executor/src/lib.rs | 4 ++ 7 files changed, 81 insertions(+) diff --git a/runtime/kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs b/runtime/kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs index 69421bdb414b..b82501bcde49 100644 --- a/runtime/kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs +++ b/runtime/kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs @@ -142,6 +142,9 @@ impl WeightInfo { pub(crate) fn expect_error() -> Weight { Weight::from_ref_time(3_645_000 as u64) } + pub(crate) fn expect_transact_status() -> Weight { + Weight::from_ref_time(3_645_000 as u64) + } // Storage: XcmPallet SupportedVersion (r:1 w:0) // Storage: XcmPallet VersionDiscoveryQueue (r:1 w:1) // Storage: XcmPallet SafeXcmVersion (r:1 w:0) diff --git a/runtime/rococo/src/weights/xcm/mod.rs b/runtime/rococo/src/weights/xcm/mod.rs index 49252e3662fb..bd3c528fa5da 100644 --- a/runtime/rococo/src/weights/xcm/mod.rs +++ b/runtime/rococo/src/weights/xcm/mod.rs @@ -198,6 +198,9 @@ impl XcmWeightInfo for RococoXcmWeight { fn expect_error(_error: &Option<(u32, XcmError)>) -> Weight { XcmGeneric::::expect_error() } + fn expect_transact_status(_transact_status: &MaybeErrorCode) -> Weight { + XcmGeneric::::expect_transact_status() + } fn query_pallet(_module_name: &Vec, _response_info: &QueryResponseInfo) -> Weight { XcmGeneric::::query_pallet() } diff --git a/runtime/rococo/src/weights/xcm/pallet_xcm_benchmarks_generic.rs b/runtime/rococo/src/weights/xcm/pallet_xcm_benchmarks_generic.rs index f2d786e85a46..baa2517946ae 100644 --- a/runtime/rococo/src/weights/xcm/pallet_xcm_benchmarks_generic.rs +++ b/runtime/rococo/src/weights/xcm/pallet_xcm_benchmarks_generic.rs @@ -145,6 +145,9 @@ impl WeightInfo { pub(crate) fn expect_error() -> Weight { Weight::from_ref_time(3_633_000 as u64) } + pub(crate) fn expect_transact_status() -> Weight { + Weight::from_ref_time(3_633_000 as u64) + } // Storage: XcmPallet SupportedVersion (r:1 w:0) // Storage: XcmPallet VersionDiscoveryQueue (r:1 w:1) // Storage: XcmPallet SafeXcmVersion (r:1 w:0) diff --git a/runtime/westend/src/weights/xcm/mod.rs b/runtime/westend/src/weights/xcm/mod.rs index 26df93378959..834f908938b0 100644 --- a/runtime/westend/src/weights/xcm/mod.rs +++ b/runtime/westend/src/weights/xcm/mod.rs @@ -201,6 +201,9 @@ impl XcmWeightInfo for WestendXcmWeight { fn expect_error(_error: &Option<(u32, XcmError)>) -> Weight { XcmGeneric::::expect_error() } + fn expect_transact_status(_transact_status: &MaybeErrorCode) -> Weight { + XcmGeneric::::expect_transact_status() + } fn query_pallet(_module_name: &Vec, _response_info: &QueryResponseInfo) -> Weight { XcmGeneric::::query_pallet() } diff --git a/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs b/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs index aed7316cdd28..d9efb10d702d 100644 --- a/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs +++ b/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs @@ -143,6 +143,9 @@ impl WeightInfo { pub(crate) fn expect_error() -> Weight { Weight::from_ref_time(5_775_000 as u64) } + pub(crate) fn expect_transact_status() -> Weight { + Weight::from_ref_time(5_775_000 as u64) + } // Storage: XcmPallet SupportedVersion (r:1 w:0) // Storage: XcmPallet VersionDiscoveryQueue (r:1 w:1) // Storage: XcmPallet SafeXcmVersion (r:1 w:0) diff --git a/xcm/xcm-builder/src/tests/transacting.rs b/xcm/xcm-builder/src/tests/transacting.rs index bcff05f35d66..bab6772ea0e7 100644 --- a/xcm/xcm-builder/src/tests/transacting.rs +++ b/xcm/xcm-builder/src/tests/transacting.rs @@ -156,6 +156,68 @@ fn report_failed_transact_status_should_work() { assert_eq!(sent_xcm(), vec![(Parent.into(), expected_msg, expected_hash)]); } +#[test] +fn expect_successful_transact_status_should_work() { + AllowUnpaidFrom::set(vec![Parent.into()]); + + let message = Xcm::(vec![ + Transact { + origin_kind: OriginKind::Native, + require_weight_at_most: Weight::from_parts(50, 50), + call: TestCall::Any(Weight::from_parts(50, 50), None).encode().into(), + }, + ExpectTransactStatus(MaybeErrorCode::Success), + ]); + let hash = fake_message_hash(&message); + let weight_limit = Weight::from_parts(70, 70); + let r = XcmExecutor::::execute_xcm(Parent, message, hash, weight_limit); + assert_eq!(r, Outcome::Complete(Weight::from_parts(70, 70))); + + let message = Xcm::(vec![ + Transact { + origin_kind: OriginKind::Native, + require_weight_at_most: Weight::from_parts(50, 50), + call: TestCall::OnlyRoot(Weight::from_parts(50, 50), None).encode().into(), + }, + ExpectTransactStatus(MaybeErrorCode::Success), + ]); + let hash = fake_message_hash(&message); + let weight_limit = Weight::from_parts(70, 70); + let r = XcmExecutor::::execute_xcm(Parent, message, hash, weight_limit); + assert_eq!(r, Outcome::Incomplete(Weight::from_parts(70, 70), XcmError::ExpectationFalse)); +} + +#[test] +fn expect_failed_transact_status_should_work() { + AllowUnpaidFrom::set(vec![Parent.into()]); + + let message = Xcm::(vec![ + Transact { + origin_kind: OriginKind::Native, + require_weight_at_most: Weight::from_parts(50, 50), + call: TestCall::OnlyRoot(Weight::from_parts(50, 50), None).encode().into(), + }, + ExpectTransactStatus(MaybeErrorCode::Error(vec![2])), + ]); + let hash = fake_message_hash(&message); + let weight_limit = Weight::from_parts(70, 70); + let r = XcmExecutor::::execute_xcm(Parent, message, hash, weight_limit); + assert_eq!(r, Outcome::Complete(Weight::from_parts(70, 70))); + + let message = Xcm::(vec![ + Transact { + origin_kind: OriginKind::Native, + require_weight_at_most: Weight::from_parts(50, 50), + call: TestCall::Any(Weight::from_parts(50, 50), None).encode().into(), + }, + ExpectTransactStatus(MaybeErrorCode::Error(vec![2])), + ]); + let hash = fake_message_hash(&message); + let weight_limit = Weight::from_parts(70, 70); + let r = XcmExecutor::::execute_xcm(Parent, message, hash, weight_limit); + assert_eq!(r, Outcome::Incomplete(Weight::from_parts(70, 70), XcmError::ExpectationFalse)); +} + #[test] fn clear_transact_status_should_work() { AllowUnpaidFrom::set(vec![Parent.into()]); diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 175f9e54fae7..913d1bfcfb50 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -753,6 +753,10 @@ impl XcmExecutor { ensure!(self.error == error, XcmError::ExpectationFalse); Ok(()) }, + ExpectTransactStatus(transact_status) => { + ensure!(self.transact_status == transact_status, XcmError::ExpectationFalse); + Ok(()) + }, QueryPallet { module_name, response_info } => { let pallets = Config::PalletInstancesInfo::infos() .into_iter() From 94a18b0dcaecdd8110a21afe820ba02ab1f7952c Mon Sep 17 00:00:00 2001 From: Gav Date: Wed, 18 Jan 2023 13:53:16 -0300 Subject: [PATCH 5/7] Benchmark --- .../src/generic/benchmarking.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs b/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs index 797238babe20..2bd73a85088b 100644 --- a/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs +++ b/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs @@ -360,6 +360,22 @@ benchmarks! { }))); } + expect_transact_status { + let mut executor = new_executor::(Default::default()); + // 1024 is an overestimate but should be good enough until we have `max_encoded_len`. + // Eventually it should be replaced by `DispatchError::max_encoded_len()`. + let worst_error = || MaybeErrorCode::Error(vec![0; 1024]); + executor.set_transact_status(worst_error()); + + let instruction = Instruction::ExpectTransactStatus(worst_error()); + let xcm = Xcm(vec![instruction]); + let mut _result = Ok(()); + }: { + _result = executor.bench_process(xcm); + } verify { + assert!(matches!(_result, Ok(..))); + } + query_pallet { let query_id = Default::default(); let destination = T::valid_destination().map_err(|_| BenchmarkError::Skip)?; From 50dde477d3b3b413f20cad2c1edbde2c2fbee36c Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 18 Jan 2023 13:53:43 -0300 Subject: [PATCH 6/7] Update xcm/src/v3/mod.rs Co-authored-by: Keith Yeung --- xcm/src/v3/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xcm/src/v3/mod.rs b/xcm/src/v3/mod.rs index d1cec0867174..009d52bdeb5c 100644 --- a/xcm/src/v3/mod.rs +++ b/xcm/src/v3/mod.rs @@ -830,7 +830,7 @@ pub enum Instruction { /// - `ExpectationFalse`: If the value of the Error Register is not equal to the parameter. ExpectError(Option<(u32, Error)>), - /// Ensure that the Transact StatusError Register equals some given value and throw an error if + /// Ensure that the Transact Status Register equals some given value and throw an error if /// not. /// /// Kind: *Instruction* From 7e997bce6d1da473cfb1b1c6c4d79762bb98b79a Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 18 Jan 2023 13:53:50 -0300 Subject: [PATCH 7/7] Update xcm/src/v3/mod.rs Co-authored-by: Keith Yeung --- xcm/src/v3/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xcm/src/v3/mod.rs b/xcm/src/v3/mod.rs index 009d52bdeb5c..fb3e3ba95c26 100644 --- a/xcm/src/v3/mod.rs +++ b/xcm/src/v3/mod.rs @@ -836,7 +836,7 @@ pub enum Instruction { /// Kind: *Instruction* /// /// Errors: - /// - `ExpectationFalse`: If the value of the Error Register is not equal to the parameter. + /// - `ExpectationFalse`: If the value of the Transact Status Register is not equal to the parameter. ExpectTransactStatus(MaybeErrorCode), /// Query the existence of a particular pallet type.