From 1a1ef75203458eed2d5ed2a1943f9159745a61b0 Mon Sep 17 00:00:00 2001 From: Ricardo Date: Mon, 3 Jun 2024 19:18:45 +0100 Subject: [PATCH 1/4] tests: add benchmarking test of revert_outbound_transfer --- .../argo-bridge/src/benchmarking.rs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/runtime-modules/argo-bridge/src/benchmarking.rs b/runtime-modules/argo-bridge/src/benchmarking.rs index b22b7bcd4a..2f5a750828 100644 --- a/runtime-modules/argo-bridge/src/benchmarking.rs +++ b/runtime-modules/argo-bridge/src/benchmarking.rs @@ -130,6 +130,37 @@ benchmarks! { RawEvent::OutboundTransferRequested(transfer_id, sender, dest_account, transfer_amount, fee).into()); } + revert_outbound_transfer{ + let fee: BalanceOf = 10u32.into(); + let pauser_acount = T::AccountId::create_account_id(1u32); + let operator_account = T::AccountId::create_account_id(1u32); + let remote_chains: Vec = (0..MAX_REMOTE_CHAINS).collect(); + let parameters = BridgeConstraints { + operator_account: Some(operator_account.clone()), + pauser_accounts: Some(vec![pauser_acount.clone()]), + bridging_fee: Some(fee), + thawn_duration: Some(1u32.into()), + remote_chains: Some(BoundedVec::try_from(remote_chains).unwrap()) + }; + + ArgoBridge::::update_bridge_constrains( + RawOrigin::Root.into(), + parameters + ).unwrap(); + activate_bridge::(&pauser_acount, &operator_account); + + let revert_amount: u32 = 1030u32; + set_bridge_mint_allowance::(revert_amount.into(), fee); + + let transfer_id = 1u64; + let rationale = "test".as_bytes().to_vec(); + let revert_account = T::AccountId::create_account_id(2u32); + }: _(RawOrigin::Signed(operator_account), transfer_id, revert_account.clone(), revert_amount.into(), rationale.clone()) + verify { + assert_last_event::( + RawEvent::OutboundTransferReverted(transfer_id, revert_account, revert_amount.into(), rationale).into()); + } + // Worst case scenario: // - max number of remote chains being use @@ -295,6 +326,13 @@ mod tests { }); } + #[test] + fn test_revert_outbound_transfer() { + with_test_externalities(|| { + assert_ok!(ArgoBridge::test_benchmark_revert_outbound_transfer()); + }); + } + #[test] fn test_finalize_inbound_transfer() { with_test_externalities(|| { From 14420b3eaaee43e238b4b508fa7ca70f804c1cda Mon Sep 17 00:00:00 2001 From: Ricardo Date: Tue, 4 Jun 2024 19:25:31 +0100 Subject: [PATCH 2/4] limit size of revert_outbound_transfer rationale --- runtime-modules/argo-bridge/src/benchmarking.rs | 8 +++++--- runtime-modules/argo-bridge/src/events.rs | 11 +++++++++-- runtime-modules/argo-bridge/src/lib.rs | 2 +- runtime-modules/argo-bridge/src/tests/mod.rs | 12 ++++++------ runtime-modules/argo-bridge/src/types.rs | 2 ++ 5 files changed, 23 insertions(+), 12 deletions(-) diff --git a/runtime-modules/argo-bridge/src/benchmarking.rs b/runtime-modules/argo-bridge/src/benchmarking.rs index 2f5a750828..788bc0832f 100644 --- a/runtime-modules/argo-bridge/src/benchmarking.rs +++ b/runtime-modules/argo-bridge/src/benchmarking.rs @@ -130,6 +130,8 @@ benchmarks! { RawEvent::OutboundTransferRequested(transfer_id, sender, dest_account, transfer_amount, fee).into()); } + // Worse case scenario + // - rationale of the maximum size revert_outbound_transfer{ let fee: BalanceOf = 10u32.into(); let pauser_acount = T::AccountId::create_account_id(1u32); @@ -153,12 +155,12 @@ benchmarks! { set_bridge_mint_allowance::(revert_amount.into(), fee); let transfer_id = 1u64; - let rationale = "test".as_bytes().to_vec(); + let rationale = vec![0u8; (MAX_BYTES_RATIONALE) as usize]; let revert_account = T::AccountId::create_account_id(2u32); - }: _(RawOrigin::Signed(operator_account), transfer_id, revert_account.clone(), revert_amount.into(), rationale.clone()) + }: _(RawOrigin::Signed(operator_account), transfer_id, revert_account.clone(), revert_amount.into(), rationale.clone().try_into().unwrap()) verify { assert_last_event::( - RawEvent::OutboundTransferReverted(transfer_id, revert_account, revert_amount.into(), rationale).into()); + RawEvent::OutboundTransferReverted(transfer_id, revert_account, revert_amount.into(), rationale.try_into().unwrap()).into()); } diff --git a/runtime-modules/argo-bridge/src/events.rs b/runtime-modules/argo-bridge/src/events.rs index e2b2bc5076..0f8aad6b3b 100644 --- a/runtime-modules/argo-bridge/src/events.rs +++ b/runtime-modules/argo-bridge/src/events.rs @@ -3,10 +3,12 @@ use frame_support::decl_event; use crate::{RemoteAccount, RemoteTransfer, TransferId}; -use sp_std::vec::Vec; use crate::types::*; +use frame_support::storage::bounded_vec::BoundedVec; +use frame_support::traits::ConstU32; + // Balance type alias type BalanceOf = ::Balance; @@ -20,7 +22,12 @@ decl_event!( { OutboundTransferRequested(TransferId, AccountId, RemoteAccount, Balance, Balance), InboundTransferFinalized(RemoteTransfer, AccountId, Balance), - OutboundTransferReverted(TransferId, AccountId, Balance, Vec), + OutboundTransferReverted( + TransferId, + AccountId, + Balance, + BoundedVec>, + ), BridgePaused(AccountId), BridgeThawnStarted(AccountId, BlockNumber), BridgeThawnFinished(), diff --git a/runtime-modules/argo-bridge/src/lib.rs b/runtime-modules/argo-bridge/src/lib.rs index 9ea9cc11a1..0b9b34401a 100644 --- a/runtime-modules/argo-bridge/src/lib.rs +++ b/runtime-modules/argo-bridge/src/lib.rs @@ -144,7 +144,7 @@ decl_module! { transfer_id: TransferId, revert_account: T::AccountId, revert_amount: BalanceOf, - rationale: vec::Vec, + rationale: BoundedVec>, ) -> DispatchResult { ensure!(Self::operator_account().is_some(), Error::::OperatorAccountNotSet); let caller = ensure_signed(origin)?; diff --git a/runtime-modules/argo-bridge/src/tests/mod.rs b/runtime-modules/argo-bridge/src/tests/mod.rs index a604912d31..69695bf5dd 100644 --- a/runtime-modules/argo-bridge/src/tests/mod.rs +++ b/runtime-modules/argo-bridge/src/tests/mod.rs @@ -1,6 +1,6 @@ #![cfg(test)] -use core::convert::TryFrom; +use core::convert::{TryFrom, TryInto}; use crate::tests::mock::*; use frame_support::{assert_err, assert_ok}; @@ -338,7 +338,7 @@ fn revert_outbound_transfer_success() { transfer_id, revert_account, revert_amount, - rationale.clone(), + rationale.clone().try_into().unwrap(), ); assert_ok!(result); assert_eq!(Balances::free_balance(revert_account), revert_amount); @@ -346,7 +346,7 @@ fn revert_outbound_transfer_success() { transfer_id, revert_account, revert_amount, - rationale, + rationale.try_into().unwrap(), )); }); } @@ -359,7 +359,7 @@ fn revert_outbound_transfer_with_no_operator_account() { 1u64, account!(2), joy!(123), - vec![], + vec![].try_into().unwrap(), ); assert_err!(result, Error::::OperatorAccountNotSet); }); @@ -386,7 +386,7 @@ fn revert_outbound_transfer_with_unauthorized_account() { 1u64, account!(2), joy!(123), - vec![], + vec![].try_into().unwrap(), ); assert_err!(result, Error::::NotOperatorAccount); }); @@ -413,7 +413,7 @@ fn revert_outbound_transfer_with_insufficient_bridge_mint() { 1u64, account!(2), joy!(100), - vec![], + vec![].try_into().unwrap(), ); assert_err!(result, Error::::InsufficientBridgeMintAllowance); }); diff --git a/runtime-modules/argo-bridge/src/types.rs b/runtime-modules/argo-bridge/src/types.rs index 25b674bab9..b1b0ecf952 100644 --- a/runtime-modules/argo-bridge/src/types.rs +++ b/runtime-modules/argo-bridge/src/types.rs @@ -14,6 +14,8 @@ pub type TransferId = u64; pub const MAX_REMOTE_CHAINS: u32 = 10; +pub const MAX_BYTES_RATIONALE: u32 = 200 * 1000; // 200 Kb + #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] #[derive(Encode, Decode, Clone, PartialEq, Eq, Debug, TypeInfo, MaxEncodedLen)] pub struct BridgeConstraints { From 0e611ec8250f3bfcf5cf4f7d3e56c25576bf8546 Mon Sep 17 00:00:00 2001 From: Ricardo Date: Tue, 4 Jun 2024 19:29:57 +0100 Subject: [PATCH 3/4] update Argo Bridge weights --- runtime-modules/argo-bridge/src/lib.rs | 3 +- runtime-modules/argo-bridge/src/weights.rs | 48 ++++++++++++++++------ 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/runtime-modules/argo-bridge/src/lib.rs b/runtime-modules/argo-bridge/src/lib.rs index 0b9b34401a..8f8368991f 100644 --- a/runtime-modules/argo-bridge/src/lib.rs +++ b/runtime-modules/argo-bridge/src/lib.rs @@ -137,8 +137,7 @@ decl_module! { Ok(()) } - // TODO: add weight for revert_outbound_transfer - #[weight = WeightInfoArgo::::finalize_inbound_transfer()] + #[weight = WeightInfoArgo::::revert_outbound_transfer()] pub fn revert_outbound_transfer( origin, transfer_id: TransferId, diff --git a/runtime-modules/argo-bridge/src/weights.rs b/runtime-modules/argo-bridge/src/weights.rs index 0fd08a2c54..0e424f43c6 100644 --- a/runtime-modules/argo-bridge/src/weights.rs +++ b/runtime-modules/argo-bridge/src/weights.rs @@ -18,7 +18,7 @@ //! Autogenerated weights for argo_bridge //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev -//! DATE: 2024-05-31, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2024-06-04, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("prod-test"), DB CACHE: 1024 // Executed Command: @@ -45,6 +45,7 @@ use sp_std::marker::PhantomData; /// Weight functions needed for argo_bridge. pub trait WeightInfo { fn request_outbound_transfer() -> Weight; + fn revert_outbound_transfer() -> Weight; fn finalize_inbound_transfer() -> Weight; fn pause_bridge() -> Weight; fn init_unpause_bridge() -> Weight; @@ -71,8 +72,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `430` // Estimated: `11104` - // Minimum execution time: 998_107 nanoseconds. - Weight::from_parts(1_038_838_000, 0u64) + // Minimum execution time: 1_128_198 nanoseconds. + Weight::from_parts(1_308_617_000, 0u64) .saturating_add(Weight::from_parts(0, 11104)) .saturating_add(T::DbWeight::get().reads(6_u64)) .saturating_add(T::DbWeight::get().writes(3_u64)) @@ -83,6 +84,24 @@ impl WeightInfo for SubstrateWeight { // Proof: ArgoBridge Status (max_values: Some(1), max_size: Some(5), added: 500, mode: MaxEncodedLen) // Storage: ArgoBridge MintAllowance (r:1 w:1) // Proof: ArgoBridge MintAllowance (max_values: Some(1), max_size: Some(16), added: 511, mode: MaxEncodedLen) + // Storage: System Account (r:1 w:0) + // Proof: System Account (max_values: None, max_size: Some(128), added: 2603, mode: MaxEncodedLen) + fn revert_outbound_transfer() -> Weight { + // Proof Size summary in bytes: + // Measured: `321` + // Estimated: `8101` + // Minimum execution time: 858_538 nanoseconds. + Weight::from_parts(937_558_000, 0u64) + .saturating_add(Weight::from_parts(0, 8101)) + .saturating_add(T::DbWeight::get().reads(4_u64)) + .saturating_add(T::DbWeight::get().writes(1_u64)) + } + // Storage: ArgoBridge OperatorAccount (r:1 w:0) + // Proof: ArgoBridge OperatorAccount (max_values: Some(1), max_size: Some(32), added: 527, mode: MaxEncodedLen) + // Storage: ArgoBridge Status (r:1 w:0) + // Proof: ArgoBridge Status (max_values: Some(1), max_size: Some(5), added: 500, mode: MaxEncodedLen) + // Storage: ArgoBridge MintAllowance (r:1 w:1) + // Proof: ArgoBridge MintAllowance (max_values: Some(1), max_size: Some(16), added: 511, mode: MaxEncodedLen) // Storage: ArgoBridge RemoteChains (r:1 w:0) // Proof: ArgoBridge RemoteChains (max_values: Some(1), max_size: Some(41), added: 536, mode: MaxEncodedLen) // Storage: System Account (r:1 w:1) @@ -91,8 +110,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `473` // Estimated: `9627` - // Minimum execution time: 883_138 nanoseconds. - Weight::from_parts(900_668_000, 0u64) + // Minimum execution time: 967_528 nanoseconds. + Weight::from_parts(1_171_918_000, 0u64) .saturating_add(Weight::from_parts(0, 9627)) .saturating_add(T::DbWeight::get().reads(5_u64)) .saturating_add(T::DbWeight::get().writes(2_u64)) @@ -105,8 +124,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `498` // Estimated: `1806` - // Minimum execution time: 371_039 nanoseconds. - Weight::from_parts(382_129_000, 0u64) + // Minimum execution time: 414_149 nanoseconds. + Weight::from_parts(424_310_000, 0u64) .saturating_add(Weight::from_parts(0, 1806)) .saturating_add(T::DbWeight::get().reads(1_u64)) .saturating_add(T::DbWeight::get().writes(1_u64)) @@ -121,8 +140,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `498` // Estimated: `3295` - // Minimum execution time: 402_659 nanoseconds. - Weight::from_parts(420_319_000, 0u64) + // Minimum execution time: 426_939 nanoseconds. + Weight::from_parts(467_489_000, 0u64) .saturating_add(Weight::from_parts(0, 3295)) .saturating_add(T::DbWeight::get().reads(2_u64)) .saturating_add(T::DbWeight::get().writes(1_u64)) @@ -135,8 +154,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `173` // Estimated: `3007` - // Minimum execution time: 412_269 nanoseconds. - Weight::from_parts(422_459_000, 0u64) + // Minimum execution time: 613_939 nanoseconds. + Weight::from_parts(791_438_000, 0u64) .saturating_add(Weight::from_parts(0, 3007)) .saturating_add(T::DbWeight::get().reads(2_u64)) .saturating_add(T::DbWeight::get().writes(1_u64)) @@ -155,8 +174,8 @@ impl WeightInfo for SubstrateWeight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` - // Minimum execution time: 293_519 nanoseconds. - Weight::from_parts(298_009_000, 0u64) + // Minimum execution time: 355_860 nanoseconds. + Weight::from_parts(454_089_000, 0u64) .saturating_add(Weight::from_parts(0, 0)) .saturating_add(T::DbWeight::get().writes(5_u64)) } @@ -167,6 +186,9 @@ impl WeightInfo for () { fn request_outbound_transfer() -> Weight { Weight::from_parts(0, 0) } + fn revert_outbound_transfer() -> Weight { + Weight::from_parts(0, 0) + } fn finalize_inbound_transfer() -> Weight { Weight::from_parts(0, 0) } From 7ba4ffb97721abd7857f65d01b667ba0e19e2a33 Mon Sep 17 00:00:00 2001 From: Ricardo Date: Tue, 4 Jun 2024 20:46:02 +0100 Subject: [PATCH 4/4] fix: decrease revert_outbound_transfer rationale to 200 --- runtime-modules/argo-bridge/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime-modules/argo-bridge/src/types.rs b/runtime-modules/argo-bridge/src/types.rs index b1b0ecf952..366df313dc 100644 --- a/runtime-modules/argo-bridge/src/types.rs +++ b/runtime-modules/argo-bridge/src/types.rs @@ -14,7 +14,7 @@ pub type TransferId = u64; pub const MAX_REMOTE_CHAINS: u32 = 10; -pub const MAX_BYTES_RATIONALE: u32 = 200 * 1000; // 200 Kb +pub const MAX_BYTES_RATIONALE: u32 = 200; #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] #[derive(Encode, Decode, Clone, PartialEq, Eq, Debug, TypeInfo, MaxEncodedLen)]