-
Notifications
You must be signed in to change notification settings - Fork 357
CIP-0144? | Wallet Connector API #957
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
base: master
Are you sure you want to change the base?
CIP-0144? | Wallet Connector API #957
Conversation
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
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.
At initial review in the CIP meeting yesterday we agreed this represents long-awaited progress as a wallet API that we have needed for some time. We feel optimistic this will move ahead after some refinement so agreed to accept its CIP candidate status immediately.
@Ryun1 @Crypto2099 @perturbing we should start tagging wallet devs left & right on this, as well as kicking off some activity in the CIP Discord in both the Wallets and Query Layer threads. - Q: is the Wallets Working Group still meeting, and could they start chewing on this there?
In the meantime @nazrhom please rename the containing directory to CIP-0144
and update the link to your proposal in the original posting with the new pathname. 🎉
CIP-XXXX/README.md
Outdated
License: CC-BY-4.0 | ||
Version-Connection-API: 0.0.0 | ||
Version-CIP-30-Extension: 0.0.0 | ||
--- |
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.
--- | |
Solution-To: CPS-0010 | |
Solution-To: CPS-0012 | |
--- |
On that note, now that we are gradually evolving better mapping between CIP and CPS sets, we should make it official in the header.
@Ryun1 @perturbing @Crypto2099 I think we also need to establish a policy on how these inclusions should be performed in the other direction... I would propose this CIP also modify the target CPS to add the corresponding header line there: which would risk more merge conflicts but provide a more robust CIP repository in the long run (since these mappings would be less likely to get missed if adding one also added the other).
@Crypto2099 mentioned at yesterday's meeting that this could be a solution to more than one CPS: if there are others please by all means add them (also we could remove CPS-0012 if we're not all in agreement that this CIP "solves" that CPS).
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 will add the Solution-to: CPS-0010
header, as for CPS-12, maybe we should add the Solution-to
tag to #869 ?
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.
yes, thanks: suggested in #869 (comment).
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.
@nazrhom regarding the applicability of this CIP as a solution to CPS-0012: doesn't it provide an answer to the "open question" https://github.com/cardano-foundation/CIPs/tree/master/CPS-0012#how-can-we-encourage-wallet-developers-to-adopt-the-solution ? I would like to hear what you & @Ryun1 @Crypto2099 @perturbing think about how broadly we should be thinking in matching these CIPs to CPSs.
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 think that marking a CIP with the CPS it solves is good. But for the other direction, note that there may exist many CIP's that solve a CPS. So, we can only add a list of CIP's to a CPS which solved it, not a unique one.
Co-authored-by: Ryan <[email protected]>
…om/CIPs into nazrhom/full-data-wallet-connector
@nazrhom when your commits have stabilised please post to let us know. If you're ready, we might get it on the (deeper) Review part of the agenda for this meeting (https://hackmd.io/@cip-editors/106) where #986 would also be introduced (hopefully by you). We might try to invite a number from the working group to ensure we can start filling out review on GitHub for these related proposals. |
Hey @rphair the commits should have mostly stabilised by now. I split the CIP into 2 different ones as suggested (and integrated the CIP-139 extension directly in CIP-139). |
thanks @nazrhom - I've added it to tomorrow's CIP meeting agenda as linked above & hope you can attend for the discussion. 🙏 |
} | ||
``` | ||
|
||
This is the entrypoint to start communication with the user's wallet. The wallet should request the user's permission to connect the web page to the user's wallet, and if permission has been granted, the APIs will be made available for the dApp to use. The wallet can choose to maintain a whitelist to not necessarily ask the user's permission every time access is requested, but this behavior is up to the wallet and should be transparent to web pages using this API. If a wallet is already connected this function should not request access a second time. |
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.
but this behavior is up to the wallet and should be transparent to web pages using this API.
I like this a lot, this gives wallets the freedom to decide
|
||
When requesting the functionalities of a cip extension, dApps can optionally specify a version number for it. Every cip extension defined in the future, must also define a versioning scheme following SemVer. When the version argument is not specified, wallets should take that as the greater version they implement. In this context - and for the rest of this discussion - we will assume versions are ordered with the canonical ordering of SemVer. If the dApp requests a specific version of an extension, wallets can only accept the request if a version of the extension they implement has: | ||
|
||
- the same major value as the one requested by the dApp AND |
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.
do we need to include SemVer versioning for extensions?
I feel like this maybe adding unneeded complexity
Existing CIP30 extensions, aren't versioned, instead to replace one you must provide another CIP
I think this is mostly fine
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.
In prior discussions for both CIPs and CIP-0030 extensions for last 4 years or so, in my recollection all parties agreed each time that an integer "serial number" was both adequate and future-proof as a version ID except for those who argued that "semantic versioning is always preferred" as a general principle.
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 think we should have the option to update an API through versions. It enables a more 'agile' approach to defining and implementing extension CIPs
|
||
This appendix contains additional schemas for types that are used in the APIs. | ||
|
||
#### Connection API Data Types |
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.
really nice idea to include these :)
We will use the following schema to define operations: | ||
|
||
```json | ||
{ |
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.
{ | |
{ | |
"connectionVersion": { "type": "semVar" }, |
does it make sense to have the connection API versioned?
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 think so, I added an endpoint both to the connection API and to the different extensions to report their apiVersion
. Adding it here however, would potentially make each endpoint have it's own separate version which might be a bit too much complexity
``` | ||
{ | ||
"operation": "enable", | ||
"request": { "$ref": "#/appendix/Extensions" }, |
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.
just a thought, would there be a usecase where a dApp doesnt request a whole extension?
perhaps a dApp only wants a couple methods, maybe this would just add unneeded complexity
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.
Discussed in meeting
we can consider this in the future, or when we have more feedback from stakeholders
Returns the API version for the wallet connection API. This must correspond to the value of `Connection-API` specified in this document, appropriately transformed into a `SemVer` object. | ||
|
||
### Versioning | ||
|
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.
note to discuss using SemVar or other versioning scheme
@nazrhom now that the last hour's CIP meeting has confirmed we have a new title taxonomy as @Ryun1 has done here for CIP-0030? 🙏 @Ryun1 @Crypto2099 and other wallet reviewers... I would leave it to your consensus about whether or not "candidate" (not yet merged) CIPs should be included here.
|
} | ||
``` | ||
|
||
Returns true if the dApp is already connected to the user's wallet, or if requesting access would return true without user confirmation (e.g. the dApp is whitelisted), and false otherwise. |
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.
or if requesting access would return true without user confirmation
I find this problematic:
- This could be an issue to implement for some wallets if they decide to request user confirmation when new extension is requested (e.g. some kind of identity extension, side chain extension)
- Multi-account wallets might want to provide user with an option to connect a different account on each enable
Also I find the operation name isEnabled
a little bit misleading here, as it does not necessarily mean that enable
was called and resolved successfully.
What are the use cases of this operation?
- `InvalidRequest`: (-1) Inputs do not conform to this spec or are otherwise invalid. | ||
- `InternalError`: (-2) An error occurred during execution of this API call. | ||
- `Refused`: (-3) The request was refused due to lack of access - e.g. wallet disconnects. | ||
- `AccountChange`: (-4) The account has changed. The dApp should call wallet.enable() to reestablish connection to the new account. The wallet should not ask for confirmation as the user was the one who initiated the account change in the first place. |
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.
Which operation(s) of wallet connector API could return AccountChange
? Or is this intended for extensions?
|
||
While this CIP attempts to define an API in a transport agnostic way, implementations will be forced to pick a specific transport. In the spec we have not given specifics on how to namespace and generally structure the api in an implementation. This is both because details depend on the underlying transport that is chosen to implement the API, but also because we wanted to make backwards compatibility with the original CIP-30 proposal as easy as possible. | ||
|
||
Each transport specific implementation will make a choice on how to namespace access to the operations. In the following we give a description of how this should work for an implementation using an injected javascript object. Support for different transports can be added to this CIP in form of PRs. |
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.
Support for different transports can be added to this CIP in form of PRs.
Maybe we could keep this CIP transport-agnostic and have separate CIPs for each transport, including Injected Javascript Object
💡
|
||
[CPS-0010](https://github.com/Ryun1/CIPs/blob/cps-wallet-connector/CPS-0010/README.md) discusses several limitations of CIP-30. In this CIP we try to solve the issues pointed out in that CPS. There are still some areas to improve on: things like the event listener API are intentionally left out of this CIP to prevent bloat of scope. We welcome future CIPs, or updates to this CIP to refine any limitation that is not tackled in this document. | ||
|
||
### Use of CBOR representations |
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 find this confusing: connector API itself doesn't use any types from CIP-0116. Is this specified as a requirement for extensions?
|
||
### CPS-10 | ||
|
||
[CPS-0010](https://github.com/Ryun1/CIPs/blob/cps-wallet-connector/CPS-0010/README.md) discusses several limitations of CIP-30. In this CIP we try to solve the issues pointed out in that CPS. There are still some areas to improve on: things like the event listener API are intentionally left out of this CIP to prevent bloat of scope. We welcome future CIPs, or updates to this CIP to refine any limitation that is not tackled in this document. |
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.
There are still some areas to improve on: things like the event listener API are intentionally left out of this CIP to prevent bloat of scope
I think we should have some primitives for defining events in the base spec/cip. As far as I understand, request
has to be a plain object as you can't define callbacks in a JSON schema.
``` | ||
{ | ||
"operation": "enable", | ||
"request": { "$ref": "#/appendix/Extensions" }, |
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.
Existing dapps are always in context of a specific network. Could we add chain id in the enable request?
@@ -0,0 +1,356 @@ | |||
--- | |||
CIP: 144 | |||
Title: Wallet Connector API |
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.
Title: Wallet Connector API | |
Title: Extensible Wallet Connector Framework |
I feel maybe we can make the title a bit more specific
This CIP is defining a new framework for wallet connectors, whilst also proposing a connector API
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.
yes, that's good 🚀 but for brevity I think the shorter form (Wallet Connector -
) should still be used for its compliant extensions (e.g. #986 (review)).
just posted on Discord in response to query about this proposal's status: You are right that the work stopped in February. Without the author (@nazrhom) being involved in the submission anymore, and nobody to respond to suggestions & questions that came after then (with a few points unresolved at that time), this CIP has been stalled. Normally editors would mark a proposal In my opinion it was mature & likely would be ready for merge after it can be taken up by another advocate, even if outside of MLabs: bringing appropriate experience in the wallet industry & responsibility for some related implementations of this API. Alternatively, if a number of such devs got together to certify this proposal is ready to go "as-is", we might merge it if there are enough confirmations on GitHub... plus answers provided to at least the review points that begin here: #957 (review) |
Just to say 👋 This is a subject I’m super interested in, and really want to help improve both sides of the equation: user UX and dapps capabilities. I would be happy to contribute and help restart the discussions on this subject in a couple months. I’m currently busy on a couple things, but this is super important and will be critical for adoption and retention, both of users and devs. |
im also keen to move this along |
A wallet connector API. The goal of this CIP is to define a connection API that is independent of functionalities provided by wallet extensions. Future CIPs will introduce extensions to gain parity, and eventually surpass the capabilities offered by CIP-30 today.
See CPS-10 for motivation.
(rendered version on fork)