Improve developer experience by removing timepoint from multisig

[ozgunozerk]

What is timepoint?

• In multisig pallet, we have timepoint parameter for as_multi and approve_as_multi functions.
• Multisig operations can be uniquely identified with AccountId, Hash pair, and they are stored in storage as StorageDoubleMap. And we don't need timepoint to uniquely identify multisig operations.
• I believe the only advantage of having timepoint is:
  • say, someone created a multisig operation (M), with AccoundId = A, and Hash = H
  • then, this multisig operation gets approved, and dispatched -> after dispatch, it will get removed from the storage
  • but, one of the approvers of M (say Alice) forgot to approve it (and it's ok. It got dispatched, which means it got enough approvals already)
  • after some time, another multisig operation (M') is created, with AccountId = A, and Hash = H (so basically, it is the same multisig operation again)
  • and Alice, continues to be careless by:
    • not submitting approval on time
    • not being aware of the multisig operation got dispatched already
    • not being aware of a new one is already waiting for approval
  • and then she decides to approve the multisig operation M, but in fact, she will be approving M'
  • and although M' is basically the exact copy of M, it will somehow be a problem that Alice approved M'.
  • so, yes, having a timepoint parameter prevents that highly unlikely and probably harmless situation... But at what cost?

The problem

Having a timepoint parameter is significantly hindering the developer experience for multisig pallet. Due to following reasons:

1. providing the timepoint is an arduous task. Let me elaborate:
   • no function is exposed in the library for finding the timepoint of a multisig operation.
   • see this example for how many steps are required for finding the timepoint: https://github.com/paritytech/txwrapper-core/tree/768bb445beb2907582b2d5e13ade3be5d995af3e/packages/txwrapper-examples/multisig#timepoint-of-approveasmulti--storage
   • this complexity is probably discouraging developers from using multisig pallet and substrate
2. it is not obvious what timepoints purpose is in the pallet. It is confusing for most of the developers to encounter timepoint

The solution

Simply getting rid of timepoint in multisig pallet.

And in the scenario where Alice is too careless, and approves M' instead of M, and this becomes a problem for her, I think it is Alice to blame, not this pallet.

To me, it seems like: to make maybe 1 or 2 user/s happy, we are making 99.9% of the users unhappy.

[joepetrowski]

so, yes, having a timepoint parameter prevents that highly unlikely and probably harmless situation... But at what cost?

At the cost of the scenario you described. It's very likely that a multisig would approve the same call many times (e.g. recurring payments), so I don't think it's highly unlikely nor harmless. The documentation of the type that you deleted tells you that it is to "allow a transaction in which a multisig operation of a particular composite was created to be uniquely identified.".

no function is exposed in the library for finding the timepoint of a multisig operation.

Adding one seems like the better solution than removing the replay protection.

[gavofyork]

It's not about "carelessness", it's about the other key(s) being compromised and acting in a malevolent fashion.

The timepoint is there primarily to ensure that the authorization can only be utilized on the instance of the call expected at the time of authorizing. It's a real concern, but probably not so important for most use-cases.

I appreciate that it may be a friction point for developers. However, rather than the change suggested of removing the timepoint entirely and making a breaking change, not to mention preclude the possibility of security-minded applications utilizing it, I would consider making it optional. Since it is already passed as an option with maybe_timepoint, I would relax this so it can be None at any time, and if None, that the requirement that it equals the ongoing multisig operation be relaxed. In particular, the two lines:

			let timepoint = maybe_timepoint.ok_or(Error::<T>::NoTimepoint)?;
			ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);

would become

			if let Some(timepoint) = maybe_timepoint.as_ref() {
				ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);
			}

[ozgunozerk]

I think the suggested solution by Gavin Wood should be great for everything:

• backwards compatibility
• security
• reducing developer friction

do you want me to contribute by changing my PR to that solution?

Edit: opened a new PR with the suggested solution (changed docs, added tests, and changed events)