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-0104? | Web-Wallet Bridge - Account public key #588

Merged
merged 11 commits into from
May 14, 2024

Conversation

Scitz0
Copy link
Contributor

@Scitz0 Scitz0 commented Sep 3, 2023

A CIP-30 extension that allows for a DApp (if allowed) to fetch the connected account public key. Utilizes yet to-be-merged CIP-30 namespace PR #577.


(rendered proposal in branch)

A [CIP-30 extension](https://cips.cardano.org/cips/cip30/#cardanowalletnameenableextensionsextensionpromiseapi) that allows for a DApp (if allowed) to fetch the connected account public key. Utilizes yet to-be-merged CIP-30 namespace PR cardano-foundation#577.
@rphair
Copy link
Collaborator

rphair commented Sep 3, 2023

@Scitz0 looks proper so far but I'm waiting on review until #587 (review) is addressed.

@rphair rphair added the Category: Wallets Proposals belonging to the 'Wallets' category. label Sep 3, 2023
Copy link
Collaborator

@Ryun1 Ryun1 left a comment

Choose a reason for hiding this comment

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

Hey @Scitz0!

Thanks for this proposal, I am a fan of this 🤓

In general I think the motivation and rationale could be matured a bit more 🙏

See comments!

CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
CIP-XXXX/README.md Outdated Show resolved Hide resolved
Returns hex-encoded string representing cbor of extended account public key. Throws APIError if needed as defined by CIP30.

## Rationale: how does this CIP achieve its goals?
Raw cbor is returned instead of bech32 encoding to follow specification of other CIP30 endpoints. Wallets implementing this CIP should, but not enforced, request additional access from the user to access this endpoint as it allows for complete read access to account history and derivation paths.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this CIP should, but not enforced

Should they or not? IMO sharing public keys should not require permission.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit subtle, but connecting a wallet tells the dApp about all the currently used utxos and addresses of a wallet. Sharing the account public keys tells the dApp about any currently used utxos and addresses along with PAST/FUTURE addresses. It's basically asking for an updatable read-only copy of the user's wallet instead of just the current snapshot of a user's wallet.

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 think wallets SHOULD request additional access for the reason Andrew mentioned. But I wasn't sure if CIP should mention it as enforced or just as a recommendation. This is why I expressed myself as I did. In addition to past/future addresses, it will also allow you to derive not just payment and stake keys but also upcoming DRep pub keys (CIP-95).

## Motivation: why is this CIP necessary?
Normally it's up to the wallet to handle the logic for utxo selection, derived addresses etc through the established CIP-30 api. Sometimes however, dApp needs greater control due to subpar utxo selection or other specific needs that can only be handled by chain lookup from derived address(es). This moves the control and complexity from wallet to dApp for those dApps that prefer this setup. A dApp has better control and can make a more uniform user experience. By exporting only the account public key, this gives read-only access to the dApp.

## Specification
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a need to support wallet's proving ownership of the shared account key?

Perhaps CIP-30's signed data just be used (if the dApp derives payment/stake keys)?

Copy link
Contributor

Choose a reason for hiding this comment

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

This would add extra friction. All we really need here is a popup asking for permission from the user.

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 think its enough to handle it through the normal enable() call, if this extension is requested, just show enable popup to either allow or disallow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah no, what I meant was adding a separate endpoint where the wallet signs with this key to prove ownership.
But I dont think this is needed as existing signData could be used (albeit with extra steps for the verifier).

Errors: APIError

Returns hex-encoded string representing cbor of extended account public key. Throws APIError if needed as defined by CIP30.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How should this proposal be versioned? see specification from CIP-0001.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 ... any recommendation for wording on this?
I'm open to any suggestion really.

Copy link
Collaborator

Choose a reason for hiding this comment

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

something like

"This CIP is not to be versioned using a traditional scheme, rather if any large technical changes are required then a new proposal must replace this one. Small changes can be made if they are completely backwards compatible with implementations, but this should be avoided."

rphair and others added 2 commits September 10, 2023 12:44
@Scitz0 Scitz0 changed the title CIP-???? | CIP-30 ext: Account public key CIP-???? | Web-Wallet Bridge: Account public key Sep 10, 2023
@rphair rphair changed the title CIP-???? | Web-Wallet Bridge: Account public key CIP-???? | Web-Wallet Bridge | Account public key Sep 19, 2023
@rphair
Copy link
Collaborator

rphair commented Sep 19, 2023

@Scitz0 @AndrewWestberg we've had to change the delimiter character because the colon was causing YAML errors due to the mapping syntax. If you see any problems with this please post here and/or in #594.

@Scitz0
Copy link
Contributor Author

Scitz0 commented Sep 19, 2023

@Scitz0 @AndrewWestberg we've had to change the delimiter character because the colon was causing YAML errors due to the mapping syntax. If you see any problems with this please post here and/or in #594.

I saw that | character also caused some issues 😄 ... I'll just hold off on the final decision on this. 👍

@rphair
Copy link
Collaborator

rphair commented Sep 19, 2023

thanks @Scitz0 - we're trying to settle the issue now, as summarised in #594 (comment)

@rphair rphair changed the title CIP-???? | Web-Wallet Bridge | Account public key CIP-???? | Web-Wallet Bridge - Account public key Sep 21, 2023
@rphair
Copy link
Collaborator

rphair commented Oct 4, 2023

@Scitz0 the 2 pending wallet API proposals were confirmed as candidates at the CIP meeting today... congratulations & please rename your CIP content folder to CIP-0104 🤩

@rphair rphair changed the title CIP-???? | Web-Wallet Bridge - Account public key CIP-0104? | Web-Wallet Bridge - Account public key Oct 4, 2023
@Scitz0
Copy link
Contributor Author

Scitz0 commented Oct 4, 2023

Should we wait on merge until #601 is resolved?
Or we can deal with that after?

There are backslashes here, and this is how it was done in CIP30, so I just mimicked that.

@rphair
Copy link
Collaborator

rphair commented Oct 4, 2023

sure @Scitz0 I think it's reasonable to first get consensus about how to handle the backslash-escaping of the left brackets in general. Now that we have that PR on CIP-0030 I think we'll get attention from more devs including those who have worked with the affected parsers directly.

@rphair
Copy link
Collaborator

rphair commented Apr 26, 2024

@Scitz0 I've put this on next CIP meeting agenda (https://hackmd.io/@cip-editors/87) in the hope of moving this forward; I'll follow the lead of the more wallet-oriented CIP editor(s) & approve this as soon as it passes their technical review.

@rphair rphair added the State: Last Check Review favourable with disputes resolved; staged for merging. label Apr 30, 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.

Came up all hands in favour at yesterday's CIP Editors meeting. Once #588 (comment) is added I expect @Ryun1 will approve it & it would be nice for @Crypto2099 also to approve since he was happy about it at the meeting. So @Scitz0 we are looking at merging this very soon & it's still marked Last Check for the next meeting if not.

@rphair rphair requested a review from Crypto2099 May 1, 2024 10:54
@rphair rphair merged commit 98437b9 into cardano-foundation:master May 14, 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.

4 participants