Multisig: Fix tests and re-introduce reserve logic #12241

ruseinov · 2022-09-12T12:47:23Z

fixes for #12072

frame/multisig/src/benchmarking.rs

kianenigma · 2022-09-14T12:30:11Z

frame/multisig/src/lib.rs

@@ -603,6 +604,8 @@ impl<T: Config> Pallet<T> {
 			// Just start the operation by recording it in storage.
 			let deposit = T::DepositBase::get() + T::DepositFactor::get() * threshold.into();

+			T::Currency::reserve(&who, deposit)?;


only logical change of this PR, which seems correct to me.

kianenigma · 2022-09-14T12:32:26Z

frame/multisig/src/tests.rs

 			call_weight
 		));
 		assert_eq!(Balances::free_balance(1), 5);
 		assert_eq!(Balances::reserved_balance(1), 0);
 	});
 }

-#[test]
-fn multisig_deposit_is_taken_and_returned_with_call_storage() {


IIRC, there is still a fixed deposit that is collected, which you brought back in this PR.

Do we still have tests for this. Should we keep this test as multisig_deposit_is_taken_and_returned? or do we already have something like that?

yeah, this is tested in another scenario afaik, I will double-check

substrate/frame/multisig/src/tests.rs

Line 137 in 7bacd73

fn multisig_deposit_is_taken_and_returned() {

exactly the same test without storage

kianenigma · 2022-09-14T12:33:09Z

frame/multisig/src/tests.rs

 			Error::<Test>::WrongTimepoint,
 		);
 	});
 }

-#[test]
-fn multisig_2_of_3_works_with_call_storing() {


similarly, should this test scenario remain, just without call storing?

AFAIK this already exists in another test.

exactly the same test with no storage

substrate/frame/multisig/src/tests.rs

Line 251 in 7bacd73

fn multisig_2_of_3_works() {

kianenigma

Looks mostly good to me, some tests might need to come back again.

gavofyork

Tests which are now just duplicates of other tests can be removed. Looks good otherwise.

gavofyork

Good other than above comment

ruseinov · 2022-09-16T12:24:38Z

Tests which are now just duplicates of other tests can be removed. Looks good otherwise.

@gavofyork I have removed all the storage tests the duplicated normal ones. The stuff that @kianenigma was pointing out exists in a non-storage shape only.

If I'm missing something - please point me to it, looking at the tests now I can't see any redundant or missing ones.

ruseinov · 2022-09-16T13:26:24Z

Merging this into the original branch.
Migration will come in a follow-up pr.

ruseinov · 2022-09-16T13:26:29Z

bot merge

paritytech-processbot · 2022-09-16T13:26:36Z

Error: Checks failed for 42acac2

* Remove multisig's Calls * Multisig: Fix tests and re-introduce reserve logic (#12241) * Fix tests and re-introduce reserve logic * fix benches * add todo * remove irrelevant bench * [Feature] Add a migration that drains and refunds stored calls (#12313) * [Feature] Add a migration that drains and refunds stored calls * migration fixes * fixes * address review comments * consume the whole block weight * fix assertions * license header * fix interface Co-authored-by: parity-processbot <> Co-authored-by: parity-processbot <> Co-authored-by: Roman Useinov <[email protected]>

* Introduce preimages module in traits * Multisize Preimages * Len not actually necessary * Tweaks to the preimage API * Fixes * Get Scheduler building with new API * Scheduler tests pass * Bounded Scheduler 🎉 * Use Agenda holes and introduce IncompleteSince to avoid need to reschedule * Tests pass with new weight system * New benchmarks * Add missing file * Drop preimage when permenantly overeight * Drop preimage when permenantly overeight * Referenda uses latest preimage API * Testing ok * Adding tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * fmt Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add preimage migration Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Docs * Remove dbg * Refactor Democracy * Refactor Democracy * Add final MEL * Remove silly maps * Fixes * Minor refactor * Formatting * Fixes * Fixes * Fixes * Update frame/preimage/src/lib.rs Co-authored-by: Shawn Tabrizi <[email protected]> * Add migrations to Democracy * WIP Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Resolve conflicts Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Revert "Resolve conflicts" This reverts commit a89cd0a. * Undo wrong resolves... Signed-off-by: Oliver Tale-Yazdi <[email protected]> * WIP Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Make compile Signed-off-by: Oliver Tale-Yazdi <[email protected]> * massage clippy Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More clippy Signed-off-by: Oliver Tale-Yazdi <[email protected]> * clippy annoyance Signed-off-by: Oliver Tale-Yazdi <[email protected]> * clippy annoyance Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix benchmarks Signed-off-by: Oliver Tale-Yazdi <[email protected]> * add missing file * Test <Preimage as QueryPreimage> Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Clippy harassment Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add test Signed-off-by: Oliver Tale-Yazdi <[email protected]> * clippy Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fixup tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Remove old stuff Signed-off-by: Oliver Tale-Yazdi <[email protected]> * fmt Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Test <Scheduler as Anon> trait functions Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Update pallet-ui tests Why is this needed? Should not be the case unless master is broken... Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More scheduler trait test Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Apply review suggestion Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Beauty fixes Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add Scheduler test migration_v3_to_v4_works Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Merge fixup Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Keep referenda benchmarks instantiatable Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Update weights Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Use new scheduler weight functions Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Use new democracy weight functions Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Use weight compare functions Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Update pallet-ui tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More renaming… Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More renaming… Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add comment Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Implement OnRuntimeUpgrade for scheduler::v3_to_v4 migration Put the migration into a proper `MigrateToV4` struct and implement the OnRuntimeUpgrade hooks for it. Also move the test to use that instead. This should make it easier for adding it to Polkadot. Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Clippy Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Handle undecodable Agendas Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Remove trash Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix test Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Use new OnRuntimeUpgrade functions Signed-off-by: Oliver Tale-Yazdi <[email protected]> * fix test Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix BoundedSlice::truncate_from Co-authored-by: jakoblell Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix pre_upgrade hook return values Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add more error logging Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Find too large preimages in the pre_upgrade hook Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Test that too large Calls in agendas are ignored Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Use new OnRuntimeUpgrade hooks Why did the CI not catch this?! Signed-off-by: Oliver Tale-Yazdi <[email protected]> * works fine - just more logs Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix staking migration Causing issues on Kusama... Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix UI tests No idea why this is needed. This is actually undoing an earlier change. Maybe the CI has different rustc versions!? Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Remove multisig's Calls (#12072) * Remove multisig's Calls * Multisig: Fix tests and re-introduce reserve logic (#12241) * Fix tests and re-introduce reserve logic * fix benches * add todo * remove irrelevant bench * [Feature] Add a migration that drains and refunds stored calls (#12313) * [Feature] Add a migration that drains and refunds stored calls * migration fixes * fixes * address review comments * consume the whole block weight * fix assertions * license header * fix interface Co-authored-by: parity-processbot <> Co-authored-by: parity-processbot <> Co-authored-by: Roman Useinov <[email protected]> * Fix test Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix multisig benchmarks Signed-off-by: Oliver Tale-Yazdi <[email protected]> * ".git/.scripts/bench-bot.sh" pallet dev pallet_democracy * ".git/.scripts/bench-bot.sh" pallet dev pallet_scheduler * ".git/.scripts/bench-bot.sh" pallet dev pallet_preimage Signed-off-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: Shawn Tabrizi <[email protected]> Co-authored-by: parity-processbot <> Co-authored-by: Roman Useinov <[email protected]>

* Introduce preimages module in traits * Multisize Preimages * Len not actually necessary * Tweaks to the preimage API * Fixes * Get Scheduler building with new API * Scheduler tests pass * Bounded Scheduler 🎉 * Use Agenda holes and introduce IncompleteSince to avoid need to reschedule * Tests pass with new weight system * New benchmarks * Add missing file * Drop preimage when permenantly overeight * Drop preimage when permenantly overeight * Referenda uses latest preimage API * Testing ok * Adding tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * fmt Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add preimage migration Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Docs * Remove dbg * Refactor Democracy * Refactor Democracy * Add final MEL * Remove silly maps * Fixes * Minor refactor * Formatting * Fixes * Fixes * Fixes * Update frame/preimage/src/lib.rs Co-authored-by: Shawn Tabrizi <[email protected]> * Add migrations to Democracy * WIP Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Resolve conflicts Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Revert "Resolve conflicts" This reverts commit a89cd0a. * Undo wrong resolves... Signed-off-by: Oliver Tale-Yazdi <[email protected]> * WIP Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Make compile Signed-off-by: Oliver Tale-Yazdi <[email protected]> * massage clippy Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More clippy Signed-off-by: Oliver Tale-Yazdi <[email protected]> * clippy annoyance Signed-off-by: Oliver Tale-Yazdi <[email protected]> * clippy annoyance Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix benchmarks Signed-off-by: Oliver Tale-Yazdi <[email protected]> * add missing file * Test <Preimage as QueryPreimage> Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Clippy harassment Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add test Signed-off-by: Oliver Tale-Yazdi <[email protected]> * clippy Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fixup tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Remove old stuff Signed-off-by: Oliver Tale-Yazdi <[email protected]> * fmt Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Test <Scheduler as Anon> trait functions Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Update pallet-ui tests Why is this needed? Should not be the case unless master is broken... Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More scheduler trait test Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Apply review suggestion Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Beauty fixes Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add Scheduler test migration_v3_to_v4_works Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Merge fixup Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Keep referenda benchmarks instantiatable Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Update weights Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Use new scheduler weight functions Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Use new democracy weight functions Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Use weight compare functions Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Update pallet-ui tests Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More renaming… Signed-off-by: Oliver Tale-Yazdi <[email protected]> * More renaming… Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add comment Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Implement OnRuntimeUpgrade for scheduler::v3_to_v4 migration Put the migration into a proper `MigrateToV4` struct and implement the OnRuntimeUpgrade hooks for it. Also move the test to use that instead. This should make it easier for adding it to Polkadot. Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Clippy Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Handle undecodable Agendas Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Remove trash Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix test Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Use new OnRuntimeUpgrade functions Signed-off-by: Oliver Tale-Yazdi <[email protected]> * fix test Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix BoundedSlice::truncate_from Co-authored-by: jakoblell Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix pre_upgrade hook return values Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Add more error logging Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Find too large preimages in the pre_upgrade hook Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Test that too large Calls in agendas are ignored Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Use new OnRuntimeUpgrade hooks Why did the CI not catch this?! Signed-off-by: Oliver Tale-Yazdi <[email protected]> * works fine - just more logs Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix staking migration Causing issues on Kusama... Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix UI tests No idea why this is needed. This is actually undoing an earlier change. Maybe the CI has different rustc versions!? Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Remove multisig's Calls (paritytech#12072) * Remove multisig's Calls * Multisig: Fix tests and re-introduce reserve logic (paritytech#12241) * Fix tests and re-introduce reserve logic * fix benches * add todo * remove irrelevant bench * [Feature] Add a migration that drains and refunds stored calls (paritytech#12313) * [Feature] Add a migration that drains and refunds stored calls * migration fixes * fixes * address review comments * consume the whole block weight * fix assertions * license header * fix interface Co-authored-by: parity-processbot <> Co-authored-by: parity-processbot <> Co-authored-by: Roman Useinov <[email protected]> * Fix test Signed-off-by: Oliver Tale-Yazdi <[email protected]> * Fix multisig benchmarks Signed-off-by: Oliver Tale-Yazdi <[email protected]> * ".git/.scripts/bench-bot.sh" pallet dev pallet_democracy * ".git/.scripts/bench-bot.sh" pallet dev pallet_scheduler * ".git/.scripts/bench-bot.sh" pallet dev pallet_preimage Signed-off-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: Shawn Tabrizi <[email protected]> Co-authored-by: parity-processbot <> Co-authored-by: Roman Useinov <[email protected]>

Fix tests and re-introduce reserve logic

7bacd73

ruseinov requested review from gavofyork, kianenigma and bkchr September 12, 2022 12:47

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 12, 2022

fix benches

e9ab03f

ruseinov marked this pull request as ready for review September 13, 2022 07:27

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 13, 2022

ruseinov added A3-in_progress Pull request is in progress. No review needed at this stage. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. and removed A0-please_review Pull request needs code review. labels Sep 13, 2022

add todo

d2776bd

ruseinov commented Sep 13, 2022

View reviewed changes

frame/multisig/src/benchmarking.rs Outdated Show resolved Hide resolved

remove irrelevant bench

42acac2

kianenigma reviewed Sep 14, 2022

View reviewed changes

kianenigma approved these changes Sep 14, 2022

View reviewed changes

gavofyork reviewed Sep 15, 2022

View reviewed changes

gavofyork approved these changes Sep 15, 2022

View reviewed changes

ruseinov merged commit b1c681b into gav-rm-multisig-calls Sep 16, 2022

ruseinov deleted the ru/feature/rm-multisig-calls branch September 16, 2022 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multisig: Fix tests and re-introduce reserve logic #12241

Multisig: Fix tests and re-introduce reserve logic #12241

ruseinov commented Sep 12, 2022 •

edited

Loading

kianenigma Sep 14, 2022

gavofyork Sep 15, 2022

kianenigma Sep 14, 2022

ruseinov Sep 16, 2022

ruseinov Sep 16, 2022 •

edited

Loading

kianenigma Sep 14, 2022

ruseinov Sep 16, 2022

ruseinov Sep 16, 2022 •

edited

Loading

kianenigma left a comment

gavofyork left a comment

gavofyork left a comment

ruseinov commented Sep 16, 2022

ruseinov commented Sep 16, 2022

ruseinov commented Sep 16, 2022

paritytech-processbot bot commented Sep 16, 2022

Multisig: Fix tests and re-introduce reserve logic #12241

Multisig: Fix tests and re-introduce reserve logic #12241

Conversation

ruseinov commented Sep 12, 2022 • edited Loading

kianenigma Sep 14, 2022

Choose a reason for hiding this comment

gavofyork Sep 15, 2022

Choose a reason for hiding this comment

kianenigma Sep 14, 2022

Choose a reason for hiding this comment

ruseinov Sep 16, 2022

Choose a reason for hiding this comment

ruseinov Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

kianenigma Sep 14, 2022

Choose a reason for hiding this comment

ruseinov Sep 16, 2022

Choose a reason for hiding this comment

ruseinov Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

gavofyork left a comment

Choose a reason for hiding this comment

gavofyork left a comment

Choose a reason for hiding this comment

ruseinov commented Sep 16, 2022

ruseinov commented Sep 16, 2022

ruseinov commented Sep 16, 2022

paritytech-processbot bot commented Sep 16, 2022

ruseinov commented Sep 12, 2022 •

edited

Loading

ruseinov Sep 16, 2022 •

edited

Loading

ruseinov Sep 16, 2022 •

edited

Loading