-
Notifications
You must be signed in to change notification settings - Fork 323
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-0062? | Cardano dApp-Wallet Web Bridge Catalyst Extension #296
CIP-0062? | Cardano dApp-Wallet Web Bridge Catalyst Extension #296
Conversation
…scription enable()
… to sign and submit delegation cert into one step api.submitDelegation
…wallet should manage these internally
CIP-0062/README.md
Outdated
|
||
Errors: `APIError`, `TxSendError` | ||
|
||
## **Delegation Cert process** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect a similar breakdown for the Voting process, it would make the motivation behind the available calls clearer to the reader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
CIP-0062/README.md
Outdated
|
||
This api being an extension of [CIP-30 (Cardano dApp-Wallet Web Bridge)](https://cips.cardano.org/cips/cip30/), expects that `cardano.{walletName}.enable()` to be enabled and added to CIP-30 whitelist implicitly. | ||
|
||
When both this API and **CIP-30** is being enabled, is up to the wallet to decide the number of prompts requesting permissions to be displayed to the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This api being an extension of CIP-30 (Cardano dApp-Wallet Web Bridge), expects that
cardano.{walletName}.enable()
to be enabled and added to CIP-30 whitelist implicitly.
Doesn't this sentence imply that the dapp has to always enable first the CIP-30 API and only then request the governance one? On the other hand this sentence
When both this API and CIP-30 is being enabled, is up to the wallet to decide the number of prompts requesting permissions to be displayed to the user.
seems to contradict that, hinting that it may not always be the case that CIP-30 is being enabled before CIP-62. Is my understanding that CIP-30 always has to be enabled before CIP-62 correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct @refi93 . I've updated this section, could you have a re-read?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm checking https://github.com/cardano-foundation/CIPs/pull/296/files#diff-d3ac75d45e4e621f6ad95567378e0dc2b671f44d3c32425dda9ca08210e18876R265-R272 but I didn't notice a difference, am I checking perhaps the wrong commit/branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this sentence imply that the dapp has to always enable first the CIP-30 API and only then request the governance one? On the other hand this sentence
Yes, it ties to explain that CIP-30 should be enabled as well, if not so already.
Maybe Is there a reason why it shouldn't?
Is my understanding that CIP-30 always has to be enabled before CIP-62 correct?
Yes, that was the idea.
Would it be more clear if i were to remove the section that states:
When both this API and CIP-30 is being enabled, is up to the wallet to decide the number of prompts requesting permissions to be displayed to the user.
CIP-0062/README.md
Outdated
proposal information. Include the range of options we can use to vote. This defines the allowed values in `choice`. | ||
|
||
``` | ||
interface Proposal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will there be an API where the wallet could fetch the proposal metadata to show something more user friendly in the confirmation prompt to the user than a list of numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see; this would definitely be easier to implement on the client side.
About some sort of Map<string,string> // <planId, proposalTitle> ?
106ff34
to
da063a4
Compare
Minimum changes to generalize description and explains how to use it in any voting system (not just catalyst). Small fixes, typos.
@rphair Rationale section in now and a few updates. |
Co-authored-by: Rafael Korbaš <[email protected]>
Thanks: Rationale looks good. There have been a few changes to the document format since you originally submitted your proposal and it would be helpful if you could edit to conform to them while this document is still fluid: New header preamble (https://github.com/cardano-foundation/CIPs/tree/master/CIP-0001#header-preamble) has some fields added & others removed. All top level headings like Abstract, Motivation, Specification, etc. should be H2's (markdown: All proposals must have a Path to Active, even if already active & completed (in that case having a description of the path taken so far). This section & its two sections (acceptance & implementation) are described here (there are some live examples in the CIP archive already): https://github.com/cardano-foundation/CIPs/tree/master/CIP-0001#status-proposed |
Errors: [`APIError`](#extended-apierror), [`TxSignError`](#extended-txsignerror) | ||
|
||
* `votes` - an array of up to 10 votes to be validated with the wallet user and, if valid, signed. | ||
* `settings` - *Optional*. Settings which are universally applicable to all `votes` being signed. This is a json string. The fields within the settings json string depend on the format of the vote record. The wallet does not need to interpret this field and should pass it unchanged to the logic used to format the vote transaction. However, to support multiple future vote transaction formats, the `settings` must contain at least: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the wallet doesn't validate the purpose in settings, can't a malicious actor abuse that to pass a different purpose than the one whitelisted by the wallet, causing it to sign a vote for a different purpose? If so, I'd consider removing the purpose from the settings and passing it separately, or at least specifying that the wallet should validate that field in the settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though looking at the js bidings lib to sign votes, I don't see the purpose
field being used at all: https://github.com/input-output-hk/catalyst-core/blob/f5865be9a9d227e975689e32e6c03b5b34ff5d96/src/chain-wallet-libs/bindings/wallet-wasm-js/js-test/test/vote_cast.js#L10 - what's the benefit of passing purpose
in the settings
parameter then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent of including the purpose here is to allow wallets to validate. Wallets should be able to show the supplied purpose of a vote to user at signing time for them to validate.
…01-refactor Refactor to fit the new CIP-0001 template
Amendments from discussion on 01/26
Rescope CIP-62 to be Catalyst Focussed
Motivation for last two commits:
|
|
||
#### Voting | ||
|
||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo?
|
||
To provide [CIP-36](https://cips.cardano.org/cips/cip36/) style governance specific functionality for Catalyst to wallet's and expose such API to the dApps (i.e Catalyst Voting Centers). | ||
|
||
This also addresses some short-comings of [CIP-30](https://cips.cardano.org/cips/cip30/); which signData can only be done by known an address; This signature is not relevant to a specific address, nor the dApp will know an address attached to the voting key. The voting key derivation is defined in [CIP-36](https://cips.cardano.org/cips/cip36/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo? "by known an address"
At the request of the Catalyst team, I am closing this CIP for now. |
This proposal can be seen as an "extension" of the existing CIP-30 (Cardano dApp-Wallet Web Bridge) to add voting delegation functionality spec'ed in CIP-36 (Catalyst/Voltaire Registration Transaction Metadata Format - Updated).
This extension also defines the API required for implementation by wallet providers to submit votes to Jormungandr.
see rendered Markdown