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

CIP-0106? | Web-Wallet Bridge - Multisig wallets #617

Merged
merged 30 commits into from
Jun 26, 2024

Conversation

leo42
Copy link
Contributor

@leo42 leo42 commented Nov 11, 2023

A new connector specification that will allow dApps to connect with multisig wallets. And enable defi for DAOs and security conscious users.

The structure of this CIP follows CIP-30 as close as possible, to make integration by applications already using CIP-30 easy and streamlined.

I am requesting the number 130 for this CIP to make the affiliation with CIP-30 clear and easy for dev's to remember and search for. (planning on making Plutus-powered-wallet connector under CIP-230 in the future)

Rendered Version

Copy link
Collaborator

@Crypto2099 Crypto2099 left a comment

Choose a reason for hiding this comment

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

Couple of minor housekeeping suggestions here and I will expand on my thoughts in aggregate in a separate comment.

CIP-130/README.md Outdated Show resolved Hide resolved
CIP-130/README.md Outdated Show resolved Hide resolved

##### Should extensions follow a specific format?

Yes. They all are CIPs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use a bit more prose here to explain that authors wishing to extend this format should submit a new CIP and what the criteria should be for consideration by wallets to implement said extensions.

@Crypto2099
Copy link
Collaborator

  1. What is the rationale here to "replicate" the existing CIP-0030 functionality for web wallets under a new global-level namespace rather than simply creating the handful of new API calls as an extension to CIP-0030 itself? i.e.
window.cardano.broclan.enable([130])
  1. The idea of being able to use Script/Multisig addresses to interact with dApps is a good one, I would like to see more rationale and justification surrounding how dApps are expected to behave when encountering a multisig address... Is it up to the dApp to maintain the record of unsigned/unfinished multisig transactions? Should these naturally "decay" over time? Doesn't this present an issue of requiring dApps to be centralized rather than decentralized by placing this onus on the dApp to record state rather than putting the onus on the signers? Maybe a importTransaction endpoint makes sense so that members of the multisig can pass it around amongst themselves? Or the wallet provider could have a record of pending transactions somehow passed between members of the multisig?
  2. We do not allow for "reservation" of CIP numbers, regardless of rationale or justification with only a handful of very notable exceptions. You should move this file to an CIPXXXX directory and await assignment of a CIP number.

leo42 and others added 2 commits November 11, 2023 18:14
@leo42
Copy link
Contributor Author

leo42 commented Nov 11, 2023

  1. What is the rationale here to "replicate" the existing CIP-0030 functionality for web wallets under a new global-level namespace rather than simply creating the handful of new API calls as an extension to CIP-0030 itself? i.e.
window.cardano.broclan.enable([130])

Multisig Wallets cannot implement CIP-30 since it is impossible to implement the api.signTx() and api.signData() endpoints. Implementing it as an extension of CIP-30 will lead to broken applications that try to connect to the multisig wallets and use them as if they are regular wallets.

  1. The idea of being able to use Script/Multisig addresses to interact with dApps is a good one, I would like to see more rationale and justification surrounding how dApps are expected to behave when encountering a multisig address... Is it up to the dApp to maintain the record of unsigned/unfinished multisig transactions? Should these naturally "decay" over time? Doesn't this present an issue of requiring dApps to be centralized rather than decentralized by placing this onus on the dApp to record state rather than putting the onus on the signers? Maybe a importTransaction endpoint makes sense so that members of the multisig can pass it around amongst themselves? Or the wallet provider could have a record of pending transactions somehow passed between members of the multisig?

The wallet is responsible for passing the transactions among the participants, gathering the signatures, composing the final transaction and submitting it on chain, dApps need to keep track of transactions only when hidden metadata is included (only the hash is provided) since in that scenario the wallet does not have all the required information to submit the transaction

The functionality you are asking is already with "importTransaction" included with "submitUnsignedTx"

  1. We do not allow for "reservation" of CIP numbers, regardless of rationale or justification with only a handful of very notable exceptions. You should move this file to an CIPXXXX directory and await assignment of a CIP number.

Done, thank you for the clarification and sorry for the confusion.

@rphair
Copy link
Collaborator

rphair commented Nov 11, 2023

@leo42 I look forward to reading this in more detail for content. In the meantime #617 (comment) is right that we can't allow choosing CIP numbers in the present, let alone reserving them for the future: it would require a conflict resolution system that would be unmanageable because of several technical & human factors.

I have to rename it in the PR title in the meantime to avoid confusion, and also remove the redundant word "Cardano". I'm also assuming based on a quick review that, while the API strongly resembles CIP-0030, that this is a completely separate API to CIP-0030, and is therefore not an extension to it... please let me know if I'm wrong about that. cc @Ryun1

@rphair rphair changed the title CIP-130 Cardano dApp-MutlisigWallet Web Bridge CIP-???? | dApp-MutlisigWallet Web Bridge Nov 11, 2023
@leo42
Copy link
Contributor Author

leo42 commented Nov 11, 2023

@rphair you are right, as I explained in the previous commend :

Multisig Wallets cannot implement CIP-30 since it is impossible to implement the api.signTx() and api.signData() endpoints. Implementing it as an extension of CIP-30 will lead to broken applications that try to connect to the multisig wallets and use them as if they are regular wallets.

So calling it an extension is confusing.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

(#617 (comment)) What is the rationale here to "replicate" the existing CIP-0030 functionality for web wallets under a new global-level namespace rather than simply creating the handful of new API calls as an extension to CIP-0030 itself? i.e.
window.cardano.broclan.enable([130])

I would first want to seriously consider this issue on both sides... on general principle (I'm not a wallet or dApp developer) I can't imagine going forward with this as written due to practically everything from CIP-0030 being duplicated & therefore prone to divergence: not only in the documentation, but even worse in behavioural inconsistencies between conventional & multisig wallets.

I agree with that I think @Crypto2099 is suggesting: that the CIP call only for whatever minimum of functional differences is required by the multisigs.

If @leo42 it is a widespread problem that people are connecting with the wrong type of wallets then shouldn't we be able to settle on a definitive way of handling that though an initial query... even if some changes are necessary to CIP-0030 to support this?

I've put this on the Triage list for our upcoming CIP meeting (https://hackmd.io/@cip-editors/76) ... please @leo42 come to the meeting if you can, to help us all agree on the best footing to proceed with this 😎

CIP-XXXX/README.md Outdated Show resolved Hide resolved
Comment on lines 401 to 402
- [ ] Provide some reference implementation of wallet providers
- [leo42/BroClanWallet]
Copy link
Collaborator

@rphair rphair Nov 11, 2023

Choose a reason for hiding this comment

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

Suggested change
- [ ] Provide some reference implementation of wallet providers
- [leo42/BroClanWallet]
- [ ] Completed reference implementation ([leo42/BroClanWallet](https://github.com/leo42/BroClanWallet))

(continuing #617 (comment)) Hard to tell whether "BroClan Wallet" is a working implementation because of the ambiguous language... where do you still have yet to "provide" a reference implementation? in the CIP, or in software?

p.s. edit: added suggestion to answer #617 (comment)

Copy link
Contributor Author

@leo42 leo42 Nov 11, 2023

Choose a reason for hiding this comment

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

In software, I am making the CIP first to have a "blueprint" of sorts for the work that need to be done on the code.

I will be creating a reference implementation in BroClanWallet, its the next thing on the work agenda.

I am not sure how to mark that to make it more understandable.

I expect the implementation to be completed in ~1 month.

Copy link
Collaborator

@rphair rphair Nov 11, 2023

Choose a reason for hiding this comment

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

sure, I've updated the code highlight above with my suggestion... removing some extra language and unnecessary nesting.

If adoption by other wallet providers is a part of your Implementation Plan, please add some more checkboxes with the adoption plan as you see it. 🤓

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 am in contact with the Gamechanger and ADAO devs trying to get them onboard.

CIP-XXXX/README.md Outdated Show resolved Hide resolved
leo42 and others added 2 commits November 11, 2023 21:25
Co-authored-by: Robert Phair <[email protected]>
Co-authored-by: Robert Phair <[email protected]>
@leo42
Copy link
Contributor Author

leo42 commented Nov 11, 2023

@leo42 I look forward to reading this in more detail for content. In the meantime #617 (comment) is right that we can't allow choosing CIP numbers in the present, let alone reserving them for the future: it would require a conflict resolution system that would be unmanageable because of several technical & human factors.

I have to rename it in the PR title in the meantime to avoid confusion, and also remove the redundant word "Cardano". I'm also assuming based on a quick review that, while the API strongly resembles CIP-0030, that this is a completely separate API to CIP-0030, and is therefore not an extension to it... please let me know if I'm wrong about that. cc @Ryun1

You are correct, implementing as an extension would would lead to problems and it is

(#617 (comment)) What is the rationale here to "replicate" the existing CIP-0030 functionality for web wallets under a new global-level namespace rather than simply creating the handful of new API calls as an extension to CIP-0030 itself? i.e.
window.cardano.broclan.enable([130])

I would first want to seriously consider this issue on both sides... on general principle (I'm not a wallet or dApp developer) I can't imagine going forward with this as written due to practically everything from CIP-0030 being duplicated & therefore prone to divergence: not only in the documentation, but even worse in behavioural inconsistencies between conventional & multisig wallets.

I agree with that I think @Crypto2099 is suggesting that the CIP call only for whatever minimum of functional differences is required by the multisigs.

If @leo42 it is a widespread problem that people are connecting with the wrong type of wallets then shouldn't we be able to settle on a definitive way of handling that though an initial query... even if some changes are necessary to CIP-0030 to support this?

I've put this on the Triage list for our upcoming CIP meeting (https://hackmd.io/@cip-editors/76) ... please @leo42 come to the meeting if you can, to help us all agree on the best footing to proceed with this 😎

@rphair I will be happy to attend if you provide a meeting link.

@rphair
Copy link
Collaborator

rphair commented Nov 11, 2023

Discord server (CIP Editors Meetings) invitation: https://discord.gg/JtWfD4hqgz
Discord server link: https://discord.com/channels/971785110770831360
Meeting link: https://discord.com/events/971785110770831360/1164384910551818330

@Crypto2099
Copy link
Collaborator

  1. What is the rationale here to "replicate" the existing CIP-0030 functionality for web wallets under a new global-level namespace rather than simply creating the handful of new API calls as an extension to CIP-0030 itself? i.e.
window.cardano.broclan.enable([130])

Multisig Wallets cannot implement CIP-30 since it is impossible to implement the api.signTx() and api.signData() endpoints. Implementing it as an extension of CIP-30 will lead to broken applications that try to connect to the multisig wallets and use them as if they are regular wallets.

I disagree with the premise on principle. It would be quite trivial for a multi-sig wallet to only allow enable calls if the cip-130 (I know we've agreed to change the number but we'll use it for example here) extension is enabled and otherwise throw an error similar to not having a dApp account selected.

Either solution would require major overhauls for all existing dApps to account for and handle multisig wallets so it's not really a matter of one being "easier" for the dApp ecosystem to handle or not, but by making this a multisig-specific extension to CIP-30 we have less concern about maintaining feature parity when there are naturally extensions or modifications to CIP-30.

@leo42
Copy link
Contributor Author

leo42 commented Nov 12, 2023 via email

@rphair
Copy link
Collaborator

rphair commented Nov 13, 2023

@leo42 #617 (comment) Can I add changes to another CIP as part of a single pull request ?

Yes, this is common when a CIP requires changes to data structure like the registry tables in CIP-0010 & CIP-0067. But I don't know of any examples where one CIP PR has made fundamental changes to another prerequisite CIP ... which I think we are considering here.

Generally I think these types of problems are best dealt with through versioning: e.g. "v1 of CIP-0030 doesn't discriminate properly between conventional & multisig wallets, but v2 will according to CIP-0130" (again, naming this informally).

Regardless of exactly how CIP-0030 might be versioned or extended, I believe this PR for this proposal alone should be fully reviewed & merged first.... documenting any dependencies upon further modifications to CIP-0030 in its Path to Active but without modifying CIP-0030 itself... before any future PR to extend or version bump CIP-0030. Otherwise I can imagine a serious delay in adopting multisig capability as described here because of uncertainty about breaking changes.

@Ryun1 Ryun1 added the Category: Wallets Proposals belonging to the 'Wallets' category. label Nov 13, 2023
@Ryun1
Copy link
Collaborator

Ryun1 commented Nov 13, 2023

Hey @leo42

Thanks for this proposal.
I agree that such functionality should be pursued and added to a dApp-wallet connector.

Although, copying CIP30 endpoints under a new namespace I feel is a mistake.
I would be much more in favour to use the CIP-30 extension mechanism for this, this is the sort of thing the mechanism was added for.
Any potential problems of adding this as an extension I think are less consequential than trying to split this from CIP-30 which introduces unneeded fragmentation IMO.
If you strongly believe that this does not belong as a CIP-30 extension, I would implore you to check out CIP-90, which was an attempt to allow for extensions without the need to include CIP30 endpoints.

Looking forward to seeing how this proposal progresses.

@Crypto2099
Copy link
Collaborator

But it will still require some changes to CIP-30 since its specifications say that all the base endpoints need to be supported and extensions only add new endpoints.

Having the base endpoints be supported doesn't mean that they can't naturally return an error if a specific wallet (i.e. multisig, Plutus script, whatever) cannot support them as-written. I would view this more like function overloading on the part of the wallet implementers rather than complete replication.

@leo42
Copy link
Contributor Author

leo42 commented Nov 14, 2023 via email

Copy link
Contributor

@klntsky klntsky left a comment

Choose a reason for hiding this comment

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

I don't understand why is it needed to return the native script contents to dApps. As I see it, dApps should be able to work with native script addresses just fine without ever knowing who are the signatories. I think the answer to this should be in the motivation section.

When calling getCollateral, the wallet can return any of the UTxOs controlled by some of the parties as well.

The only real problem that I see in CIP-30 is that gathering of signatures can take a long time and promise-based API does not play well with page reloads.

@leo42
Copy link
Contributor Author

leo42 commented Nov 14, 2023 via email

@leo42 leo42 changed the title CIP-???? | dApp-MutlisigWallet Web Bridge CIP-???? | CIP-30 extension dApp-MutlisigWallet Web Bridge Nov 14, 2023
@leo42
Copy link
Contributor Author

leo42 commented Nov 14, 2023

I have reworked and renamed the proposal to be a CIP-30 extension.

This will provide greater maintainability and backwards compatibility as CIP-30 continues to be updated and developed.

@rphair
Copy link
Collaborator

rphair commented Nov 15, 2023

thanks @leo42, that definitely reflects the intentions of the CIP Editors' meeting today... @Ryun1 @Crypto2099 if you agree this is now properly integrated as an CIP-0030 extension, then I think it would be good to have on the agenda again next CIP meeting (# 77) to confirm the scope & keep it moving forward.

@rphair rphair added the State: Last Check Review favourable with disputes resolved; staged for merging. label May 14, 2024
@leo42
Copy link
Contributor Author

leo42 commented Jun 5, 2024

@leo42 an article from @Hornan7 that illustrates multisig configuration for dReps... perhaps these requirements would also influence the design of a multisig wallet API? (cc @Ryun1 @Crypto2099) https://forum.cardano.org/t/how-to-create-a-multi-sig-drep/130159

Yes we are fully compatible with this. I will build a dRrep management and registration portal using the connector so all users of BroCllan(and other providers that implement it) have the ability to participate in governance .(when I find some time )

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@leo42 I'm ready to approve this after 1) adjusting the Implementors: as below and 2) fixing the section headings & content as requested in #617 (comment) ... as both you & I already have for your earlier 2 CIPs (103, 104) which follow the same form.

CIP-0106/README.md Outdated Show resolved Hide resolved
CIP-0106/README.md Outdated Show resolved Hide resolved
CIP-0106/README.md Outdated Show resolved Hide resolved
rphair added a commit to rphair/CIPs that referenced this pull request Jun 22, 2024
@rphair rphair changed the title CIP-0106? | Web-Wallet Bridge - Mutlisig wallets CIP-0106? | Web-Wallet Bridge - Multisig wallets Jun 22, 2024
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@leo42 after compliance with the last round of recommendations I think this is sound enough to merge, pending @Crypto2099's final review for technical foundation + document clarity for potential implementors.

This has been at Last Check for a while, which we will again revisit in 3 days as per meeting agenda https://hackmd.io/@cip-editors/91.

@rphair rphair requested a review from Crypto2099 June 22, 2024 10:57
Crypto2099 pushed a commit that referenced this pull request Jun 22, 2024
* meeting 90: merges, promotions

* fixed spelling mistake inherited from #617 title
Copy link
Collaborator

@Crypto2099 Crypto2099 left a comment

Choose a reason for hiding this comment

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

Given that no further issues have been raised and the author also feels that this is in a state that is satisfactory, I see no blockers to prevent this from being merged in and continuing on its path to active.

@rphair rphair merged commit ff2c312 into cardano-foundation:master Jun 26, 2024
@rphair rphair removed the State: Last Check Review favourable with disputes resolved; staged for merging. label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Wallets Proposals belonging to the 'Wallets' category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants