Migrates pallet-multisig to Consideration #1782

gupnik · 2023-10-03T14:16:04Z

Part of #226

In order to simply migrate the existing runtimes, a diff can look like the following:

-	type DepositBase = DepositBase;
-	type DepositFactor = DepositFactor;
+       type Consideration = HoldConsideration<
+		AccountId,
+		Balances,
+		MultisigHoldReason,
+		LinearStoragePrice<DepositBase, DepositFactor, Balance>,
+	>;

Polkadot-Forum · 2023-11-16T14:18:35Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/frame-monthly-update/4628/1

…ik/multisig-consideration

liamaharon · 2024-01-16T06:34:14Z

substrate/frame/multisig/src/tests.rs

+		let updated_multisig = MultisigsFor::<Test>::get(&multi, &call_hash).unwrap();
+		assert_eq!(updated_multisig.depositor, 1);


should we check all fields for sanity?

liamaharon · 2024-01-16T06:38:32Z

substrate/frame/multisig/src/lib.rs

+		T::Currency::unreserve(&r.depositor, r.deposit);
+		// take consideration
+		let Ok(ticket) =
+			T::Consideration::new(&r.depositor, Footprint::from_parts(1, threshold as usize))


when migrating, shouldn't we take exactly r.deposit instead of calculating using the current config? to handle the case where the reserve amounts changed.

As we are un-reserving the previous amount, isn't it a fair assumption to charge the current amount in this lazy migration?

I wouldn't say so, because it's not what the user agreed to / understood at the time that they made the multisig. I would expect deposit amount changes to only impact new multisigs, not be applied retroactively

I see your point, but I also don't think that this is a significant issue. It occurs only for a specific case when the deposit amount has changed between the time of creation vs this migration, which is also handled, just that it requires an increase in the deposit. @ggwpez @kianenigma Would love to get your thoughts here. I imagine this was the reason we were trying to add new_from_exact API in consideration, but this looks to me as a better trade-off.

Yes this is why we were trying to add new_from_exact. I guess adding that failed/is still in progress? :P

#2274 can be used to migrate them over with an exact amount.
It was added as new trait to not bloat the existing one and not introduce use-once code that lingers forever.

I think we shouldn't change the amounts on the fly, the user only agreed to the amount that they signed and if they are early adopters of Polkadot we should not punish that for no good reason.

I think we shouldn't change the amounts on the fly, the user only agreed to the amount that they signed and if they are early adopters of Polkadot we should not punish that for no good reason.

I agree

I think we shouldn't change the amounts on the fly, the user only agreed to the amount that they signed and if they are early adopters of Polkadot we should not punish that for no good reason.

Thanks - but I do see this pattern already being used in preimage pallet here. Won't that be impacted with a similar reasoning?

Yes, I would have also argued to not have used that pattern in the preimage pallet.

liamaharon · 2024-01-16T06:39:29Z

substrate/frame/multisig/src/lib.rs

+			T::Consideration::new(&r.depositor, Footprint::from_parts(1, threshold as usize))
+				.defensive_proof("Unexpected inability to take deposit after unreserved")
+		else {
+			return true


if this returns early, the new multisig is not added to MultisigsFor. should we just defensive log but continue anyways to not break the multisig?

Well, multisig can't exist in the system if we were not able to take the consideration. I can throw an error in this case though.

Wouldn't it be bad ux for a multisig to just vanish?

This shouldn't be an issue if we don't try re-reserving different amounts, though

It won't vanish but the migration would fail hinting the user to add funds if needed.

I don't see where it fails, maybe I misunderstand but I see ensure_updated returns true in this conditional if the consideration fails to create

Yeah, I should have been clearer. I meant that once it throws an error as I mentioned above, it should be fine. Will update that.

I can throw an error in this case though.

ggwpez · 2024-01-17T15:35:51Z

substrate/frame/multisig/src/lib.rs

+	/// The amount held in reserve of the `depositor`, to be returned once the operation ends.
+	ticket: Ticket,


I think the name Ticket was swapped for Consideration or so.

ggwpez · 2024-01-17T15:38:28Z

substrate/frame/multisig/src/lib.rs

+
+	/// The set of open multisig operations.
+	#[pallet::storage]
+	pub type MultisigsFor<T: Config> = StorageDoubleMap<


This is a breaking change and will probably mess with wallets/indexers etc since they look at the old map.
I dont think there is an easy way out without MBMs (audit stalled for another month), so not sure what to best do.

TBH since this is not the highest priority I don't think too much of a problem to wait for MBMs.. should try to minimise breakage of these apis as much as possible, as you point out. with this change, possibly every multisig UI in the ecosystem would break :P

Yeah, fine to wait for MBM if that's the case.

ggwpez · 2024-01-17T15:40:11Z

substrate/frame/multisig/src/lib.rs

-				<Multisigs<T>>::remove(&id, call_hash);
-				T::Currency::unreserve(&m.depositor, m.deposit);
+				<MultisigsFor<T>>::remove(&id, call_hash);
+				let _ = m.ticket.drop(&m.depositor);


Does it panic when its not manually dropped?

You mean if someone forgot to call drop on the ticket?

in general i think let _ should be a no-no across the repo.

If in this situation, you specifically want to drop the result you should call mem::drop (AFAIK)

ggwpez · 2024-01-17T15:46:50Z

substrate/frame/multisig/src/lib.rs

@@ -665,6 +691,25 @@ impl<T: Config> Pallet<T> {
 		signatories.insert(index, who);
 		Ok(signatories)
 	}
+
+	pub(crate) fn ensure_updated(id: &T::AccountId, call_hash: &[u8; 32], threshold: u16) -> bool {


I think we can safely expose this function publicly as extrinsic to manually nudge it to speed up the process.

* Try check-rustdoc pipeline * another try * another try without `--all-features` * fixed cargo doc issues * exclude relay-rialto-parachain-client from cargo doc --------- Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>

cla-bot-2021 · 2024-07-08T16:18:19Z

User @codeknix, please sign the CLA here.

bkchr · 2024-07-17T12:34:15Z

@gupnik what is the status on this?

gupnik · 2024-07-17T12:48:05Z

@gupnik what is the status on this?

Yes, it's in my todo list but haven't been able to get back to this yet. Will try to prioritise. Thanks for the reminder.

cla-bot-2021 · 2024-10-01T15:17:05Z

User @codekitz, please sign the CLA here.

Uses consideration

df0ea02

gupnik mentioned this pull request Oct 3, 2023

FRAME: Move pallets over to use fungible traits #226

Open

33 tasks

gupnik added 6 commits January 3, 2024 12:01

Merge branch 'master' of github.com:paritytech/polkadot-sdk into gupn…

424d499

…ik/multisig-consideration

Adds lazy migration

c47aaf6

Fixes test

4b9d2fe

Updates benchmarking

ea4ed0c

Fixes kitchensink runtime

1891587

Updates all runtimes

ab2cf82

gupnik added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Jan 5, 2024

gupnik added 3 commits January 8, 2024 10:33

Updates all runtimes

db8aee0

Adds lazy migration test

a1f2233

Adds PrDoc

cde98b3

gupnik marked this pull request as ready for review January 9, 2024 04:08

gupnik requested review from a team January 9, 2024 04:08

Merge branch 'master' into gupnik/multisig-consideration

cf1993e

liamaharon reviewed Jan 16, 2024

View reviewed changes

ggwpez reviewed Jan 17, 2024

View reviewed changes

gupnik mentioned this pull request Jan 30, 2024

Migrate Currency trait to Fungible in Multisig pallet #3136

Closed

This was referenced Jun 5, 2024

Update polkadot-sdk from v1.7.0 to v1.11.0 moondance-labs/tanssi#573

Closed

Update polkadot-sdk from v1.10.0 to v1.11.0 moondance-labs/tanssi#577

Closed

gupnik closed this Oct 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrates pallet-multisig to Consideration #1782

Migrates pallet-multisig to Consideration #1782

gupnik commented Oct 3, 2023 •

edited

Loading

Polkadot-Forum commented Nov 16, 2023

liamaharon Jan 16, 2024

liamaharon Jan 16, 2024

gupnik Jan 16, 2024 •

edited

Loading

liamaharon Jan 17, 2024

gupnik Jan 17, 2024 •

edited

Loading

liamaharon Jan 17, 2024

ggwpez Jan 17, 2024 •

edited

Loading

liamaharon Jan 18, 2024

gupnik Jan 18, 2024

liamaharon Jan 18, 2024 •

edited

Loading

liamaharon Jan 16, 2024

gupnik Jan 16, 2024

liamaharon Jan 17, 2024

gupnik Jan 17, 2024

liamaharon Jan 17, 2024 •

edited

Loading

gupnik Jan 17, 2024 •

edited

Loading

ggwpez Jan 17, 2024

ggwpez Jan 17, 2024

liamaharon Jan 18, 2024 •

edited

Loading

gupnik Jan 18, 2024

ggwpez Jan 17, 2024

gupnik Jan 18, 2024

shawntabrizi Jul 6, 2024

ggwpez Jan 17, 2024

cla-bot-2021 bot commented Jul 8, 2024

bkchr commented Jul 17, 2024

gupnik commented Jul 17, 2024

cla-bot-2021 bot commented Oct 1, 2024

		let updated_multisig = MultisigsFor::<Test>::get(&multi, &call_hash).unwrap();
		assert_eq!(updated_multisig.depositor, 1);

		/// The amount held in reserve of the `depositor`, to be returned once the operation ends.
		ticket: Ticket,

Migrates pallet-multisig to Consideration #1782

Migrates pallet-multisig to Consideration #1782

Conversation

gupnik commented Oct 3, 2023 • edited Loading

Polkadot-Forum commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gupnik Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gupnik Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamaharon Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamaharon Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

gupnik Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamaharon Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot-2021 bot commented Jul 8, 2024

bkchr commented Jul 17, 2024

gupnik commented Jul 17, 2024

cla-bot-2021 bot commented Oct 1, 2024

gupnik commented Oct 3, 2023 •

edited

Loading

gupnik Jan 16, 2024 •

edited

Loading

gupnik Jan 17, 2024 •

edited

Loading

ggwpez Jan 17, 2024 •

edited

Loading

liamaharon Jan 18, 2024 •

edited

Loading

liamaharon Jan 17, 2024 •

edited

Loading

gupnik Jan 17, 2024 •

edited

Loading

liamaharon Jan 18, 2024 •

edited

Loading