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

Support multisigs, eventally with pure proxy in front, as a signatory of another multisig #474

Merged
merged 27 commits into from
Jan 23, 2024

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Jan 15, 2024

closes #431

How to test:

Let's have this in mind first






                                                          ┌─────────────────┐
                                                          │                 │
                                                          │ Alice           ├───────┐
                                                          │                 │       │
                                                          └─────────────────┘       │
                                                                                    │
                                                                                    │
                                                                                    │
                                                          ┌─────────────────┐       │    ┌──────────────────┐            ┌────────────────────┐
                                                          │                 │       │    │                  │            │                    │
                                                          │ Bob             ├───────┼────►  A-B-P Multi     ├───────────►│ Pure-A-B-P         │
                                                          │                 │       │    │                  │            │                    │
       ┌────────────────┐                                 └─────────────────┘       │    └──────────────────┘            └────────────────────┘
       │                │                                                           │
       │ Charlie        ├──┐                                                        │
       │                │  │   ┌────────────────┐         ┌─────────────────┐       │
       └────────────────┘  │   │                │         │                 │       │
                           ├──►│ C-D Multi      ├────────►│ Pure-C-D        ├───────┘
                           │   │                │         │                 │
       ┌────────────────┐  │   └────────────────┘         └─────────────────┘
       │                │  │
       │ David          ├──┘
       │                │
       └────────────────┘


We need to have 3 or 4 new accounts ideally

  • create a multisig with pure with Charlie and David. This pure is called Pure-C-D
  • create a multisig with pure with Alice, Bob (Bob doesn't have to be, but just to fit the shema above) and the Pure-C-D. We call this one A-B-P Multi/Pure. The Pure C-D is the one that's interesting here, because this is where ppl, members of this second multisig, have issues creating transactions to sign as a signatory of A-B-P Multi/Pure.

The moment you create A-B-P Multi, you will have a pending Tx to create a pure. This alone is a Tx that can be signed by Pure-C-D. You can switch to Pure-C-D view and see the new transaction that can be created (by either Charlie or David) and that needs to be approved by both (since it's 2/2) to make it to A-B-P Multi.

image


Submission checklist:

Layout

  • Change inspected in the desktop web ui
  • Change inspected in the mobile web ui

Compatibility

  • Functionality of change validated with a connected account with multisig
  • Applicable elements hidden / disabled for watched multisigs / pure
  • Looks good for solo multisig
  • Looks good for multisig with proxy

@Tbaut Tbaut marked this pull request as ready for review January 18, 2024 20:44
@asnaith
Copy link
Contributor

asnaith commented Jan 19, 2024

@Tbaut thanks for the fantastic setup and test details 🙏

I did encounter an issue when following them though:

After I created A-B-P Multi I saw the create pure tx in the transaction list but when I switched to PURE-C-D I did not see anything there even though it is still awaiting approval.

I did the same scenario twice to make sure and got the same result both times.

Details:
A-B-P Multi (Rococo)
5DNztv1wsBfYuH22FEnjZJRymDFTAP9nfndyVYn6dVWcvzTa

Pure-C-D (Rococo)
5Fy9xwTKvjL5TurCEi9JkGPkT48rwQgsHdjnsE22NDApfzPy

https://rococo.subscan.io/multisig_extrinsic/8822583-2?call_hash=0xe133e6f11af51d883e56216e9e1bbb1e2920c7c6120cbb55cd469b1f95b61601

CleanShot.2024-01-18.at.23.39.10.mp4

@asnaith
Copy link
Contributor

asnaith commented Jan 19, 2024

Another scenario to consider is an A-B-P-D Multi (with David added to it).

Technically would David need to approve the creation of the pure twice? Once for themself and once as their half of PURE-CD?

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jan 19, 2024

Idk what happened, but I can see if for your account. Maybe there's an issue with the graphql setup?
Could you check the console and eventually the network tab?
image

Regarding your last comment, yes David would have to sign twice, but that wouldn't be an issue. They would sign once when the multisig is selected, once when the pure-c-d is selected. The other scenario that I'll test is when you don't put the pure as a signatory but the multisig (say you put the multi-c-d instead of the pure-c-d), it could be intentional or not, but it should work.

I'll add the callData input possibility and test it all. You're all set for testing now at least :)

@Tbaut Tbaut changed the title DeepTx Support multisigs, eventally with pure proxy in front, as a signatory of another multisig Jan 19, 2024
@asnaith
Copy link
Contributor

asnaith commented Jan 19, 2024

Unfortunately seeing the same thing my side. I do not get the blue banner above the transaction list or the createPure tx in the list.

Here's what I'm seeing in the console

Console log

13:07:48.158 This error page has no error code in its security info aboutNetError.mjs:856:13
13:07:49.211 2024-01-19 13:07:49        API/INIT: RPC methods not decorated: chainSpec_v1_chainName, chainSpec_v1_genesisHash, chainSpec_v1_properties index-bM-yxXTy.js:600:65247
13:07:49.260 Unable to map [u8; 32] to a lookup index index-bM-yxXTy.js:602:336242
13:07:49.261 2024-01-19 13:07:49        API/INIT: rococo/1006002: Not decorating runtime apis without matching versions: ParachainHost/10 (1/2/3/4/5 known) index-bM-yxXTy.js:600:65247
13:07:49.261 2024-01-19 13:07:49        API/INIT: rococo/1006002: Not decorating unknown runtime apis: 0xfbc577b9d747efd6/1 index-bM-yxXTy.js:600:65247
13:07:49.274 No local name to load index-bM-yxXTy.js:607:11331
13:07:49.781 web3Enable: Enabled 1 extension: polkadot-js/0.44.1 index-bM-yxXTy.js:607:1912
13:07:50.754 XHRPOST
https://111792a7.multix.pages.dev/undefined
[HTTP/3 405  273ms]

13:07:52.047 XHRPOST
https://111792a7.multix.pages.dev/undefined
[HTTP/3 405  72ms]

13:07:53.381 This error page has no error code in its security info aboutNetError.mjs:856:13
13:07:54.134 XHRPOST
https://111792a7.multix.pages.dev/undefined
[HTTP/3 405  111ms]

13:07:58.252 XHRPOST
https://111792a7.multix.pages.dev/undefined
[HTTP/3 405  113ms]

Post Data from the 405 in the network tab
{"query":"\n query multisigsByMultisigOrPureSignatories($accountIds: [String!]) {\n accountMultisigs(where: {signatory: {id_in: $accountIds}}, limit: 10) {\n multisig {\n address\n threshold\n signatories(limit: 10) {\n signatory {\n address\n }\n }\n }\n }\n}\n ","variables":{"accountIds":["rococo-5Fy9xwTKvjL5TurCEi9JkGPkT48rwQgsHdjnsE22NDApfzPy","rococo-5CsJysfEYedjtHHwPNxd9wGRLYLR3t4P7gLA8W4a1UXBDUyK"]}}

CleanShot.2024-01-19.at.13.27.18.mp4

@asnaith
Copy link
Contributor

asnaith commented Jan 22, 2024

@Tbaut Latest changes are working well on preview for the critical path flow 👍

Confirmed scenarios tested:

🟢 Accepting the proposal to create the pure
🟢 Accepting proposed rotations from others (if accepted in the right order)
🟢 Accepting proposed send transactions from others
🟢 Confirmed that canceling proposals from A-B-P will remove the banner of tx for Pure-C-D

Some issues that I experienced though:

  1. During a rotation initiated by Alice to remove Bob from Pure-A-B-P, when the transaction goes to Pure-C-D for approval, two issues were observed.
  • The 'proxy.removeProxy' and 'proxy.addProxy' transactions are displayed as separate banners for C-D Pure but both are shown as 'proxy.proxy.' It's unclear which is which until clicking on them.

  • Pure-C-D can then approve the removal of the old Pure-A-B-P proxy before approving the addition of the new one. If Charlie does this, and then David approves the multisig.AsMultisig, the transaction will fail with an "Error: Proxy not a proxy."

An example:
https://rococo.subscan.io/extrinsic/0x6c982352cef875a59e42f9860379ff520305c491d7832d0fc199476da7635781

  1. When only Charlie and David accounts were enabled in the Polkadot js wallet, they could only see one Pure (Pure-C-D) in the top-right dropdown, and Pure-A-B-P was not visible.

  2. Perhaps related to the above. Users connected as Charlie or David while viewing Pure-A-B-P cannot propose transactions in the UI. The icon on the Pure badge indicates they are in watch despite being a member of that Pure (by being signatories of Pure-C-D). Maybe this is related to what is being kept in local storage?

  3. There's no balance check on the "Create Transaction" modal before signing (but users will see an insufficient balance toast if they attempt to create a transaction without sufficient funds).

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jan 23, 2024

The first 3 are unavoidable for now unfortunately.

  1. Those TXs are on the blockchain. The fact that others don't see it is bc the new multisig isn't in the interface as a controller of the pure yet. If you do a proxy.proxy with the new multisig, while it hasn't gained control of the proxy.. Then it doesn't work. I can't think of an easy way to go around this very niche of a niche. The most important is that they can't shoot themselves in the foot. Your case is a little special also because you are on a 2/3 where only 1 signature from pure-c-d executes the call. Such multisigs generally have many more signatories, try with a 3/3 and this problem won't happen as in, you can sign both, and nothing will happen bc you need others signing, if they do it in order they'll be good.

  2. That's also expected. I could add somehow the linked msig, but they can also just watch it.. That's a lot less messy for the rest of the UI, bear in mind that this functionality is for a hand full of ppl. I must keep it simple.

  3. You have to think in terms of signatories. If it's a normal account, you can sign, if not, you can't. That's definitely a limitation, but again that's to avoid crazy complexity. Now facts are, in this complex msig, there are pure as signatories, but there are also normal accounts. These normal accounts should be the proposers for now, or the child msig should craft a manual transaction for the parent. In any case, it doesn't come from the parent unless you have a normal account as a signatory.

  4. There is only for 0 indeed. I'll need to add this.

All in all very valid arguments, but for the sake of complexity and the fact that this is a feature for a very niche audience (that also needs to be validated and see that it's indeed useful) I will not clutter things more beside the 4. It also means we need some kind of explanation, video, etc. This is all super advanced, but crafting the msig of msig was manual so far, which is I think a great UX enhancement for those users.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jan 23, 2024

Enhanced the mobile view a bit
image

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jan 23, 2024

I fixed the point 4. Let's get this in the hands of users and see if that adds value already

@Tbaut Tbaut merged commit aaaa221 into main Jan 23, 2024
7 checks passed
@Tbaut Tbaut deleted the tbaut-deeptx branch January 23, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A proxy as a signatory of a multisig
2 participants