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-0095? | Web-Wallet Bridge - Governance #509

Merged
merged 35 commits into from
Sep 19, 2023

Conversation

Ryun1
Copy link
Collaborator

@Ryun1 Ryun1 commented Apr 27, 2023

These definitions extend CIP-30 | Cardano dApp-Wallet Web Bridge to provide support for CIP-1694 | A First Step Towards On-Chain Decentralized Governance focussed web-based stacks. Here we aim to support the requirements of the Conway Ledger era, this specification is based on the Draft Conway Ledger Era Specification.

For additional discussion see wallets-sanchonet channel in the IOG Technical Discord to view you have to opt-in to the SanchoNet group via the start-here channel. This channel should compliment the technical discussions on Github rather than replace them - I will ensure any major discussions on Discord are summarized here.

Major Changes


Rendered proposal in branch

@Ryun1 Ryun1 added the Category: Wallets Proposals belonging to the 'Wallets' category. label Apr 28, 2023
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.

To me this looks future-proof enough to handle other governance systems than CIP-1694 (as long as the Ledger components are compatible). I won't be able to confirm or dispute that all technical details are in order, but this looks to be the result of solid development and cooperation.

Even at this early review stage, I think it could already serve a guide for governance dApps to start development.

Since I can't do much more than confirm its consistency with CIP-0030, I would be happy to sign off on it as soon as the sections marked // todo are completed, to leave @KtorZ and/or @SebastienGllmt free to confirm the technical rigour.

I understand balancing this draft with the evolving #380 will probably take as much time as the latter PR, and will look forward to reading updates here in the meantime. ✔️

CIP-XXXX/CIP-XXXX.md Outdated Show resolved Hide resolved
CIP-XXXX/CIP-XXXX.md Outdated Show resolved Hide resolved
CIP-XXXX/CIP-XXXX.md Outdated Show resolved Hide resolved
CIP-XXXX/CIP-XXXX.md Outdated Show resolved Hide resolved
Ryun1 and others added 5 commits May 1, 2023 20:45
Co-authored-by: Robert Phair <rphair@cosd.com>
Co-authored-by: Robert Phair <rphair@cosd.com>
Co-authored-by: Robert Phair <rphair@cosd.com>
@Ryun1 Ryun1 changed the title CIP-???? | Cardano dApp-Wallet Web Bridge Governance Extension CIP-0095? | Cardano dApp-Wallet Web Bridge Governance Extension May 5, 2023
Copy link
Contributor

@flyerman flyerman left a comment

Choose a reason for hiding this comment

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

💡 Would it be preferable to move the building of the transactions out of the wallet and onto the application? And provide a support library for building the TX alongside the new data types.

Reducing the burden on the wallet will improve adoption. As well as keeping the wallet to its current primitives: inspect, sign and submit.

The CIP extension would then become something like:

api.getDRepKey(): Promise<PubDRepKey>
api.getActiveStakeKeys(): Promise<PubStakeKey[]>

api.submitDelegation(tx): Promise<txHash>
api.submitDRepRegistration(tx): Promise<txHash>
api.submitDRepRetirement(tx): Promise<txHash>
api.submitVote(tx): Promise<txHash>
api.submitGovernanceAction(tx): Promise<txHash>

each submitXXX(tx) would inspect the transaction, present the relevant data to the user, sign and submit.

I expect the key material and way of signing to rarely change. Unlike the transaction building part, governance flags, fields or parameters. Those section of the protocol are more likely to evolve. And that's why it could be more pertinent to place it in an application helper library for faster iterations and upgrades.

Whenever an application submit a transaction that follow a later protocol version than what the was implemented by the wallet, the wallet can still display the backward compatible portions of the transaction. Indicates that the TX contains extra fields, possibly even display a unserialized representation of those new fields. And the wallet could still provide the ability to sign and transmit granted proper disclaimer and checkboxes for user consent.

Which bring me to a major concern:
🚧 We are missing an API version. That's essential for enabling a rolling deployment of newer versions of the API.
And eventually a set of rules for backward compatibility.

@Ryun1
Copy link
Collaborator Author

Ryun1 commented Jun 6, 2023

Hey @flyerman thank you for your review/comment - much appreciated!

Following our discussion, I think moving the transaction construction burden from wallets to the application is a good idea. As you mentioned this brings a number of benefits:

  • Reduces complexity for wallet implementers.
    • They do not need to know how to construct these new transactions/certs.
    • They do not need to include complex error handling.
  • Keeps "the role of the wallet" to it's primitives of inspect, sign and submit.
    • We move application specific complexities to the application.
  • Aids iterative updates, all wallet implementers do not need to update if the format of these transactions is adjusted.
    • By utilizing a common transaction building library and maintaining that we avoid the need for wallets to update their implementations.
    • This benefit is immediately relevant as these new certificates/transactions are being developed for the Conway era.

I will work on an update to this specification to match the described changes.

We are missing an API version.

Yes, this was done intentionally, and perhaps poorly communicated within this specification (I will add). My intension for this design was to avoid some of the issues that were present with the versioning of CIP-0030. I aim to avoid the use of numbered versioning because I do not deem it necessary.

Whilst this CIP is in it's unmerged proposed state, it remains very fluid and substantial changes can happen, so I would advise against any implementation. Once more feedback is received, maturing this design I think implementations can emerge alongside this proposal's merger into the CIPs repo. Once merged only small necessary changes should be made, ideally in backwards compatible fashion. This alongside mature implementations should, move this proposal to an active state where changes cannot be made. If any large changes are needed once active then a new proposal should be made to replace this one. This I believe aligns with the (new) extendibility design of CIP-0030.

I would be keen to hear feedback on this "versioning" (or lack there of) approach. 😎

@rphair rphair mentioned this pull request Jun 6, 2023
@flyerman
Copy link
Contributor

flyerman commented Jun 7, 2023

If any large changes are needed once active then a new proposal should be made to replace this one.

Sure that's one way to do it. Versions of the spec will still be there regardless. They won't be explicitly named. You would only introduce backward compatible changes. And you would need to change the namespace for backward incompatible changes. Applications will rely on heuristics to detect the capabilities of the wallet and which version of the API to call.

This I believe aligns with the (new) extendibility design of CIP-0030.

We had a versioning issue with CIP-30. Many Dapps implemented a draft of it that was using cbor encoding. Lace had implemented the finalized version of CIP-30 that had been changed to use bech32 encoding. Upon releasing Lace we realized that:

  1. too many dapps are implementing a previous draft version of CIP-30
  2. applications have no way to detect if the wallet is expecting cbor or bech32
  3. we reverted Lace and the CIP-30 spec to a previous version
  4. we regretted that CIP-30 did not have a version number for its spec to manage this change

@gitmachtl
Copy link
Contributor

gitmachtl commented Jun 8, 2023

@Ryun1 @rphair Why is there a need to introduce another derivation path for a voting key? We introduced m / 1694' / 1815' / account' / chain / address_index with CIP36/CIP62. This is already implemented also in the HW-Wallets. You can simply use another index/chain number if you wanna have more Voting-Keys. I don't see the need to introduce another derivation path for that tbh.

Is it just to differentiate from Catalyst? So a Catalyst dRep Key - which is also a normal voting key - is than different from such a new dRep Key? It really starts to get confusing i guess.

Also, please always keep in mind, we have WebWallets, CLI-Wallets and HW-Wallets.
No mather what we come up with, there can't be any exclusion, so it must work for all kinds of wallets.

Doing implementations especially on the HW-Wallet front takes always a lot longer (usually) than on just the software side (Web and CLI). It also blows up the app on the hw-wallet more and more. Currently an App-Splitting is going underway, but cutting of some users because they can't afford the higher priced HW-Wallet to load the App thats supports its is a bit difficult imo.

Don't wanna blame anyone, but it becomes a bit of a pattern with CIPs imo. We introduce new ones, make them super open for extensions instead of keeping them focused on a single task. After a while, a new CIP comes around because the previous was not open enough to cover the new stuff, and so a new one is created with the attempt to make it super open so it can be extended... 🤣

@Ryun1
Copy link
Collaborator Author

Ryun1 commented Jun 8, 2023

Hey @gitmachtl thanks for great comment (as always :) ). These are good questions and I will add discussion of these to the rationale.

I don't see the need to introduce another derivation path for that tbh.

This could be an alternative approach. The issue I see with this is that we would be forcing every tooling provider to support CIP-36 and since CIP-36 is not entirely Voltaire/CIP-1694 it could make things more confusing for people (confusing Catalyst and Voltaire).

Is it just to differentiate from Catalyst?

Yup the primary reason I chose to separate the DRep key here from the CIP-36 Vote key used in Catalyst is so that both keys have clear and explicit usage. In essence, I am trying to avoid potential mistakes from confusion. As (CIP-95) DRep keys are used for more than just voting. I know originally the intension was for the CIP-36 key to be used in both Voltaire and Catalyst, but due to Voltaire design decisions (CIP-1694) that did not happen unfortunetly.

It really starts to get confusing i guess.

Yes I totally see the confusion, I have really tried with the naming here to clearly differentiate. But (unfortunately) we have DReps in both Catalyst and Voltaire - perhaps remaining the key here will help avoid confusion?

Furthermore as noted in the open questions, I think it may make sense to lift this DRep key from CIP-95 and place it into CIP-1852, since it is likely to be used in a much wider range of tools than CIP-36 keys. This may help it become a "first class key" similar to Stake and Payment keys. This is probably warranted as it this DRep key is represented in the Conway Ledger here.

No mather what we come up with, there can't be any exclusion, so it must work for all kinds of wallets.

I completely agree, I would be more than keen to here feedback from CLI-Wallets and HW-Wallets. I have reached out to some for comments. Probably lifting this key from a software-wallet focussed spec may make sense?

It also blows up the app on the hw-wallet more and more.

Yes, I am aware of this and its an issue/ design decision that needs to be addressed and we have been in contact with the relevant parties to discuss our approach in relation to Cardano's Age of Voltaire.

Don't wanna blame anyone, but it becomes a bit of a pattern with CIPs imo.

Yes I have noticed this too, sometimes there is a lack of a larger vision (I think I could be guilty for sure)... I think I need to put on my CIP editor hat and pay more attention to this issue. I appreciate you calling this out.

@gitmachtl
Copy link
Contributor

gitmachtl commented Jun 8, 2023

This could be an alternative approach. The issue I see with this is that we would be forcing every tooling provider to support CIP-36 and since CIP-36 is not entirely Voltaire/CIP-1694 it could make things more confusing for people (confusing Catalyst and Voltaire).

Only thing to thing about is to reuse the already implemented derivation path for a voting key. We had this discussion in the past to make it open as a general vote thing if you remember, and to remove all catalyst related references. Like they are also gone in the cardano-hw-cli and refered to just as "vote"

cardano-hw-cli vote registration-metadata
usage: cardano-hw-cli vote registration-metadata [-h] [--mainnet [NETWORK]] [--testnet-magic [NETWORK]] [--vote-public-key-jcli VOTEPUBLICKEYS] [--vote-public-key-string VOTEPUBLICKEYS] [--vote-public-key-hwsfile VOTEPUBLICKEYS]
                                                 [--vote-public-key-file VOTEPUBLICKEYS] [--vote-weight VOTEWEIGHTS] --stake-signing-key-hwsfile HWSTAKESIGNINGFILEDATA --payment-address PAYMENTADDRESS --nonce NONCE
                                                 [--voting-purpose VOTINGPURPOSE] [--payment-address-signing-key-hwsfile PAYMENTADDRESSSIGNINGKEYDATA] --metadata-cbor-out-file OUTFILE [--derivation-type DERIVATIONTYPE]
                                                 {} ...

But i understand your argument, its just a bit strange to me to reinvent the wheel again and again. The derivation path for CIP36/62 with the 1694 was choosen to do it, thats why we have the 1694 in it 😄

Yes I have noticed this too, sometimes there is a lack of a larger vision (I think I could be guilty for sure)... I think I need to put on my CIP editor hat and pay more attention to this issue. I appreciate you calling this out.

No you're not more guilty than me or any other one. But i noticed it. Some developments are slow, some are fast. Some are overtaking others. Its just i thought that especially with voting keys, we were at a point to say: "ok, you derive your voting key via this derivation path". And thats it, how to use that key is another context. From the enduser pov, its confusing. Because why can't i use my Voting Key i use to vote on Catalyst also for Voltaire Votings, etc...

I am sorry if my text made you feel that i would attack you, thats not the case at all. Just wanna bring up thoughts i have.

@Ryun1
Copy link
Collaborator Author

Ryun1 commented Jun 8, 2023

We had this discussion in the past to make it open as a general vote thing if you remember, and to remove all catalyst related references.

Yes, I do remember, this is a good point. If I remember correctly I think this was partly due so other organisations beyond Catalyst could use the CIP-36 standard. But yup, this is a good point.

The derivation path for CIP36/62 with the 1694 was choosen to do it, thats why we have the 1694 in it 😄

Yes I know! seems quite (in)convenient.

I am sorry if my text made you feel that i would attack you, thats not the case at all. Just wanna bring up thoughts i have.

Not at all @gitmachtl - I'm always keen to hear your feedback 😎.

@Ryun1
Copy link
Collaborator Author

Ryun1 commented Jun 20, 2023

As discussed in CIP Editors Meeting 68 a review from yourself @KtorZ would be appreciated.

Copy link
Contributor

@flyerman flyerman left a comment

Choose a reason for hiding this comment

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

NIT: see comments below

CIP-0095/README.md Outdated Show resolved Hide resolved
CIP-0095/README.md Outdated Show resolved Hide resolved
CIP-0095/README.md Outdated Show resolved Hide resolved
@Ryun1
Copy link
Collaborator Author

Ryun1 commented Aug 4, 2023

After a review and some thinking, I will include DRep Key in CIP-1852, but it will also get it's own CIP matching how stake keys were rolled out, see CIP-0011.

@gitmachtl
Copy link
Contributor

gitmachtl commented Aug 7, 2023

@Ryun1 shouldn't there be some mentions about to also accept ed25519e keys - if directly derived from mnemonics and the drep path?

Also, the CIP and/or the cli implementation should be in line when it comes to the type naming and description i think. I mentioned this here: IntersectMBO/cardano-cli#128 (comment)

CIP-0095/README.md Outdated Show resolved Hide resolved
@Ryun1
Copy link
Collaborator Author

Ryun1 commented Sep 8, 2023

2023.08.09 Sanchonet-Wallets Open Hour

Questions:

  • What does the active mean in .getActivePubStakeKeys()?
  • Should wallets be able to pass inactive stake keys?

Result:

  • Split .getActivePubStakeKeys() into two separate endpoints: .getRegisteredPubStakeKeys() and .getUnregisteredPubStakeKeys()
  • See d02b02f.

Motivation

  • This delineation was made to allow wallets without active stake keys to communicate this to dApps.
  • We chose to create two endpoints to match the flow of .getUsedAddresses() and .getUnusedAddresses() from CIP-30.

@Ryun1 Ryun1 mentioned this pull request Sep 9, 2023
@Ryun1 Ryun1 changed the title CIP-0095? | Cardano dApp-Wallet Web Bridge Governance Extension CIP-0095? | Web-Wallet Bridge: Governance Sep 17, 2023
@Ryun1 Ryun1 changed the title CIP-0095? | Web-Wallet Bridge: Governance CIP-0095? | Web-Wallet Bridge | Governance Sep 19, 2023
@Ryun1 Ryun1 changed the title CIP-0095? | Web-Wallet Bridge | Governance CIP-0095? | Web-Wallet Bridge - Governance Sep 19, 2023
@Ryun1 Ryun1 changed the title CIP-0095? | Web-Wallet Bridge - Governance CIP-0095? | Web-Wallet Bridge | Governance Sep 19, 2023
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.

Based on recap of everything so far in the currently ongoing CIP meeting I would be happy to see this go ahead. There is a plan in progress where a dRep key definition may be moved to a separate CIP but it's marked as "todo" in the document and implementations for SanchoNet can proceed without that separation & I believe we shouldn't wait for that before merging this.

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.

I believe that this CIP is ready and acceptable for merging as written. I have suggested that we again reiterate that the key derivation for dRep keys will be moved to its own separate CIP process and so integrators should be mindful that the key derivation paths are subject to change but otherwise we need to go ahead and get wallets implementing this logic so that we can push SanchoNet and the Voltaire era forward for the everyday users.

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.

Also would add that #509 (review) is a good summary of the consensus at today's CIP Editors' meeting.

@rphair rphair merged commit bff9379 into cardano-foundation:master Sep 19, 2023
Ryun1 added a commit to Ryun1/CIPs that referenced this pull request Nov 17, 2023
* Governance wallet connector first draft

* Small formatting

* Small formatting adds

* Update CIP-XXXX/CIP-XXXX.md

Co-authored-by: Robert Phair <rphair@cosd.com>

* Update CIP-XXXX/CIP-XXXX.md

Co-authored-by: Robert Phair <rphair@cosd.com>

* Update CIP-XXXX/CIP-XXXX.md

Co-authored-by: Robert Phair <rphair@cosd.com>

* added Bech32 prefixes to CIP-0005

* Add open Qs from CIP editors + small formatting

* aligned with CIP-95 namining

* Added open q and fixed prefixes

* moved transaction building to the application

* Small tidy throughout

* Added flow examples and tidied

* Incorporate changes from comments

* Added discussion on reusing cip36 keys

* Added DRep ID bech 32 prefix to CIP-05

* added more rationale around CIP30 compatibility

* add notes from hackathon

* Addressing comments + hackathon adjustments

* Added description to sign endpoints

* refactor prose + tidy

* added open question

* Addressed todos in rationale

* added error codes and other small adjustments

* fix naming

* added name spacing of endpoints

* Fixed change to CIP-1852

* added versioning discussion

* misc tidy

* Update ledger design + add implementors

* responded to feedback from open hour call

* namespace and more rationale

* final adjustments

* changed naming to pipe

* added warning about DRep keys

---------

Co-authored-by: Robert Phair <rphair@cosd.com>
@rphair rphair changed the title CIP-0095? | Web-Wallet Bridge | Governance CIP-0095? | Web-Wallet Bridge - Governance Nov 27, 2023
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.