Skip to content

Fix benchmarks of pallet-asset-index #323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
clearloop wants to merge 10 commits into from
Closed

Fix benchmarks of pallet-asset-index #323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
23c554f
chore(origins): try keep EnsureOneOf
clearloop Sep 1, 2021
0adac26
feat(asset-index): use committee origin for pallet-index
clearloop Sep 2, 2021
45d51ba
feat(saft-registry): use committee origin in saft-registry
clearloop Sep 2, 2021
63fcac7
chore(common): add docs to origins in runtime-common
clearloop Sep 2, 2021
a2ac59a
feat(asset-index): fix benchmarks without remove_asset
clearloop Sep 2, 2021
e0da53c
Merge branch 'main' into cl/chain-spec
clearloop Sep 2, 2021
b860003
feat(asset-index): supports new_test_ext_from_genesis
clearloop Sep 2, 2021
498e018
feat(asset-index): fix benchmarks
clearloop Sep 2, 2021
eab04eb
feat(asset-index): adapt tests and benchmarks for deposit call
clearloop Sep 2, 2021
ac50e8e
feat(asset-index): move back currency deposit call in deposit
clearloop Sep 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions node/src/chain_spec/dev.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ fn pint_testnet_genesis(
code: WASM_BINARY.expect("WASM binary was not build, please build it!").to_vec(),
changes_trie_config: Default::default(),
},
balances: BalancesConfig { balances: endowed_accounts.iter().cloned().map(|k| (k, 1 << 60)).collect() },
balances: BalancesConfig { balances: endowed_accounts.iter().cloned().map(|k| (k, 1 << 12)).collect() },
committee: CommitteeConfig { council_members: council_members.clone(), ..Default::default() },
chainlink_feed: ChainlinkFeedConfig { pallet_admin: Some(root_key.clone()), feed_creators: council_members },
sudo: SudoConfig { key: root_key },
Expand Down Expand Up @@ -139,8 +139,8 @@ fn pint_testnet_genesis(
//
// this config is only for tests for now
balances: vec![
endowed_accounts.iter().cloned().map(|k| (k, 42, 1 << 60)).collect::<Vec<_>>(),
endowed_accounts.iter().cloned().map(|k| (k, 43, 1 << 60)).collect::<Vec<_>>(),
endowed_accounts.iter().cloned().map(|k| (k, 2, 1 << 12)).collect::<Vec<_>>(),
endowed_accounts.iter().cloned().map(|k| (k, 3, 1 << 12)).collect::<Vec<_>>(),
]
.concat(),
},
Expand Down
170 changes: 91 additions & 79 deletions pallets/asset-index/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@

#![cfg(feature = "runtime-benchmarks")]

use frame_benchmarking::{account, benchmarks, vec};
use frame_benchmarking::{benchmarks, vec};
use frame_support::{
assert_ok,
dispatch::UnfilteredDispatchable,
sp_runtime::{traits::AccountIdConversion, FixedPointNumber},
traits::{Currency as _, EnsureOrigin, Get},
traits::{EnsureOrigin, Get},
};
use frame_system::RawOrigin;
use orml_traits::MultiCurrency;
use pallet_price_feed::{PriceFeed, PriceFeedBenchmarks};
use primitives::{traits::NavProvider, AssetAvailability};
Expand All @@ -20,18 +19,6 @@ use crate::Pallet as AssetIndex;

use super::*;

const SEED: u32 = 0;

fn whitelisted_account<T: Config>(name: &'static str, counter: u32) -> T::AccountId {
let acc = account(name, counter, SEED);
whitelist_acc::<T>(&acc);
acc
}

fn whitelist_acc<T: Config>(acc: &T::AccountId) {
frame_benchmarking::benchmarking::add_to_whitelist(frame_system::Account::<T>::hashed_key_for(acc).into());
}

benchmarks! {
add_asset {
let asset_id :T::AssetId = T::try_convert(2u8).unwrap();
Expand Down Expand Up @@ -60,22 +47,25 @@ benchmarks! {
let asset_id : T::AssetId = T::try_convert(2u8).unwrap();
let units = 100_u32.into();
let tokens = 500_u32.into();
let admin = T::AdminOrigin::successful_origin();
let origin = whitelisted_account::<T>("origin", 0);
let origin = T::AdminOrigin::successful_origin();
let origin_account_id = T::AdminOrigin::ensure_origin(origin.clone()).unwrap();
let deposit_units = 1000_u32.into();

// create liquid assets
assert_ok!(<AssetIndex<T>>::add_asset(
admin,
origin.clone(),
asset_id,
units,
MultiLocation::Null,
tokens
));

// create price feed
T::PriceFeedBenchmarks::create_feed(origin_account_id.clone(), asset_id).unwrap();

// deposit some funds into the index from an user account
assert_ok!(T::Currency::deposit(asset_id, &origin, deposit_units));
assert_ok!(<AssetIndex<T>>::deposit(RawOrigin::Signed(origin.clone()).into(), asset_id, deposit_units));
assert_ok!(T::Currency::deposit(asset_id, &origin_account_id, deposit_units));
assert_ok!(<AssetIndex<T>>::deposit(origin.clone(), asset_id, deposit_units));

// advance the block number so that the lock expires
<frame_system::Pallet<T>>::set_block_number(
Expand All @@ -86,20 +76,19 @@ benchmarks! {

// start withdraw
assert_ok!(<AssetIndex<T>>::withdraw(
RawOrigin::Signed(origin.clone()).into(),
origin.clone(),
42_u32.into(),
));
}: _(
RawOrigin::Signed(origin.clone())
) verify {
assert_eq!(pallet::PendingWithdrawals::<T>::get(&origin), None);
let call = Call::<T>::complete_withdraw();
}: { call.dispatch_bypass_filter(origin)? } verify {
assert_eq!(pallet::PendingWithdrawals::<T>::get(&origin_account_id), None);
}

deposit {
let asset_id : T::AssetId = T::try_convert(2u8).unwrap();
let asset_id = T::try_convert(2u8).unwrap();
let origin = T::AdminOrigin::successful_origin();
let origin_account_id = T::AdminOrigin::ensure_origin(origin.clone()).unwrap();
let admin_deposit = 5u32;
let admin_deposit = 1_000_000u32;
let units = 1_000u32.into();

assert_ok!(AssetIndex::<T>::add_asset(
Expand All @@ -110,39 +99,57 @@ benchmarks! {
admin_deposit.into(),
));

let index_tokens = AssetIndex::<T>::index_token_balance(&origin_account_id).into();
T::PriceFeedBenchmarks::create_feed(origin_account_id.clone(), asset_id).unwrap();
assert_ok!(T::Currency::deposit(asset_id, &origin_account_id, units));

// construct call
let call = Call::<T>::deposit(asset_id, units);
}: { call.dispatch_bypass_filter(origin)? } verify {
let nav = AssetIndex::<T>::nav().unwrap();
let deposit_value = T::PriceFeed::get_price(asset_id).unwrap().checked_mul_int(units.into()).unwrap();
let received = nav.reciprocal().unwrap().saturating_mul_int(deposit_value).saturating_add(1u128);
assert_eq!(AssetIndex::<T>::index_token_balance(&origin_account_id).into(), received + admin_deposit as u128);
let received = nav.reciprocal().unwrap().saturating_mul_int(deposit_value);

// NOTE:
//
// the result will be 0 or 1
//
// - 0 for tests
// - 1 for benchmarks ( transaction fee )
Comment on lines +113 to +118
Copy link
Contributor

@mattsse mattsse Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is rather confusing and depends on the configured fees,
can we simplify this check somehow, by checking that fees are sent to the treasury?
or remove it, this selective check does probably more harm than good

assert!(AssetIndex::<T>::index_token_balance(&origin_account_id).into() - (index_tokens + received) < 2);
}

remove_asset {
let asset_id :T::AssetId = T::try_convert(2u8).unwrap();
let origin = T::AdminOrigin::successful_origin();
let units: u32 = 100;
let amount = 500u32.into();

assert_ok!(AssetIndex::<T>::add_asset(
origin.clone(),
asset_id,
units.into(),
MultiLocation::Null,
amount,
));

// ensure
assert_eq!(T::IndexToken::total_balance(&Default::default()), 500u32.into());

// construct remove call
let call = Call::<T>::remove_asset(asset_id, units.into(), None);
}: { call.dispatch_bypass_filter(origin.clone())? } verify {
assert_eq!(T::IndexToken::total_balance(&Default::default()), 0u32.into());
}
// TODO:
//
// This extrinsic requires `remote-asset-manager`
//
// ----
//
// remove_asset {
// let asset_id = T::try_convert(2u8).unwrap();
// let units = 100_u32.into();
// let amount = 1_000u32.into();
// let origin = T::AdminOrigin::successful_origin();
// let origin_account_id = T::AdminOrigin::ensure_origin(origin.clone()).unwrap();
// let receiver = whitelisted_account::<T>("receiver", 0);
//
// // create liquid assets
// assert_ok!(<AssetIndex<T>>::add_asset(
// origin.clone(),
// asset_id,
// units,
// MultiLocation::Null,
// amount
// ));
//
// // create price feed
// T::PriceFeedBenchmarks::create_feed(origin_account_id.clone(), asset_id).unwrap();
//
// // construct call
// let call = Call::<T>::remove_asset(asset_id, units, Some(receiver));
Comment on lines +122 to +149
Copy link
Contributor

@mattsse mattsse Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch with all the xcm dependent calls.
They can't be benchmarked like that in any case so we can remove them.
the weights for these calls will come from the upcoming xcm benchmarking suite in polkadot.

// }: { call.dispatch_bypass_filter(origin.clone())? } verify {
// assert_eq!(T::IndexToken::total_balance(&origin_account_id), 0u32.into());
// }

register_asset {
let asset_id :T::AssetId = T::try_convert(2u8).unwrap();
Expand Down Expand Up @@ -182,52 +189,57 @@ benchmarks! {
let asset_id :T::AssetId = T::try_convert(2u8).unwrap();
let units = 100_u32.into();
let tokens = 500_u32.into();
let admin = T::AdminOrigin::successful_origin();
let origin = whitelisted_account::<T>("origin", 0);
let origin = T::AdminOrigin::successful_origin();
let origin_account_id = T::AdminOrigin::ensure_origin(origin.clone()).unwrap();
let deposit_units = 1_000_u32.into();

// create liquid assets
assert_ok!(<AssetIndex<T>>::add_asset(
admin,
origin.clone(),
asset_id,
units,
MultiLocation::Null,
tokens
));

// create price feed
T::PriceFeedBenchmarks::create_feed(origin_account_id.clone(), asset_id).unwrap();

// deposit some funds into the index from an user account
assert_ok!(T::Currency::deposit(asset_id, &origin, deposit_units));
assert_ok!(<AssetIndex<T>>::deposit(RawOrigin::Signed(origin.clone()).into(), asset_id, deposit_units));
assert_ok!(T::Currency::deposit(asset_id, &origin_account_id, deposit_units));
assert_ok!(<AssetIndex<T>>::deposit(origin.clone(), asset_id, deposit_units));

// advance the block number so that the lock expires
<frame_system::Pallet<T>>::set_block_number(
<frame_system::Pallet<T>>::block_number()
+ T::LockupPeriod::get()
+ 1_u32.into(),
);
}: _(
RawOrigin::Signed(origin.clone()),
42_u32.into()
) verify {
assert_eq!(pallet::PendingWithdrawals::<T>::get(&origin).expect("pending withdrawals should be present").len(), 1);

let call = Call::<T>::withdraw(42_u32.into());
}: { call.dispatch_bypass_filter(origin)? } verify {
assert_eq!(pallet::PendingWithdrawals::<T>::get(&origin_account_id).expect("pending withdrawals should be present").len(), 1);
}

unlock {
let asset_id :T::AssetId = T::try_convert(2u8).unwrap();
let origin = T::AdminOrigin::successful_origin();
let depositor = whitelisted_account::<T>("depositor", 0);
let origin_account_id = T::AdminOrigin::ensure_origin(origin.clone()).unwrap();
let amount = 500u32.into();
let units = 100u32.into();

assert_ok!(AssetIndex::<T>::add_asset(origin, asset_id, units, MultiLocation::Null, amount));
assert_ok!(T::Currency::deposit(asset_id, &depositor, units));
assert_ok!(<AssetIndex<T>>::deposit(RawOrigin::Signed(depositor.clone()).into(), asset_id, units));
}: _(
RawOrigin::Signed(depositor.clone())
) verify {
assert_eq!(<pallet::IndexTokenLocks<T>>::get(&depositor), vec![types::IndexTokenLock{
locked: 500_u32.into(),
end_block: <frame_system::Pallet<T>>::block_number() + T::LockupPeriod::get(),
// create price feed
T::PriceFeedBenchmarks::create_feed(origin_account_id.clone(), asset_id).unwrap();

assert_ok!(AssetIndex::<T>::add_asset(origin.clone(), asset_id, units, MultiLocation::Null, amount));
assert_ok!(T::Currency::deposit(asset_id, &origin_account_id, units));
assert_ok!(<AssetIndex<T>>::deposit(origin.clone(), asset_id, units));

let call = Call::<T>::unlock();
}: { call.dispatch_bypass_filter(origin)? } verify {
assert_eq!(<pallet::IndexTokenLocks<T>>::get(&origin_account_id), vec![types::IndexTokenLock{
locked: <AssetIndex<T>>::index_token_equivalent(asset_id, units).unwrap(),
end_block: <frame_system::Pallet<T>>::block_number() + T::LockupPeriod::get() - 1u32.into()
}]);
}
}
Expand All @@ -236,7 +248,7 @@ benchmarks! {
mod tests {
use frame_support::assert_ok;

use crate::mock::{new_test_ext, Test};
use crate::mock::{new_test_ext, new_test_ext_from_genesis, Test};

use super::*;

Expand Down Expand Up @@ -275,16 +287,16 @@ mod tests {
});
}

#[test]
fn remove_asset() {
new_test_ext().execute_with(|| {
assert_ok!(Pallet::<Test>::test_benchmark_remove_asset());
});
}
// #[test]
// fn remove_asset() {
// new_test_ext().execute_with(|| {
// assert_ok!(Pallet::<Test>::test_benchmark_remove_asset());
// });
// }

#[test]
fn unlock() {
new_test_ext().execute_with(|| {
new_test_ext_from_genesis().execute_with(|| {
Copy link
Contributor Author

@clearloop clearloop Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs this to async with benchmarks

assert_ok!(Pallet::<Test>::test_benchmark_unlock());
});
}
Expand Down
3 changes: 2 additions & 1 deletion pallets/asset-index/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,6 @@ pub mod pallet {

// the amount of index token the given units of the liquid assets are worth
let index_tokens = Self::index_token_equivalent(asset_id, units)?;

if index_tokens.is_zero() {
return Err(Error::<T>::InsufficientDeposit.into());
}
Expand Down Expand Up @@ -1168,9 +1167,11 @@ pub mod pallet {
if total_issuance.is_zero() {
return Ok(Ratio::zero());
}

Assets::<T>::iter().try_fold(Ratio::zero(), |nav, (asset, availability)| -> Result<_, DispatchError> {
let value =
if availability.is_liquid() { Self::net_liquid_value(asset)? } else { Self::net_saft_value(asset) };

let proportion = Ratio::checked_from_rational(value.into(), total_issuance.into())
.ok_or(ArithmeticError::Overflow)?;
Ok(nav.checked_add(&proportion).ok_or(ArithmeticError::Overflow)?)
Expand Down
10 changes: 10 additions & 0 deletions pallets/asset-index/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,3 +298,13 @@ pub fn new_test_ext_with_balance(balances: Vec<(AccountId, AssetId, Balance)>) -

ext
}

pub fn new_test_ext_from_genesis() -> sp_io::TestExternalities {
let ext = ExtBuilder::default().build();
MockPriceFeed::set_prices(vec![
(ASSET_A_ID, Price::from(ASSET_A_PRICE_MULTIPLIER)),
(ASSET_B_ID, Price::from(ASSET_B_PRICE_MULTIPLIER)),
]);

ext
}