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

Aligned with New CIP-30 Extension Scheme #15

Conversation

Ryun1
Copy link

@Ryun1 Ryun1 commented Mar 20, 2023

Context

Changes

  • Removed initial API components:

    • cardano.{walletName}.catalyst.apiVersion
    • cardano.{walletName}.catalyst.enable(purpose: VotingPurpose[])
  • Added:

    • api.getVotingPurposes()
  • Prose:

    • Added description of connection flow to Examples of Message Flows and Processes.
    • Added description of missing voting flow to Examples of Message Flows and Processes.
    • Supporting prose throughout .

Rendered Version

Comment on lines 268 to 272
#### api.getVotingPurposes(): Promise\<VotingPurpose>[]

Errors: [`APIError`](#extended-apierror)

This is a chance for dApps to ask wallets what `VotingPurpose`s the user wishes to allow the dApp access to. It is up to wallet implementors if they deem it necessary to prompt the user for their permission to share supported Purposes.
Copy link

Choose a reason for hiding this comment

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

As this is just defined as an endpoint to fetch supported purposes by the wallet, there is no access request needed.

Else endpoint would need to be redefined as enableVotingPurposes(purpose: VotingPurpose[]) that takes a list of purposes to enable just like previous catalyst.enable(purpose: VotingPurpose[]) call.

But I dont see this as needed, I happy with wallet just returning a list of purposes it support. Its enough that the user ask for CIP62 permission once, when calling CIP30 enable(). Else it will result in a bad UX.

So text here should be updated to remove info about extra access request.

Copy link
Author

Choose a reason for hiding this comment

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

@Scitz0

I agree -- change made.

CIP-0062/README.md Outdated Show resolved Hide resolved
CIP-0062/README.md Outdated Show resolved Hide resolved
@refi93
Copy link

refi93 commented Apr 4, 2023

unrelated to this PR but I think there is an inconsistency in how choice is currently defined:

  • choice - This value MUST match one of the available voteOptions in the proposal element. An UnknownChoiceError should be thrown if the value is not one of the valid voteOptions of the proposal.

However this is the current definition of the voteOptions field in Proposal:

voteOptions - Total number of vote options. Only present in a private/encrypted vote.

I guess the choice definition is based on an outdated definition of voteOptions when it was a list of numbers, but maybe I'm missing something. Can you update the docs to make this consistent?

1. **dApp Dialogue** - User indicates to the dApp intention of wallet connection, the user selects their chosen wallet from a list of supported ones offered by the dApp.
2. **Connection** - Using the name of the selected wallet the dApp can call [CIP-30's enable](https://github.com/cardano-foundation/CIPs/tree/master/CIP-0030#cardanowalletnameenable-extensions-extension----promiseapi) passing in this CIPs identifier: `cardano.{walletName}.enable([{"cip": 62 }])`. This should cause the wallet to ask the user to grant permission for the dApp connecting and using the CIP-62 API Extension.
3. **Purpose** - Once connection is established the dApp will want to establish the supported wallet's `VotingPurpose`s, this is achieved via `.getVotingPurposes()`. By knowing the supported purposes the dApp can correctly choose which ones to use when calling `.signVotes()` and `.submitDelegation()`.

#### Delegation

Recall from [CIP-36](https://github.com/cardano-foundation/CIPs/tree/master/CIP-0036) a registration is a self-delegation, allocating one's voting power to one's own voting key.

1. **Get Voting Key** - dApp calls the method `api.getVotingCredentials()` to return the connected wallet account's public `voteKey`.
Copy link

@refi93 refi93 Apr 5, 2023

Choose a reason for hiding this comment

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

not related to this PR, but I was just polishing HW wallet support when I realized that only Ledger HW wallet will be able to return the voting key. So this call would fail for a Trezor user even though they may want just to delegate their voting rights and for that usecase it's sufficient to get the staking key. One solution I can think of is to split this call into two, so each piece of information is available independently, i.e. getVotingKey and getStakingKey - what do you think @Ryun1 ?

Alternatively, we may make the votingKey in the returned value optional though that may not lead to good UX as it may no longer be clear why the votingKey wasn't returned as there would be no error code/message to indicate that

Copy link
Author

Choose a reason for hiding this comment

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

Copy link

@refi93 refi93 Apr 6, 2023

Choose a reason for hiding this comment

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

for the record, as we discussed that offline - it seems that the stakingKeyPub may not need to be exposed at all as the wallet can handle it internally. For the sake of constructing the delegation transaction, it's enough for the wallet to receive the staking address in the DelegatedCertficate structure and it can map it back to the respective public key - wallets need to do much heavier reverse mapping under the hood already anyway to support signing Cardano transactions with hardware wallets as hw wallets expect derivation paths, not raw addresses/keys.

If we suppose that a wallet has a single staking key, not even the staking address would be needed, but it seems to be more future proof and consistent with CIP-30 to include it as the CIP-30 API itself exposes the reward (i.e. staking) addresses as an array: https://github.com/cardano-foundation/CIPs/blob/master/CIP-0030/README.md?plain=1#L305

So to sum it up - unless there's some usecase I'm missing, I think the stakingKeyPub can be dropped from the getVotingCredentials() call altogether in which case it probably makes sense to rename the call back to getVotingKey() and the submitDelegation() call's parameter could reference just the stakingAddress which can be retrieved from the CIP-30 getRewardAddresses() call. This would also solve the problem raised in the comment above

Co-authored-by: Rafael Korbaš <rafael.korbas@gmail.com>
@Ryun1 Ryun1 closed this Dec 15, 2023
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.

3 participants