Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 9 additions & 13 deletions pallets/pallet-bonded-coins/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -355,25 +355,18 @@ mod benchmarks {
}

#[benchmark]
fn reset_team() {
fn reset_team(c: Linear<1, { T::MaxCurrenciesPerPool::get() }>) {
let origin = T::DefaultOrigin::try_successful_origin().expect("creating origin should not fail");
let account_origin = origin
.clone()
.into_signer()
.expect("generating account_id from origin should not fail");
make_free_for_deposit::<T>(&account_origin);

let bonded_coin_id = T::BenchmarkHelper::calculate_bonded_asset_id(0);
create_bonded_asset::<T>(bonded_coin_id.clone());
let bonded_currencies = create_bonded_currencies_in_range::<T>(c, false);

let curve = get_linear_bonding_curve::<CurveParameterTypeOf<T>>();
let pool_id = create_pool::<T>(
curve,
[bonded_coin_id.clone()].to_vec(),
Some(account_origin),
None,
None,
);
let pool_id = create_pool::<T>(curve, bonded_currencies.clone(), Some(account_origin), None, None);

let admin: AccountIdOf<T> = account("admin", 0, 0);
let freezer: AccountIdOf<T> = account("freezer", 0, 0);
Expand All @@ -382,12 +375,15 @@ mod benchmarks {
freezer: freezer.clone(),
};

let max_currencies = T::MaxCurrenciesPerPool::get();
#[extrinsic_call]
_(origin as T::RuntimeOrigin, pool_id, fungibles_team, 0);
_(origin as T::RuntimeOrigin, pool_id, fungibles_team, max_currencies);

// Verify
assert_eq!(T::Fungibles::admin(bonded_coin_id.clone()), Some(admin));
assert_eq!(T::Fungibles::freezer(bonded_coin_id), Some(freezer));
bonded_currencies.iter().for_each(|asset_id| {
assert_eq!(T::Fungibles::admin(asset_id.clone()), Some(admin.clone()));
assert_eq!(T::Fungibles::freezer(asset_id.clone()), Some(freezer.clone()));
});
}

#[benchmark]
Expand Down
41 changes: 20 additions & 21 deletions pallets/pallet-bonded-coins/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -449,18 +449,17 @@ pub mod pallet {
Ok(())
}

/// Changes the managing team of a bonded currency which is issued by
/// this pool. The new team will be set to the provided team. The
/// currency index is used to select the currency that the team will
/// manage. The origin account must be a manager of the pool.
/// Changes the managing team of all bonded currencies issued by this
/// pool, setting it to the provided `team`. The origin account must be
/// a manager of the pool.
///
/// # Parameters
/// - `origin`: The origin of the call, requiring the caller to be a
/// manager of the pool.
/// - `pool_id`: The identifier of the pool.
/// - `team`: The new managing team.
/// - `currency_idx`: The index of the currency in the bonded currencies
/// vector.
/// - `currency_count`: The number of bonded currencies vector linked to
/// the pool. Required for weight estimations.
///
/// # Returns
/// - `DispatchResult`: The result of the dispatch.
Expand All @@ -469,40 +468,40 @@ pub mod pallet {
/// - `Error::<T>::PoolUnknown`: If the pool does not exist.
/// - `Error::<T>::NoPermission`: If the caller is not a manager of the
/// pool.
/// - `Error::<T>::IndexOutOfBounds`: If the currency index is out of
/// bounds.
/// - `Error::<T>::CurrencyCount`: If the actual number of currencies in
/// the pool is larger than `currency_count`.
/// - Other errors depending on the types in the config.
#[pallet::call_index(1)]
#[pallet::weight(T::WeightInfo::reset_team())]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
#[pallet::weight(T::WeightInfo::reset_team())]
#[pallet::weight(T::WeightInfo::reset_team(currency_count.to_owned()))]

I'm realising this needs to be changed, but in order for this to work it seems that I'd have to re-run benchmarks? I'll definitely need guidance for that, I have never done this before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is correct. I think it would make sense to tackle benchmarks in a separate PR tho at the end of all changes, since benchmarking itself has some "fixed costs" which we could lower if we were to batch all changes into one benchmarking operation.

Copy link
Contributor

Choose a reason for hiding this comment

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

But in general, what you would need to do, is to pre-populate the storage with as many pools as the theoretical maximum, and then use the range of possible inputs to pass to the extrinsic. You can see an example of it for the create_pool_polynomial benchmark here:

fn create_pool_polynomial(c: Linear<1, { T::MaxCurrenciesPerPool::get() }>) {
. So I think it's sufficient to pre-populate the storage with N assets where N is the benchmark input, and then simply call reset_team(N). That should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand, 701410c already makes changes in line with the snippet you linked to, but I still can't make the change I suggested in #869 (comment) because default_weights.rs has not been updated (the compiler complains that this function takes 0 arguments but 1 argument was supplied). Should I just manually mess around in the default_weights.rs for now until it compiles, even though it's an autogenerated file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just leave the previous weight for now. Once the new benchmarks are run, the pallet will complain that it now requires one parameter, and then you can fill in the result of the benchmarks. Sorry I did not see the commit you previously linked. But yeah, just leave the old weight for the extrinsic. As soon as the new benchmarks are run, the code won't compile and you will update that. As a temporary solution, you COULD also update the weight file, since it will be overwritten anyway upon new benchmarks, but I would not recommend that.

pub fn reset_team(
origin: OriginFor<T>,
pool_id: T::PoolId,
team: PoolManagingTeam<AccountIdOf<T>>,
currency_idx: u32,
currency_count: u32,
) -> DispatchResult {
let who = T::DefaultOrigin::ensure_origin(origin)?;

let pool_details = Pools::<T>::get(&pool_id).ok_or(Error::<T>::PoolUnknown)?;

let number_of_currencies = Self::get_currencies_number(&pool_details);
ensure!(number_of_currencies <= currency_count, Error::<T>::CurrencyCount);

ensure!(pool_details.is_manager(&who), Error::<T>::NoPermission);
ensure!(pool_details.state.is_live(), Error::<T>::PoolNotLive);

let asset_id = pool_details
.bonded_currencies
.get(currency_idx.saturated_into::<usize>())
.ok_or(Error::<T>::IndexOutOfBounds)?;

let pool_id_account = pool_id.into();

let PoolManagingTeam { freezer, admin } = team;

T::Fungibles::reset_team(
asset_id.to_owned(),
pool_id_account.clone(),
admin,
pool_id_account,
freezer,
)
pool_details.bonded_currencies.into_iter().try_for_each(|asset_id| {
T::Fungibles::reset_team(
asset_id,
pool_id_account.clone(),
admin.clone(),
pool_id_account.clone(),
freezer.clone(),
)
})
}

/// Resets the manager of a pool. The new manager will be set to the
Expand Down
58 changes: 51 additions & 7 deletions pallets/pallet-bonded-coins/src/tests/transactions/reset_team.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ fn resets_team() {
admin: ACCOUNT_00,
freezer: ACCOUNT_01,
},
0
1
));

assert_eq!(Assets::admin(DEFAULT_BONDED_CURRENCY_ID), Some(ACCOUNT_00));
Expand All @@ -61,6 +61,50 @@ fn resets_team() {
})
}

#[test]
fn resets_team_for_all() {
let currencies = vec![DEFAULT_BONDED_CURRENCY_ID, DEFAULT_BONDED_CURRENCY_ID + 1];

let pool_details = generate_pool_details(
currencies.clone(),
get_linear_bonding_curve(),
false,
Some(PoolStatus::Active),
Some(ACCOUNT_00),
None,
None,
None,
);
let pool_id: AccountIdOf<Test> = calculate_pool_id(&currencies);

ExtBuilder::default()
.with_pools(vec![(pool_id.clone(), pool_details)])
.with_collaterals(vec![DEFAULT_COLLATERAL_CURRENCY_ID])
.build_and_execute_with_sanity_tests(|| {
let manager_origin = RawOrigin::Signed(ACCOUNT_00).into();

assert_ok!(BondingPallet::reset_team(
manager_origin,
pool_id.clone(),
PoolManagingTeam {
admin: ACCOUNT_00,
freezer: ACCOUNT_01,
},
2
));

assert_eq!(Assets::admin(currencies[0]), Some(ACCOUNT_00));
assert_eq!(Assets::freezer(currencies[0]), Some(ACCOUNT_01));
assert_eq!(Assets::owner(currencies[0]), Some(pool_id.clone()));
assert_eq!(Assets::issuer(currencies[0]), Some(pool_id.clone()));

assert_eq!(Assets::admin(currencies[1]), Some(ACCOUNT_00));
assert_eq!(Assets::freezer(currencies[1]), Some(ACCOUNT_01));
assert_eq!(Assets::owner(currencies[1]), Some(pool_id.clone()));
assert_eq!(Assets::issuer(currencies[1]), Some(pool_id));
})
}

#[test]
fn does_not_change_team_when_not_live() {
let pool_details = generate_pool_details(
Expand Down Expand Up @@ -89,7 +133,7 @@ fn does_not_change_team_when_not_live() {
admin: ACCOUNT_00,
freezer: ACCOUNT_00,
},
0
1
),
BondingPalletErrors::<Test>::PoolNotLive
);
Expand Down Expand Up @@ -130,7 +174,7 @@ fn only_manager_can_change_team() {
admin: ACCOUNT_00,
freezer: ACCOUNT_00,
},
0
1
),
BondingPalletErrors::<Test>::NoPermission
);
Expand All @@ -143,7 +187,7 @@ fn only_manager_can_change_team() {
admin: ACCOUNT_00,
freezer: ACCOUNT_00,
},
0
1
),
BondingPalletErrors::<Test>::NoPermission
);
Expand All @@ -153,7 +197,7 @@ fn only_manager_can_change_team() {
}

#[test]
fn handles_currency_idx_out_of_bounds() {
fn handles_currency_number_incorrect() {
let pool_details = generate_pool_details(
vec![DEFAULT_BONDED_CURRENCY_ID],
get_linear_bonding_curve(),
Expand All @@ -180,9 +224,9 @@ fn handles_currency_idx_out_of_bounds() {
admin: ACCOUNT_00,
freezer: ACCOUNT_00,
},
2
0
),
BondingPalletErrors::<Test>::IndexOutOfBounds
BondingPalletErrors::<Test>::CurrencyCount
);
})
}
Loading