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

relaxing timepoint constraint for multisig #2735

Closed

Conversation

ozgunozerk
Copy link
Contributor

@ozgunozerk ozgunozerk commented Dec 18, 2023

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

@ozgunozerk ozgunozerk requested review from a team December 18, 2023 12:49
@paritytech-review-bot paritytech-review-bot bot requested a review from a team December 18, 2023 12:50
@ozgunozerk ozgunozerk mentioned this pull request Dec 18, 2023
4 tasks
Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ozgunozerk
Copy link
Contributor Author

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

@xlc
Copy link
Contributor

xlc commented Dec 18, 2023

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

@ozgunozerk ozgunozerk requested a review from xlc December 19, 2023 06:46
@ozgunozerk
Copy link
Contributor Author

ozgunozerk commented Dec 19, 2023

  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
Copy link
Contributor Author

Any updates on this?

@kianenigma kianenigma added the T2-pallets This PR/Issue is related to a particular pallet. label Mar 27, 2024
@kianenigma kianenigma requested review from gupnik and removed request for xlc March 27, 2024 10:13
@paritytech-review-bot paritytech-review-bot bot requested a review from a team March 27, 2024 10:14
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

Copy link
Contributor

@gupnik gupnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team April 2, 2024 09:40
@ozgunozerk
Copy link
Contributor Author

@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
Copy link
Contributor Author

Done!

substrate/frame/multisig/src/migrations.rs Outdated Show resolved Hide resolved
@ozgunozerk
Copy link
Contributor Author

ozgunozerk commented Apr 4, 2024

@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

/// # Multi-Block Migrations Module
///
/// This module showcases a simple use of the multi-block migrations framework.
pub mod v2;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -0,0 +1,84 @@
// This file is part of Substrate.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea we can re-generate them as last step.

@github-actions github-actions bot requested a review from gupnik April 17, 2024 14:07
Copy link

Review required! Latest push from author must always be reviewed

@ozgunozerk
Copy link
Contributor Author

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

@ozgunozerk
Copy link
Contributor Author

@ggwpez should we progress into generating weights?

@@ -109,7 +124,9 @@ where
MaxApprovals: Get<u32>,
{
/// The extrinsic when the multisig operation was opened.
when: Timepoint<BlockNumber>,
/// `timepoint` is an optional extra security measure, which can be enabled by the creator of
/// the multisig during the creation by supplying a `timepoint` for the `maybe_when` field.
Copy link
Member

@ggwpez ggwpez Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@Tbaut Tbaut Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

bkchr commented Jul 17, 2024

Closing as this seems to be stale.

@bkchr bkchr closed this Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve developer experience by removing timepoint from multisig
8 participants