Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove timepoint from multisig #35

Closed
ozgunozerk opened this issue Nov 30, 2023 · 2 comments
Closed

Remove timepoint from multisig #35

ozgunozerk opened this issue Nov 30, 2023 · 2 comments
Assignees
Labels
discussion research and discussion items

Comments

@ozgunozerk
Copy link
Collaborator

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:
  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.

Here is the relevant PR I've opened: paritytech/polkadot-sdk#2542

@ozgunozerk ozgunozerk added the discussion research and discussion items label Nov 30, 2023
@ozgunozerk ozgunozerk self-assigned this Nov 30, 2023
@4meta5
Copy link
Contributor

4meta5 commented Nov 30, 2023

Thank you! I agree that it is not the extrinsic's responsibility to safeguard user input.

Here is my understood pros/con list for the proposed change. Please let me know if I am missing anything!

Pros:

  • fewer extrinsic arguments so easier for callers to form the call
  • less storage space by removing key and event

Con:

  • new call format requires updating polkadot-js and any middleware that calls pallet-multisig calls

I do think it is important for us to try to measure these pros/cons:

  1. How unhappy are users with the timepoints argument? How much happier would they be if it was removed?
  2. How much is space or compute efficiency improved by the code changes?
  3. How much work is required downstream to update calling code to the new call format?

@4meta5 4meta5 moved this from Todo to In Review in Substrate Runtime Templates Nov 30, 2023
@4meta5 4meta5 added this to the 1st Deliverable milestone Nov 30, 2023
@ozgunozerk
Copy link
Collaborator Author

ozgunozerk commented Dec 1, 2023

I agree with the Pros/Cons list.

Here are my takes on the questions:

  1. How unhappy are users with the timepoints argument? How much happier would they be if it was removed?

I think this question will lead us to the wrong decision. Current users are already ok with the current design, hence, they are users.

The improvement I'm proposing will probably increase the adoption of new users. For example, suppose someone is planning to use substrate and multisig, and he comes across how difficult it will be for him with the timepoint. In that case, he might consider to switching to other frameworks/blockchains due to developer experience. And I believe this is the most important thing regarding this change.

For the existing users, it might be more complicated:

  • they will have less code to maintain for the future
  • it will be a breaking change
  1. How much is space or compute efficiency improved by the code changes?

I don't think much computing will be gained. However, I think each time you profit from storage, it adds up, since you are making the whole history smaller, and that is very important for blockchains.

  1. How much work is required downstream to update calling code to the new call format?

I have no idea, maybe parity team can answer this better than us

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion research and discussion items
Projects
Archived in project
Development

No branches or pull requests

3 participants