From 5b922a210b2a6705d3ea6fefbf67b317698f7b80 Mon Sep 17 00:00:00 2001 From: Hugo Lindsay <8341280+ashneverdawn@users.noreply.github.com> Date: Wed, 19 Jul 2023 09:24:17 -0400 Subject: [PATCH] 256 audit pdm 006 add weight charging to chain extension calls (#280) * add placeholder weights to chain extensions * update weights * fix issues and generate weights * fix tests * remove trace, shorten allowance weight info --- Cargo.toml | 1 + .../src/benchmarking.rs | 70 +++++++++++-- .../src/default_weights.rs | 99 +++++++++++-------- .../src/lib.rs | 56 ++++++++++- .../src/mock.rs | 2 +- runtime/foucoco/Cargo.toml | 2 + runtime/foucoco/src/lib.rs | 39 +++++++- 7 files changed, 213 insertions(+), 56 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index edee14c7e..869d53a2b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,7 @@ members = [ "node", "pallets/parachain-staking", "pallets/vesting-manager", + "pallets/orml-currencies-allowance-extension", "runtime/common", "runtime/amplitude", "runtime/foucoco", diff --git a/pallets/orml-currencies-allowance-extension/src/benchmarking.rs b/pallets/orml-currencies-allowance-extension/src/benchmarking.rs index a333bc9da..f712b11b7 100644 --- a/pallets/orml-currencies-allowance-extension/src/benchmarking.rs +++ b/pallets/orml-currencies-allowance-extension/src/benchmarking.rs @@ -6,24 +6,82 @@ use sp_std::prelude::*; benchmarks! { add_allowed_currencies { - let native_currency_id = ::GetNativeCurrencyId::get(); + let native_currency_id = ::GetNativeCurrencyId::get(); let added_currencies: Vec> = vec![native_currency_id]; - }: _(RawOrigin::Root, added_currencies) + }: add_allowed_currencies(RawOrigin::Root, added_currencies) verify { - let native_currency_id = ::GetNativeCurrencyId::get(); + let native_currency_id = ::GetNativeCurrencyId::get(); assert_eq!(AllowedCurrencies::::get(native_currency_id), Some(())); } remove_allowed_currencies { - let native_currency_id = ::GetNativeCurrencyId::get(); + let native_currency_id = ::GetNativeCurrencyId::get(); AllowedCurrencies::::insert(native_currency_id, ()); let removed_currencies: Vec> = vec![native_currency_id]; - }: _(RawOrigin::Root, removed_currencies) + }: remove_allowed_currencies(RawOrigin::Root, removed_currencies) verify { - let native_currency_id = ::GetNativeCurrencyId::get(); + let native_currency_id = ::GetNativeCurrencyId::get(); assert_eq!(AllowedCurrencies::::get(native_currency_id), None); } + + approve { + //allow currency + let native_currency_id = ::GetNativeCurrencyId::get(); + AllowedCurrencies::::insert(native_currency_id, ()); + + //initialize accounts + let owner: T::AccountId = account("Alice", 0, 0); + let delegate: T::AccountId = account("Bob", 0, 0); + + //fund account + let amount = BalanceOf::::from(1_000_000_000u32); + as MultiCurrency>::deposit(native_currency_id, &owner, amount); + + }: approve(RawOrigin::Signed(owner), native_currency_id, delegate, amount) + verify { + let native_currency_id = ::GetNativeCurrencyId::get(); + let owner: T::AccountId = account("Alice", 0, 0); + let delegate: T::AccountId = account("Bob", 0, 0); + let amount = BalanceOf::::from(1_000_000_000u32); + + //check that the allowance was updated + assert_eq!(TokenAllowance::::allowance(native_currency_id, &owner, &delegate), amount); + } + + transfer_from { + //allow currency + let native_currency_id = ::GetNativeCurrencyId::get(); + AllowedCurrencies::::insert(native_currency_id, ()); + + //initialize accounts + let owner: T::AccountId = account("Alice", 0, 0); + let delegate: T::AccountId = account("Bob", 0, 0); + let destination: T::AccountId = account("Charlie", 0, 0); + + //fund accounts + let amount = BalanceOf::::from(1_000_000_000u32); + as MultiCurrency>::deposit(native_currency_id, &owner, amount); + as MultiCurrency>::deposit(native_currency_id, &delegate, amount); + + //approve + TokenAllowance::::do_approve_transfer(native_currency_id, &owner, &delegate, amount); + + }: transfer_from(RawOrigin::Signed(delegate), native_currency_id, owner, destination, amount) + verify { + let native_currency_id = ::GetNativeCurrencyId::get(); + let owner: T::AccountId = account("Alice", 0, 0); + let delegate: T::AccountId = account("Bob", 0, 0); + let destination: T::AccountId = account("Charlie", 0, 0); + let amount = BalanceOf::::from(1_000_000_000u32); + + //check that the allowance was updated + assert_eq!(TokenAllowance::::allowance(native_currency_id, &owner, &delegate), BalanceOf::::from(0u32)); + + //check that the balance was updated + let destination_balance = as MultiCurrency>::free_balance(native_currency_id, &destination); + assert_eq!(destination_balance, amount); + } } impl_benchmark_test_suite!(TokenAllowance, crate::mock::ExtBuilder::build(), crate::mock::Test); diff --git a/pallets/orml-currencies-allowance-extension/src/default_weights.rs b/pallets/orml-currencies-allowance-extension/src/default_weights.rs index 205b4de13..b97de2a0c 100644 --- a/pallets/orml-currencies-allowance-extension/src/default_weights.rs +++ b/pallets/orml-currencies-allowance-extension/src/default_weights.rs @@ -1,67 +1,84 @@ -//! Autogenerated weights for orml_currencies_allowance_extension +//! Autogenerated weights for `orml_currencies_allowance_extension` //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev -//! DATE: 2023-03-20, STEPS: `100`, REPEAT: 10, LOW RANGE: `[]`, HIGH RANGE: `[]` -//! HOSTNAME: `MacBook-Pro`, CPU: `` -//! EXECUTION: None, WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 1024 +//! DATE: 2023-07-18, STEPS: `100`, REPEAT: `10`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! WORST CASE MAP SIZE: `1000000` +//! HOSTNAME: `Hugos-MacBook-Pro.local`, CPU: `` +//! EXECUTION: None, WASM-EXECUTION: Compiled, CHAIN: Some("foucoco"), DB CACHE: 1024 // Executed Command: -// ./target/release/spacewalk-standalone +// ./target/release/pendulum-node // benchmark // pallet -// --chain=dev +// --chain=foucoco // --pallet=orml-currencies-allowance-extension // --extrinsic=* // --steps=100 // --repeat=10 // --wasm-execution=compiled -// --output=pallets/default_weights.rs -// --template=./.maintain/frame-weight-template.hbs +// --output=pallets/orml-currencies-allowance-extension/src/default_weights.rs #![cfg_attr(rustfmt, rustfmt_skip)] #![allow(unused_parens)] #![allow(unused_imports)] -#![allow(clippy::unnecessary_cast)] -use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; +use frame_support::{traits::Get, weights::Weight}; use sp_std::marker::PhantomData; -/// Weight functions needed for orml_currencies_allowance_extension. -pub trait WeightInfo { - fn add_allowed_currencies() -> Weight; - fn remove_allowed_currencies() -> Weight; -} - -/// Weights for orml_currencies_allowance_extension using the Substrate node and recommended hardware. -pub struct SubstrateWeight(PhantomData); -impl WeightInfo for SubstrateWeight { - // Storage: TokenAllowance AllowedCurrencies (r:0 w:1) +/// Weight functions for `orml_currencies_allowance_extension`. +pub struct WeightInfo(PhantomData); +impl crate::WeightInfo for WeightInfo { + /// Storage: TokenAllowance AllowedCurrencies (r:0 w:1) + /// Proof Skipped: TokenAllowance AllowedCurrencies (max_values: None, max_size: None, mode: Measured) fn add_allowed_currencies() -> Weight { - // Minimum execution time: 12_000 nanoseconds. - Weight::from_parts(14_000_000 as u64, 0) - .saturating_add(T::DbWeight::get().writes(1 as u64)) + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 5_000_000 picoseconds. + Weight::from_parts(6_000_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) } - // Storage: TokenAllowance AllowedCurrencies (r:0 w:1) + /// Storage: TokenAllowance AllowedCurrencies (r:0 w:1) + /// Proof Skipped: TokenAllowance AllowedCurrencies (max_values: None, max_size: None, mode: Measured) fn remove_allowed_currencies() -> Weight { - // Minimum execution time: 31_000 nanoseconds. - Weight::from_parts(32_000_000 as u64, 0) - .saturating_add(T::DbWeight::get().writes(1 as u64)) + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 5_000_000 picoseconds. + Weight::from_parts(6_000_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) } -} - -// For backwards compatibility and tests -impl WeightInfo for () { - // Storage: TokenAllowance AllowedCurrencies (r:0 w:1) - fn add_allowed_currencies() -> Weight { - // Minimum execution time: 12_000 nanoseconds. - Weight::from_parts(14_000_000 as u64, 0) - .saturating_add(RocksDbWeight::get().writes(1 as u64)) + /// Storage: TokenAllowance AllowedCurrencies (r:1 w:0) + /// Proof Skipped: TokenAllowance AllowedCurrencies (max_values: None, max_size: None, mode: Measured) + /// Storage: TokenAllowance Approvals (r:1 w:1) + /// Proof Skipped: TokenAllowance Approvals (max_values: None, max_size: None, mode: Measured) + fn approve() -> Weight { + // Proof Size summary in bytes: + // Measured: `184` + // Estimated: `7298` + // Minimum execution time: 11_000_000 picoseconds. + Weight::from_parts(12_000_000, 0) + .saturating_add(Weight::from_parts(0, 7298)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(1)) } - // Storage: TokenAllowance AllowedCurrencies (r:0 w:1) - fn remove_allowed_currencies() -> Weight { - // Minimum execution time: 31_000 nanoseconds. - Weight::from_parts(32_000_000 as u64, 0) - .saturating_add(RocksDbWeight::get().writes(1 as u64)) + /// Storage: TokenAllowance AllowedCurrencies (r:1 w:0) + /// Proof Skipped: TokenAllowance AllowedCurrencies (max_values: None, max_size: None, mode: Measured) + /// Storage: TokenAllowance Approvals (r:1 w:1) + /// Proof Skipped: TokenAllowance Approvals (max_values: None, max_size: None, mode: Measured) + /// Storage: System Account (r:2 w:2) + /// Proof: System Account (max_values: None, max_size: Some(128), added: 2603, mode: MaxEncodedLen) + fn transfer_from() -> Weight { + // Proof Size summary in bytes: + // Measured: `561` + // Estimated: `14248` + // Minimum execution time: 29_000_000 picoseconds. + Weight::from_parts(30_000_000, 0) + .saturating_add(Weight::from_parts(0, 14248)) + .saturating_add(T::DbWeight::get().reads(4)) + .saturating_add(T::DbWeight::get().writes(3)) } } diff --git a/pallets/orml-currencies-allowance-extension/src/lib.rs b/pallets/orml-currencies-allowance-extension/src/lib.rs index 8f3f0fe1b..1654831d0 100644 --- a/pallets/orml-currencies-allowance-extension/src/lib.rs +++ b/pallets/orml-currencies-allowance-extension/src/lib.rs @@ -5,8 +5,8 @@ #[cfg(test)] extern crate mocktopus; -pub use default_weights::{SubstrateWeight, WeightInfo}; -use frame_support::{dispatch::DispatchResult, ensure}; +use frame_support::{dispatch::DispatchResult, ensure, weights::Weight}; + #[cfg(test)] use mocktopus::macros::mockable; use orml_traits::MultiCurrency; @@ -16,7 +16,7 @@ use sp_std::{convert::TryInto, prelude::*, vec}; #[cfg(feature = "runtime-benchmarks")] mod benchmarking; -mod default_weights; +pub mod default_weights; #[cfg(test)] mod mock; @@ -36,10 +36,18 @@ pub(crate) type CurrencyOf = ::AccountId, >>::CurrencyId; +/// Weight functions needed for orml_currencies_allowance_extension. +pub trait WeightInfo { + fn add_allowed_currencies() -> Weight; + fn remove_allowed_currencies() -> Weight; + fn approve() -> Weight; + fn transfer_from() -> Weight; +} + #[frame_support::pallet] pub mod pallet { use frame_support::{pallet_prelude::*, transactional}; - use frame_system::{ensure_root, pallet_prelude::OriginFor}; + use frame_system::{ensure_root, ensure_signed, pallet_prelude::OriginFor}; use super::*; @@ -166,6 +174,46 @@ pub mod pallet { Self::deposit_event(Event::AllowedCurrenciesDeleted { currencies }); Ok(()) } + + /// Approve an amount for another account to spend on owner's behalf. + /// + /// # Arguments + /// * `id` - the currency_id of the asset to approve + /// * `delegate` - the spender account to approve the asset for + /// * `amount` - the amount of the asset to approve + #[pallet::call_index(2)] + #[pallet::weight(::WeightInfo::approve())] + #[transactional] + pub fn approve( + origin: OriginFor, + id: CurrencyOf, + delegate: T::AccountId, + amount: BalanceOf, + ) -> DispatchResult { + let owner = ensure_signed(origin)?; + Self::do_approve_transfer(id, &owner, &delegate, amount) + } + + /// Execute a pre-approved transfer from another account + /// + /// # Arguments + /// * `id` - the currency_id of the asset to transfer + /// * `owner` - the owner account of the asset to transfer + /// * `destination` - the destination account to transfer to + /// * `amount` - the amount of the asset to transfer + #[pallet::call_index(3)] + #[pallet::weight(::WeightInfo::transfer_from())] + #[transactional] + pub fn transfer_from( + origin: OriginFor, + id: CurrencyOf, + owner: T::AccountId, + destination: T::AccountId, + amount: BalanceOf, + ) -> DispatchResult { + let delegate = ensure_signed(origin)?; + Self::do_transfer_approved(id, &owner, &delegate, &destination, amount) + } } } diff --git a/pallets/orml-currencies-allowance-extension/src/mock.rs b/pallets/orml-currencies-allowance-extension/src/mock.rs index 23729b149..e836d2c77 100644 --- a/pallets/orml-currencies-allowance-extension/src/mock.rs +++ b/pallets/orml-currencies-allowance-extension/src/mock.rs @@ -136,7 +136,7 @@ impl orml_currencies::Config for Test { impl Config for Test { type RuntimeEvent = RuntimeEvent; - type WeightInfo = (); + type WeightInfo = token_allowance::default_weights::WeightInfo; } pub struct ExtBuilder; diff --git a/runtime/foucoco/Cargo.toml b/runtime/foucoco/Cargo.toml index ca5f57a8d..dc3437892 100644 --- a/runtime/foucoco/Cargo.toml +++ b/runtime/foucoco/Cargo.toml @@ -253,6 +253,8 @@ runtime-benchmarks = [ "replace/runtime-benchmarks", "stellar-relay/runtime-benchmarks", "vault-registry/runtime-benchmarks", + + "orml-currencies-allowance-extension/runtime-benchmarks", ] try-runtime = [ diff --git a/runtime/foucoco/src/lib.rs b/runtime/foucoco/src/lib.rs index 2565d205e..d9f3368c8 100644 --- a/runtime/foucoco/src/lib.rs +++ b/runtime/foucoco/src/lib.rs @@ -97,6 +97,9 @@ use spacewalk_primitives::{ }; use weights::{BlockExecutionWeight, ExtrinsicBaseWeight, RocksDbWeight}; +use orml_currencies::WeightInfo; + +use orml_currencies_allowance_extension::{Config as AllowanceConfig, WeightInfo as AllowanceWeightInfo}; use frame_support::{ log::{error, warn}, @@ -961,10 +964,22 @@ where warn!("Calling function with ID {} from Psp22Extension", func_id); + // debug_message weight is a good approximation of the additional overhead of going + // from contract layer to substrate layer. + let overhead_weight = Weight::from_parts( + ::Schedule::get() + .host_fn_weights + .debug_message + .ref_time(), + 0, + ); + match func_id { // totalSupply(currency) 1101 => { let mut env = env.buf_in_buf_out(); + let base_weight = ::DbWeight::get().reads(1); + env.charge_weight(base_weight.saturating_add(overhead_weight))?; let input = env.read(256)?; let currency_id: CurrencyId = chain_ext::decode(input) .map_err(|_| DispatchError::Other("ChainExtension failed to decode input"))?; @@ -990,6 +1005,8 @@ where // balanceOf(currency, account) 1102 => { let mut env = env.buf_in_buf_out(); + let base_weight = ::DbWeight::get().reads(1); + env.charge_weight(base_weight.saturating_add(overhead_weight))?; let input = env.read(256)?; let (currency_id, account_id): (CurrencyId, T::AccountId) = chain_ext::decode(input).map_err(|_| { @@ -1022,7 +1039,11 @@ where let ext = env.ext(); let caller = ext.caller().clone(); - let env = env.buf_in_buf_out(); + let mut env = env.buf_in_buf_out(); + // Here we use weights for non native currency as worst case scenario, since we can't know whether it's native or not until we've already read from contract env. + let base_weight = + ::WeightInfo::transfer_non_native_currency(); + env.charge_weight(base_weight.saturating_add(overhead_weight))?; let input = env.read(256)?; let (currency_id, recipient, amount): ( CurrencyId, @@ -1053,6 +1074,8 @@ where // allowance(currency, owner, spender) 1104 => { let mut env = env.buf_in_buf_out(); + let base_weight = ::DbWeight::get().reads(1); + env.charge_weight(base_weight.saturating_add(overhead_weight))?; let input = env.read(256)?; let (currency_id, owner, spender): (CurrencyId, T::AccountId, T::AccountId) = chain_ext::decode(input).map_err(|_| { @@ -1085,7 +1108,9 @@ where let ext = env.ext(); let caller = ext.caller().clone(); - let env = env.buf_in_buf_out(); + let mut env = env.buf_in_buf_out(); + let base_weight = <::WeightInfo as AllowanceWeightInfo>::approve(); + env.charge_weight(base_weight.saturating_add(overhead_weight))?; let input = env.read(256)?; let (currency_id, spender, amount): ( CurrencyId, @@ -1118,7 +1143,9 @@ where let ext = env.ext(); let caller = ext.caller().clone(); - let env = env.buf_in_buf_out(); + let mut env = env.buf_in_buf_out(); + let base_weight = <::WeightInfo as AllowanceWeightInfo>::transfer_from(); + env.charge_weight(base_weight.saturating_add(overhead_weight))?; let input = env.read(256)?; let (owner, currency_id, recipient, amount): ( T::AccountId, @@ -1152,6 +1179,8 @@ where // get_coin_info(blockchain, symbol) 1200 => { let mut env = env.buf_in_buf_out(); + let base_weight = ::DbWeight::get().reads(1); + env.charge_weight(base_weight.saturating_add(overhead_weight))?; let (blockchain, symbol): (Blockchain, Symbol) = env.read_as()?; let result = as DiaOracle>::get_coin_info( @@ -1212,7 +1241,7 @@ impl pallet_insecure_randomness_collective_flip::Config for Runtime {} impl orml_currencies_allowance_extension::Config for Runtime { type RuntimeEvent = RuntimeEvent; - type WeightInfo = orml_currencies_allowance_extension::SubstrateWeight; + type WeightInfo = orml_currencies_allowance_extension::default_weights::WeightInfo; } parameter_types! { @@ -1566,6 +1595,8 @@ mod benches { [stellar_relay, StellarRelay] [vault_registry, VaultRegistry] [pallet_xcm, PolkadotXcm] + + [orml_currencies_allowance_extension, TokenAllowance] ); }