Feat: Multisig proposal minimum time and expiry time

[arrudagates]

Cores should be able to set the minimum amount of time a proposal should be open for (perhaps counting from when it passes the thresholds instead of from when it's open) before it's actually able to enact.

They should also be able to set an expiry time for automatically rejecting proposals in case they surpass this time without being approved.

Both of these parameters should also support the current behavior, which is no expiry time and no minimum lifetime.

[Szegoo]

I would love to see this added to the inv4 pallet. I think expiry and minimum lifetime should be considered as a basic functionality for multisigs, but a lot of them are actually missing this part for some reason(even the official substrate pallet doesn't have this 'feature' https://forum.polkadot.network/t/expiry-time-for-multisig-calls/1694/3).

@arrudagates If you haven't already started working on this I would like to implement this myself.

[arrudagates]

I would love to see this added to the inv4 pallet. I think expiry and minimum lifetime should be considered as a basic functionality for multisigs, but a lot of them are actually missing this part for some reason(even the official substrate pallet doesn't have this 'feature' https://forum.polkadot.network/t/expiry-time-for-multisig-calls/1694/3).

@arrudagates If you haven't already started working on this I would like to implement this myself.

I haven't started this one yet, feel free to work on this if you'd like, all I ask is that you share how you plan to implement it and what architecture changes would be required to implement your approach!

[Szegoo]

My plan is the following:

• Add two optional arguments to the operate_multisig extrinsic representing minimum time and expiry time.
• Add two new fields to the MultisigOperation struct also representing minimum time and expiry time.
  Also since we count the minimum time from when the proposal passed the threshold, we need a new field that will store the block number from when the proposal is passing. We will also need the block number when the proposal was submitted since the expiry time will be counted from that moment.

So essentially the MultisigOperation struct would look like this:

pub struct MultisigOperation<AccountId, TallyOf, Call, Metadata, BlockNumber> {
    pub tally: TallyOf,
    pub original_caller: AccountId,
    pub actual_call: Call,
    pub call_metadata: [u8; 2],
    pub call_weight: Weight,
    pub expiry: Option<BlockNumber>,
    pub min_passing: Option<BlockNumber>,
    pub passing_since: Option<BlockNumber>,
    pub submitted: BlockNumber,
    pub metadata: Option<Metadata>,
}

Of course, changes in inner_operate_multisig would be needed to support this new functionality.

[arrudagates]

My plan is the following:

• Add two optional arguments to the operate_multisig extrinsic representing minimum time and expiry time.
• Add two new fields to the MultisigOperation struct also representing minimum time and expiry time.
  Also since we count the minimum time from when the proposal passed the threshold, we need a new field that will store the block number from when the proposal is passing. We will also need the block number when the proposal was submitted since the expiry time will be counted from that moment.

So essentially the MultisigOperation struct would look like this:

pub struct MultisigOperation<AccountId, TallyOf, Call, Metadata, BlockNumber> {
    pub tally: TallyOf,
    pub original_caller: AccountId,
    pub actual_call: Call,
    pub call_metadata: [u8; 2],
    pub call_weight: Weight,
    pub expiry: Option<BlockNumber>,
    pub min_passing: Option<BlockNumber>,
    pub passing_since: Option<BlockNumber>,
    pub submitted: BlockNumber,
    pub metadata: Option<Metadata>,
}

Of course, changes in inner_operate_multisig would be needed to support this new functionality.

This cannot be done in operate_multisig because minimum time and expiry time are security parameters imposed by the multisig, allowing members to arbitrarily set them per call would defeat their purpose.

minimum_time and expiry_time should both be parameters of the Core, alongside required_approval and minimum_support, like this:

pub struct CoreInfo<AccountId, CoreMetadataOf> {
    pub account: AccountId,
    pub metadata: CoreMetadataOf,
    pub minimum_support: Perbill,
    pub required_approval: Perbill,
    pub frozen_tokens: bool,

    pub minimum_time: u32,
    pub expiry_time: u32,
}

These would then be mutable by the multisig when calling set_parameters.

Regarding minimum_time I actually think it would be a better user experience if it counted from the start, rather than from when the proposal becomes passing as that would quickly become very confusing (and exploitable) if members were to change their votes after it starts counting.

For how to control these, I think a better approach is to add minimum_time and expiry_time to the MultisigOperation struct as pre-calculated blocks:

minimum_block: current_block + minimum_time,
expiry_block: current_block + expiry_time,

And then in vote_multisig these can be checked against the current block alongside the required_approval and minimum_support checks.

Now there's still one thing that needs to be designed in order for all of this to work, and that is a "scheduler" of sorts. Basically we need to automatically dispatch calls once they hit the minimum_time, if the target block is reached and it's passing, and we also need to automatically clean up any expired call once the expiry block is reached.

I was thinking of the following system:

1. Member proposes a call, if it does start a vote we push an entry to a new storage made to track expiry (keys are expiry_block, core_id and call_hash).
2. Once someone votes and gets it past the thresholds, if it happens before minimum_time then we push an entry to yet another new storage, this one tracks passed proposals (keys are minimum_block, core_id and call_hash)
3. Alternatively, if the vote that gets it past the thresholds happens after the minimum time we don't need to push it to the minimum time storage and we can clean the entry from the expiry time storage.

Meanwhile there's a hook running at every block that checks both of these new storages.
If it finds a call in the minimum time storage, it dispatches the call and clears it's entry from the expiry storage.
If it finds a call in the expiry storage it clears that entry and also clears the multisig operation itself from storage.

Edit: Forgot to mention but both of these parameters should also be optional, as it shouldn't be required for a multisig to have minimum proposal time or a proposal expiry time and these should be left for the members to decide, so in the end both of these parameters should be wrapped around Option and should also be usable independently of the other (minimum time but no expiry time/expiry time but no minimum time)

[Szegoo]

Regarding minimum_time I actually think it would be a better user experience if it counted from the start, rather than from when the proposal becomes passing as that would quickly become very confusing (and exploitable) if members were to change their votes after it starts counting.

Yeah, that makes sense. Counting from when it is passing sounds better but there can be some tricky edge cases so I do agree with you. Also, the reason why I wanted to start counting from when it is passing is because you mentioned it like this in the initial issue description:

Cores should be able to set the minimum amount of time a proposal should be open for (perhaps counting from when it passes the thresholds instead of from when it's open) before it's actually able to enact.

I like the idea you mentioned in your comment. I think it is good to store the proposals in different storage at different stages.

[Szegoo]

Meanwhile there's a hook running at every block that checks both of these new storages.
If it finds a call in the minimum time storage, it dispatches the call and clears it's entry from the expiry storage.
If it finds a call in the expiry storage it clears that entry and also clears the multisig operation itself from storage.

@arrudagates I don't think iterating over the ongoing multisigs in each block would be very scalable. IMO This could only work with some bounds on the number of ongoing multisig operations.

Maybe a better idea would be to introduce new extrinsics that could handle ongoing multisigs. For example, we would have a new end_multisig that can be called by anyone and will only work if the expirty_time is greater than the current block number.

The same way we would have an execute_mutlisig extrinsic which could be called by anyone and it will actually execute the proposal if all the requirements are met.