Skip to content

Fix benchmarks of saft-registry #327

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

Merged
clearloop merged 13 commits into from
Sep 3, 2021
+196 −91
Merged

Fix benchmarks of saft-registry #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 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
ecebf46
feat(saft-registry): adds trait AssetRecorderBenchmarks
clearloop Sep 3, 2021
0e5f69f
feat(saft-registry): fix the benchmarks
clearloop Sep 3, 2021
92bc7f9
feat(saft-registry): fix tests
clearloop Sep 3, 2021
978c1a5
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 )
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));
// }: { call.dispatch_bypass_filter(origin.clone())? } verify {
// assert_eq!(T::IndexToken::total_balance(&origin_account_id), 0u32.into());
// }
Comment on lines +122 to +152
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.

is this blocked by something?

Copy link
Contributor Author

@clearloop clearloop Sep 3, 2021

Choose a reason for hiding this comment

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

This requires calls in remote-asset-manager, got InvalidDest error of xcm

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.

ah yes, you're right, the XCM dependent calls can't be benchmarked like that. voting to even remove them.


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(|| {
assert_ok!(Pallet::<Test>::test_benchmark_unlock());
});
}
Expand Down
25 changes: 22 additions & 3 deletions pallets/asset-index/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ pub mod types;
// this is requires as the #[pallet::event] proc macro generates code that violates this lint
#[allow(clippy::unused_unit, clippy::large_enum_variant, clippy::type_complexity)]
pub mod pallet {
#[cfg(feature = "runtime-benchmarks")]
use pallet_price_feed::PriceFeedBenchmarks;
#[cfg(feature = "runtime-benchmarks")]
use primitives::traits::AssetRecorderBenchmarks;

use frame_support::{
dispatch::DispatchResultWithPostInfo,
pallet_prelude::*,
Expand All @@ -44,8 +49,6 @@ pub mod pallet {
use sp_core::U256;
use xcm::v0::MultiLocation;

#[cfg(feature = "runtime-benchmarks")]
use pallet_price_feed::PriceFeedBenchmarks;
use pallet_price_feed::{AssetPricePair, Price, PriceFeed};
use primitives::{
fee::{BaseFee, FeeRate},
Expand Down Expand Up @@ -483,7 +486,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 @@ -1037,6 +1039,21 @@ pub mod pallet {
}
}

#[cfg(feature = "runtime-benchmarks")]
impl<T: Config> AssetRecorderBenchmarks<T::AssetId, T::Balance> for Pallet<T> {
fn add_asset(
asset_id: T::AssetId,
units: T::Balance,
location: MultiLocation,
amount: T::Balance,
) -> DispatchResultWithPostInfo {
let origin = T::AdminOrigin::successful_origin();
let origin_account_id = T::AdminOrigin::ensure_origin(origin.clone()).unwrap();
T::PriceFeedBenchmarks::create_feed(origin_account_id, asset_id)?;
Self::add_asset(T::AdminOrigin::successful_origin(), asset_id, units, location, amount)
}
}

impl<T: Config> MultiAssetRegistry<T::AssetId> for Pallet<T> {
fn native_asset_location(asset: &T::AssetId) -> Option<MultiLocation> {
Assets::<T>::get(asset).and_then(|availability| {
Expand Down Expand Up @@ -1168,9 +1185,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
12 changes: 12 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 @@ -136,6 +136,8 @@ impl orml_tokens::Config for Test {
}

impl pallet_saft_registry::Config for Test {
#[cfg(feature = "runtime-benchmarks")]
type AssetRecorderBenchmarks = AssetIndex;
type AdminOrigin = frame_system::EnsureSignedBy<AdminAccountId, AccountId>;
type Event = Event;
type Balance = Balance;
Expand Down Expand Up @@ -298,3 +300,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
}
Loading