Skip to content

Commit

Permalink
fix: approve benchmark + weights
Browse files Browse the repository at this point in the history
  • Loading branch information
@Daanvdplas
Daanvdplas committed Jul 25, 2024
1 parent f0fff97 commit f572fee
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 63 deletions.
85 changes: 56 additions & 29 deletions pallets/api/src/fungibles/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,26 +24,34 @@ fn assert_has_event<T: Config>(
frame_system::Pallet::<T>::assert_has_event(generic_event.into());
}

// Compare `generic_event` to the last emitted event.
fn assert_last_event<T: Config>(

Check warning on line 28 in pallets/api/src/fungibles/benchmarking.rs

View workflow job for this annotation

GitHub Actions / clippy

function `assert_last_event` is never used 

warning: function `assert_last_event` is never used
  --> pallets/api/src/fungibles/benchmarking.rs:28:4
   |
28 | fn assert_last_event<T: Config>(
   |    ^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default
generic_event: <T as pallet_assets::Config<AssetsInstanceOf<T>>>::RuntimeEvent,
) {
frame_system::Pallet::<T>::assert_last_event(generic_event.into());
}

#[benchmarks(
where
<pallet_assets::Pallet<T, AssetsInstanceOf<T>> as Inspect<<T as frame_system::Config>::AccountId>>::AssetId: Zero,
)]
mod benchmarks {
use super::*;

// The worst case scenario is when the allowance is set to a value which is lower than the
// current allowance.
// Parameter:
// - 'a': whether `approve_transfer` has been called.
// - 'c': whether `cancel_approval` has been called.
#[benchmark]
fn approve() -> Result<(), BenchmarkError> {
fn approve(a: Linear<0, 1>, c: Linear<0, 1>) -> Result<(), BenchmarkError> {
let asset_id = AssetIdOf::<T>::zero();
let decreased_value = <BalanceOf<T>>::from(50u32);
let min_balance = <BalanceOf<T>>::from(1u32);
let owner: AccountIdOf<T> = account("Alice", 0, SEED);
let spender: AccountIdOf<T> = account("Bob", 0, SEED);
let value = <BalanceOf<T>>::from(100u32);
let current_allowance = <BalanceOf<T>>::from(u32::MAX / 2);
T::Currency::make_free_balance_be(&owner, u32::MAX.into());
// Set the `current_allowance`.
assert_ok!(<AssetsOf<T> as Create<AccountIdOf<T>>>::create(
asset_id.clone().into(),
asset_id.clone(),
owner.clone(),
true,
min_balance
Expand All @@ -52,33 +60,52 @@ mod benchmarks {
asset_id.clone(),
&owner,
&spender,
value
current_allowance,
));
let approval_value = match (a, c) {
// Equal to the current allowance.
(0, 0) => current_allowance,
// Greater than the current allowance.
(1, 0) => <BalanceOf<T>>::from(u32::MAX),
// Zero.
(0, 1) => <BalanceOf<T>>::from(0u32),
// Smaller than the current allowance.
(1, 1) => <BalanceOf<T>>::from(u32::MAX / 4),
_ => unreachable!("values can only be 0 or 1"),
};

#[extrinsic_call]
_(RawOrigin::Signed(owner.clone()), asset_id.clone(), spender.clone(), decreased_value);

assert_eq!(AssetsOf::<T>::allowance(asset_id.clone(), &owner, &spender), decreased_value);
// To make sure both dispatchables have been successfully executed - i.e. worst case
// scenario.
assert_has_event::<T>(
pallet_assets::Event::ApprovalCancelled {
asset_id: asset_id.clone(),
owner: owner.clone(),
delegate: spender.clone(),
}
.into(),
);
assert_has_event::<T>(
pallet_assets::Event::ApprovedTransfer {
asset_id,
source: owner,
delegate: spender,
amount: value,
}
.into(),
);
_(RawOrigin::Signed(owner.clone()), asset_id.clone(), spender.clone(), approval_value);

assert_eq!(AssetsOf::<T>::allowance(asset_id.clone(), &owner, &spender), approval_value);
if c == 1 {
assert_has_event::<T>(
pallet_assets::Event::ApprovalCancelled {
asset_id: asset_id.clone(),
owner: owner.clone(),
delegate: spender.clone(),
}
.into(),
);
}
if a == 1 {
let amount = match c {
// When the allowance was cancelled and then approved with the new value.
1 => approval_value,
// When the allowance was increased.
0 => approval_value - current_allowance,
_ => unreachable!("`c` can only be 0 or 1"),
};
assert_has_event::<T>(
pallet_assets::Event::ApprovedTransfer {
asset_id,
source: owner,
delegate: spender,
amount,
}
.into(),
);
}
Ok(())
}

Expand Down
35 changes: 17 additions & 18 deletions pallets/api/src/fungibles/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ pub mod pallet {
traits::fungibles::approvals::Inspect as ApprovalInspect,
};
use frame_system::pallet_prelude::*;
use sp_runtime::{traits::StaticLookup, Saturating};
use sp_runtime::{
traits::{StaticLookup, Zero},
Saturating,
};
use sp_std::vec::Vec;

/// State reads for the fungibles api with required input.
Expand Down Expand Up @@ -101,22 +104,23 @@ pub mod pallet {
/// * `spender` - The account that is allowed to spend the tokens.
/// * `value` - The number of tokens to approve.
#[pallet::call_index(5)]
#[pallet::weight(<T as Config>::WeightInfo::approve())]
#[pallet::weight(<T as Config>::WeightInfo::approve(1, 1))]
pub fn approve(
origin: OriginFor<T>,
id: AssetIdOf<T>,
spender: AccountIdOf<T>,
value: BalanceOf<T>,
) -> DispatchResultWithPostInfo {
let who = ensure_signed(origin.clone())
// To have the caller pay some fees.
.map_err(|e| e.with_weight(T::DbWeight::get().reads(1)))?;
let weight = |approve: u32, cancel: u32| -> Weight {
<T as Config>::WeightInfo::approve(cancel, approve)
};
let who = ensure_signed(origin.clone()).map_err(|e| e.with_weight(weight(0, 0)))?;
let current_allowance = AssetsOf::<T>::allowance(id.clone(), &who, &spender);
let spender = T::Lookup::unlookup(spender);
let id: AssetIdParameterOf<T> = id.into();
// If the new value is equal to the current allowance, do nothing.
if value == current_allowance {
return Ok(().into());
return Ok(Some(weight(0, 0)).into());
}
// If the new value is greater than the current allowance, approve the difference
// because `approve_transfer` works additively (see pallet-assets).
Expand All @@ -127,23 +131,18 @@ pub mod pallet {
spender,
value.saturating_sub(current_allowance),
)
.map_err(|e| {
e.with_weight(
T::DbWeight::get().reads(1) + AssetsWeightInfoOf::<T>::approve_transfer(),
)
})?;
.map_err(|e| e.with_weight(weight(1, 0)))?;
Ok(Some(weight(1, 0)).into())
} else {
// If the new value is less than the current allowance, cancel the approval and set the new value
AssetsOf::<T>::cancel_approval(origin.clone(), id.clone(), spender.clone())
.map_err(|e| {
e.with_weight(
T::DbWeight::get().reads(1)
+ AssetsWeightInfoOf::<T>::cancel_approval(),
)
})?;
.map_err(|e| e.with_weight(weight(0, 1)))?;
if value.is_zero() {
return Ok(Some(weight(0, 1)).into());
}
AssetsOf::<T>::approve_transfer(origin, id, spender, value)?;
Ok(().into())
}
Ok(().into())
}

/// Increases the allowance of a spender.
Expand Down
51 changes: 35 additions & 16 deletions pallets/api/src/fungibles/weights.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@

//! Autogenerated weights for `pallet_api::fungibles`
//!
//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 33.0.0
//! DATE: 2024-07-24, STEPS: `20`, REPEAT: `5`, LOW RANGE: `[]`, HIGH RANGE: `[]`
//! DATE: 2024-07-25, STEPS: `20`, REPEAT: `5`, LOW RANGE: `[]`, HIGH RANGE: `[]`
//! WORST CASE MAP SIZE: `1000000`
//! HOSTNAME: `R0GUE`, CPU: `<UNKNOWN>`
//! WASM-EXECUTION: `Compiled`, CHAIN: `Some("dev")`, DB CACHE: `1024`
Expand Down Expand Up @@ -31,7 +32,7 @@ use core::marker::PhantomData;

/// Weight functions needed for `pallet_api::fungibles`.
pub trait WeightInfo {
fn approve() -> Weight;
fn approve(a: u32, c: u32, ) -> Weight;
}

/// Weights for `pallet_api::fungibles` using the Substrate node and recommended hardware.
Expand All @@ -43,14 +44,23 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
/// Proof: `Assets::Asset` (`max_values`: None, `max_size`: Some(210), added: 2685, mode: `MaxEncodedLen`)
/// Storage: `System::Account` (r:1 w:1)
/// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`)
fn approve() -> Weight {
/// The range of component `a` is `[0, 1]`.
/// The range of component `c` is `[0, 1]`.
fn approve(a: u32, c: u32, ) -> Weight {
// Proof Size summary in bytes:
// Measured: `515`
// Estimated: `3675`
// Minimum execution time: 53_000_000 picoseconds.
Weight::from_parts(54_000_000, 3675)
.saturating_add(T::DbWeight::get().reads(3_u64))
.saturating_add(T::DbWeight::get().writes(3_u64))
// Measured: `413 + c * (102 ±0)`
// Estimated: `3675 + c * (1797 ±0)`
// Minimum execution time: 35_000_000 picoseconds.
Weight::from_parts(1_207_482, 3675)
// Standard Error: 948_955
.saturating_add(Weight::from_parts(34_649_659, 0).saturating_mul(a.into()))
// Standard Error: 948_955
.saturating_add(Weight::from_parts(56_976_190, 0).saturating_mul(c.into()))
.saturating_add(T::DbWeight::get().reads(2_u64))
.saturating_add(T::DbWeight::get().reads((1_u64).saturating_mul(c.into())))
.saturating_add(T::DbWeight::get().writes(2_u64))
.saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(c.into())))
.saturating_add(Weight::from_parts(0, 1797).saturating_mul(c.into()))
}
}

Expand All @@ -62,14 +72,23 @@ impl WeightInfo for () {
/// Proof: `Assets::Asset` (`max_values`: None, `max_size`: Some(210), added: 2685, mode: `MaxEncodedLen`)
/// Storage: `System::Account` (r:1 w:1)
/// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`)
fn approve() -> Weight {
/// The range of component `a` is `[0, 1]`.
/// The range of component `c` is `[0, 1]`.
fn approve(a: u32, c: u32, ) -> Weight {
// Proof Size summary in bytes:
// Measured: `515`
// Estimated: `3675`
// Minimum execution time: 53_000_000 picoseconds.
Weight::from_parts(54_000_000, 3675)
.saturating_add(RocksDbWeight::get().reads(3_u64))
.saturating_add(RocksDbWeight::get().writes(3_u64))
// Measured: `413 + c * (102 ±0)`
// Estimated: `3675 + c * (1797 ±0)`
// Minimum execution time: 35_000_000 picoseconds.
Weight::from_parts(1_207_482, 3675)
// Standard Error: 948_955
.saturating_add(Weight::from_parts(34_649_659, 0).saturating_mul(a.into()))
// Standard Error: 948_955
.saturating_add(Weight::from_parts(56_976_190, 0).saturating_mul(c.into()))
.saturating_add(RocksDbWeight::get().reads(2_u64))
.saturating_add(RocksDbWeight::get().reads((1_u64).saturating_mul(c.into())))
.saturating_add(RocksDbWeight::get().writes(2_u64))
.saturating_add(RocksDbWeight::get().writes((1_u64).saturating_mul(c.into())))
.saturating_add(Weight::from_parts(0, 1797).saturating_mul(c.into()))
}
}

0 comments on commit f572fee

Please sign in to comment.