relaxing timepoint constraint for multisig

[ozgunozerk]

Description

This PR aims to improve developer experience by solving #2541. A detailed explanation is provided in the issue itself.

• What does this PR do?
  Relaxes the timeout parameter for multisig pallet as suggested by @gavofyork
• Why are these changes needed?
  It will significantly improve the developer experience
• How were these changes implemented and what do they affect?
  This won't be a breaking change :)

Fixes #2541

Checklist

• My PR includes a detailed description as outlined in the "Description" section above
• My PR follows the labeling requirements of this project (at minimum one label for T
  required) (it should probably be labeled with T2 for this)
• I have made corresponding changes to the documentation
• I have added related tests

[xlc]

please avoid tx version breaking change. you can add a new extrinsic and keep the old one remain as it is.

[ozgunozerk]

Hi @xlc , just to be sure, you meant cancel_as_multi?

[xlc]

yes. any changes to existing extrinsic signature is tx breaking change

[ozgunozerk]

1. implemented a new extrinsic called cancel_as_multi_without_timepoint.
2. extracted the reusable code to a function, to reduce code duplication between cancel_as_multi and cancel_as_multi_without_timepoint
3. added benchmark item for cancel_as_multi_without_timepoint
4. modified tests accordingly

[ozgunozerk]

Any updates on this?

[kianenigma]

LGTM, needs audit and a clear documentation both on the method and the pallet top level docs explaining the tradeoff of what you gain by using the timepoint and what not. In other words, the scenario explained in the issue.

[ozgunozerk]

@kianenigma @xlc, anything left for this PR that you want me to do? Or are we just waiting for the audit?

[gupnik]

I was expecting that we would modify the MultiSig struct to have when as optional. Then, we would ensure that it is mandatory to provide the timepoint if it was Some at the time of creation of the multisig.

The current change allows users to pass None for those multisigs as well that might want to enforce a timepoint check.

[ozgunozerk]

@gupnik I agree, I think your suggestion will be quite beneficial for the developers. Implemented the suggested way.

Before I also modify the tests accordingly, I wanted to get confirmation on my assumptions/choices.

Problem: during the creation of the multisig, how can the creator (the first approver) can select whether the multisig will be with or without timepoint?

Solutions:

• add a boolean flag for this to approve_as_multi and as_multi functions
  • did not go with this one, since it will be a breaking change, and will be useless for further approvals. I thought this would unnecessarily complicate the UX.
• use the current maybe_timepoint for this flag (whether to enable timepoint or not during the creation of the multisig).
  • went with this approach
  • supplying Some(timepoint) means that the creator wants to enable timepoint feature for this multisig
  • supplying None means that the creator wants to disable timepoint feature for this multisig

However, there are some important details:

1. supplying Some(timepoint) for the creation -> the actual value for timepoint does not serve any purpose, and it can be anything. Because, during creation, we will use Self::timepoint() function to get the timepoint. I've explained this in inline documentation, so I think it won't be confusing for those who read the description of the function.
2. I wanted to somehow allow the creator to not supply any timepoint, yet able to enable the timepoint security measure, but without introducing a breaking change, I couldn't do it.
3. since we may get a timepoint even during the creation now, the previous UnexpectedTimeout error is now gone. I don't know if this counts towards introducing a breaking change.
4. everything else now should work as described in the suggestion. If the multisig is created with a timepoint, we require the following approvals to provide a timepoint as well. If the multisig is created without a timepoint, the timepoint field in the following approvals will be ignored, so it can be anything, we don't care.

If you confirm all the above, I can proceed to updating tests accordingly.

[ozgunozerk]

Done!

[ozgunozerk]

@liamaharon
I tried to implement multi-block migrations as presented in the example you provided.

There are a few obstacles I've stumbled upon, they are all marked with TODO: in the codebase.
Please find them in the review part, I'll explicitly comment on them as self-review

[ozgunozerk]

I only applied multi-block migration approach to the new migration logic (v2). There was an already existing migration logic for v0 to v1, but I did not touch it, since I think it's outside the scope of this PR.

[ggwpez]

The existing MigrateToV1 is written in a way that will not make it work anymore, so i guess its fine to delete it.
Its also 1,5 yrs old, so hopefully most people updates already.

[ozgunozerk]

did not touch this file at all, don't know how to generate weights for this purpose

[ggwpez]

Yea we can re-generate them as last step.

[github-actions]

Review required! Latest push from author must always be reviewed

[ozgunozerk]

@liamaharon @ggwpez pinging once more in case notification got lost in the inbox

[ozgunozerk]

@ggwpez should we progress into generating weights?

[ggwpez]

Personally i think that the level of security should be decided upon by the approvers - not the creator.
Now the creator can trick the approvers into also approving all future versions by using None.

When the timepoint is always present, then the approvers can decide ad-hoc if they want to approve just this single call by sending Some(_), or if they want to also approve all future versions of this by using None.
This is very similar to immortal transactions.

[Tbaut]

As a UI dev using these pallets a lot, this sounds like worsening the DX:

• I need to understand all this, when I didn't before
• I need to take a decision for my users whether to pass the time-point or not, or pass this complexity down to my users by having a checkbox, and an explanation -> worse UX.
• the default, and easiest route, not caring about this, is not the safest unfortunately

Zooming out, this PR was supposed to make DX better. My opinion is that it's not the case. As an implementer, I used to send None with the first tx, and managed to find the timepoint for the subsequent approvals, it's 1 call to make. It was actually easier than having to think about all this IMHO.

[ozgunozerk]

@Tbaut, valid points, I mostly agree, but mostly.

Here is my take (from the issue #2541):

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

In short, with the current state of this PR, we achieve the following:

• purpose of the timepoint is clearly explained.
• BIG WIN: approving a multisig way easier (due to timepoint is not a necessity anymore for the most use cases)

---

As to your points:

I need to understand all this, when I didn't before.

I'd argue against you have to understand more. This PR does not introduce a new concept. Timepoint was already there, and the purpose of the timepoint has not changed. We are just making it optional because for the most multisigs, timepoint is an overkill. In other words, this PR just makes the purpose of everything clear imo.

I need to take a decision for my users whether to pass the time-point or not, or pass this complexity down to my users by having a checkbox, and an explanation -> worse UX.

Yes, you need to. But examples and reasonings behind those are explained clearly in the docs. I'd again argue against worse UX, because previously we still had to provide timepoint, and the reason for that was not clear. The explanation for the timepoint should have been there in the UX in the first place imo.

the default, and easiest route, not caring about this, is not the safest unfortunately

Yes, if anyone has a suggestion on the defaults, I'm all ears.

Zooming out, this PR was supposed to make DX better.

I believe it still does, a lot!

As an implementer, I used to send None with the first tx, and managed to find the timepoint for the subsequent approvals, it's 1 call to make. It was actually easier than having to think about all this IMHO.

If you don't care about the timepoint, you can still send None and everybody else will send None. Previously, this was not possible, even for the multisigs that do not need timepoint, every approver had to provide timepoint and finding the timepoint is not easy as I explained above from the perspective of the approvers.

---

From my perspective, when zoomed out:

• the functionality of this pallet is much more clear.
• for the majority of the use cases, approvers will have a much better time.

[Tbaut]

I should probably have said it from the beginning. I think this first assumption, that it's hard to get the timepoint of a multisig tx, is IMHO, wrong. Maybe because of the api you're using. It's literally one state call, that you must do in any case to know the currently pending multisig txs. Then the info is in the storage, here's an example of a pending multisig tx:

Yes, you need to. [decide or pass down this complexity to users]

Let's not confuse things. DX is my experience implementing this, UX is the experience of users of my application. I want to build applications that have good UX, and I don't want to feature creep them by adding options that users have no idea about. I should take the best decision for them. If the best decision is the safest decision at no additional cost for my users, and a minimal cost for me (my DX, my time) then I should include the timepoint... because.. it isn't hard IMHO.

Now I'm not willing to die on this hill, I just left this comment to give my opinion. To me it's a feature creep. I've implemented this in JS, there's been millions of hard things, retrieving the timepoint wasn't one of them. It's api.query.multisig.multisigs.entries(address) -> iterate over the stroage, then storage[1].toJson().when. I may be missing something, it may be hard in other languages, but it would mean that getting the pending multisig txs is also hard to start with.

[ozgunozerk]

I understand your concerns. I just wanted to fully explain my reasoning above, that's all!
Thanks for the conversation, appreciated 👍 🙏

This PR is old, and the related issue was opened when I first tried to understand the multisig pallet back then. I still think timepoint as is, will be confusing for newcomers. And maybe I don't know how to do it, but from what I recall, one cannot easily approve a multisig in a browser (polkadotjs app). As you mentioned, they have to execute some code afaik:

It's api.query.multisig.multisigs.entries(address) -> iterate over the stroage, then storage[1].toJson().when.

And I just think this requirement shouldn't be there in the first place for the majority of the multisigs (or at least, there should be an explanation on the UI side on why timepoint is required).

But after all, these are my subjective takes :)

[Tbaut]

one cannot easily approve a multisig in a browser (polkadotjs app)

You're mistaking. UIs have all they need to find and fill the timepoint. If Pjs apps isn't good at something, it's not with the timepoint, it's with the callData. Which is a whole other beast. And this is worked around by many UIs including one I build, Multix, more info about this workaround here. Now this issue is the subject of an RFC: polkadot-fellows/RFCs#74

[bkchr]

Closing as this seems to be stale.