-
Notifications
You must be signed in to change notification settings - Fork 318
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-0030 | Upgrade to latest format & propose version / extension scheme #446
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
a325ea8
to
535a5dc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
535a5dc
to
b6fccea
Compare
So, this latest version adds three things:
|
It's a good thing to get some structure for extensions to CIP-30. Though the currently suggested string array of extensions to enable, limits you from adding additional data in the enable call that might be needed. Taking CIP-0062 as an example, it now has its own enable call (that this addition is supposed to replace?) and takes an additional VotingPurpose[] in the enable call. I would like to extend {
"cip": <number>,
"ver": <number>,
...
}
The same Extension object array as above would be returned when calling Does this make sense? |
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 proposal looks fine as far as I will currently be able to understand it; I know some discussion will want to happen (above my level) but I'm approving this now based on:
- formatting improvements & compatibility with new CIP-0001 guidelines
- adding history of CIP's acceptance & implementation
- method
cardano.{walletName}.isEnabled()
answering long standing question about versioning / feature support including issues like CIP-???? | Walletapi.signTxs()
method #443
I'd advocate (rather strongly) against that, because that is the same problem we have on CIP-0030. There's no versioning scheme for CIPs. So once a CIP reaches active, we should avoid making substantial changes to it because it's now in use in real systems. CIP-0062 is arguably not yet in a proposed state, so whomever started implementing it and is going through some API breakings changes should have known that this was a possibility and the trade-offs that comes with implementing cutting-edge proposals. Thus, there's in principle no need for version numbers because CIPs do not generally have versions. They have new CIPs instead. |
Ok, I can accept the workflow of removing versioning and always requiring a new CIP for extension/changes to existing CIP. But how would you propose that additional data in enable call should be handled? EDIT |
We should then remove apiVersion from CIP-30 as well, as it makes no sense if a CIP, once merged, is final and all changes/extensions should be handled by new CIPs. |
One way to handle this would be to leave that negotiation to the extension as a separate method; For example, you call I'm not super familiar with CIP-0062, so I don't know if this would re-trigger a new user prompt, but if so, that's a slight downside UX-wise. |
Internal discussions for how to deal with the proposed workflow have resulted in I'm being ok with the PR in its current state. The only thing would be why Just as an FYI, being really more of a CIP-62 thing. I think we will instead of accepting the purpose as an input to enable specific purposes, have CIP-0062 Extension enable call (according to this PR) enable all supported governance purposes for an origin and instead add a getSupportedPurposes() where the wallet can specify the supported purposes. The apiVersion in CIP-62 will also be removed to only allow minor changes, like purpose definitions. Is this in line with the desired outcome of this PR? |
CIP-0030/README.md
Outdated
|
||
In order to initiate communication from webpages to a user's Cardano wallet, the wallet must provide the following javascript API to the webpage. A shared, namespaced `cardano` object must be injected into the page if it did not exist already. Each wallet implementing this standard must then create a field in this object with a name unique to each wallet containing a `wallet` object with the following methods. The API is split into two stages to maintain the user's privacy, as the user will have to consent to `cardano.{walletName}.enable()` in order for the dApp to read any information pertaining to the user's wallet with `{walletName}` corresponding to the wallet's namespaced name of its choice. | ||
|
||
### cardano.{walletName}.enable(): Promise\<API> | ||
#### cardano.{walletName}.enable(extensions: Extension[] = []): Promise\<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.
Should Extension
just be a string? I takes perhaps away the ability to pass more information upon requesting a new extension, which can be useful for the user to make a decision to enable it or not. For example in #296 cardano.{walletName}.governance.enable(purpose: VotingPurpose[]): Promise\<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.
Not sure to follow you? What would be the structure of that extension string?
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.
@KtorZ I think ehanoc is asking about/suggesting something like
cardano.{walletName}.enable([{ cip: "CIP-0062", parameters: { purpose: ["catalyst", "sundae"] } }]);
I like the simplicity of the string approach, since we could balloon forever on complexity here (enable by patterns, versioning, fallbacks, etc.)
That being said, I can see the UX risk if the alternative is to prompt the user twice: once to enable an extension, and another to configure that extension.
In this case, perhaps it would be sufficient for each voting purpose to be a separate CIP that you could request, but it's only one step further where that starts to break down; Imagine a hypothetical CIP that allows wallets to sign transactions without a prompt (for instant trade experiences):
cardano.walletName.enable({ cip: "CIP-XXXX", spendingLimit: { coins: 100000000, assets: {...} } })
What would be the major difference between the Experimental API and extensions? Isn't the goal anyway that most wallets support a certain extension (if popular/used a lot). If every wallet just adds whatever extensions they like it's going to be a mess for dApp developers anyway. So over time it's desired that most wallets support popular extensions, but then they should be at the base, because it's not an extension anymore then really. If extensions don't see adoption over time then they are likely dropped again, so they are "experimental". |
I really like this, particularly as the set of possible extensions is large. I wonder though, from the wallet user's perspective how they might wish to enable a subset of the extensions a dApp requested at initialisation; and maintain a whitelist of the enabled (or not to be enabled) extensions per dApp. Likewise from the dApp's perspective they may make a hard requirement on a subset of the extensions requested at initialisation. Allowing the dApp to specify a required subset of extensions and giving the wallet user the ability to selectively enable or disable extensions requested at initialisation for different dApps could provide greater flexibility and control for both parties. It could also improve the UX of a wallet, as the wallet user can choose to only enable extensions that they trust and are comfortable with. |
These are two orthogonal elements. The experimental API is (purposely) loosely specified. It's a way for wallets to provide specific features that aren't actually specified as CIP extensions. Over time, I wish we would deprecate the experimental API altogether because it creates inconsistencies in the ecosystem. It still bothers me a lot when I see a list of "supported wallets" on a DApp whereas the entire point of CIP-0030 was to remove the need for doing adhoc integration. A DApp should support any wallet that implement CIP-0030. Yet how did we actually get there? -> There are actually many divergent implementations of CIP-0030. Which is why I am proposing to freeze the current API (+ this extension mechanism) and require that all wallet (willing to follow this standard) implement at least the base API and advertise what other features they support through this extension mechanism. This is truly about features negotiation. So it becomes much more transparent for dapps and wallets alike. Instead of integrating against a wallet, a DApp can integrate against a set of interfaces. Any wallet that provide those interfaces should work. I didn't remove the experimental API because I think there's still a (narrow) use case for it. If only during development of cutting-edge features not yet fully specified; but I would encourage any wallet providers to document their interfaces as CIPs rather than shoving stuff in the experimental API so we can grow the whole ecosytem. |
I believe this to be out of scope of the current specification. CIP-0030 is a contract between dapps softwares and wallet softwares; users aren't personas of this particular subsystem. What I mean to say is that, there's nothing in the specification that constraints how wallets may want to handle what extensions they make available. So, if a wallet provider wants to leave the choice to their users through some settings dashboard they're welcomed to do so; this specification doesn't prevent it. The most naive implementation of this proposal would be for wallets to simply implement and enable all extensions they support every time. Some may want to do something more fine grained. Note that the requested extensions that the dapp specifies on initialisation is mainly to cope with possibly conflicting extensions and give a hint to the wallet to which one they should enable if they support multiple conflicting extensions. Imagine two extensions CIP-1234 and CIP-5678. The former provides a way to evaluate a transaction execution units by providing a method 'evaluateTx' in the API that returns some cpu and memory budgets. The other is an extension that performs all the phase-1 validations on a transaction (kind of dry submission) through a method also called 'evaluateTx' (!). Both extensions are mutually incompatible because they are both bound to a same endpoint with different semantics. This is where a DApp could indicate which one it wants enabled during initialisation. In practice, well obviously do our best to avoid extensions that conflicts like this, unless it is a desired outcome (eg a new version of a particular extension that provides new functionalities but with a non backward compatible interface). |
But isn't that almost unavoidable? When I look at JavaScript builtin functions you also see a list of browsers supporting that builtin or not. Extensions do not solve that problem, they just indicate better what wallets are supported by a certain dApp.
When you say freeze, you mean "forever", or under what circumstances can the base API be modified? |
And I am not saying they do!
Precisely! This is the entire goal here. Have a less ad-hoc mechanism for wallets and dapps to advertise and negotiate what features they support / need. In principle, this could now happen entirely programmatically. When writing your dApp integration, you could simply look for any available wallet, filter out those who don't support your mandatory extensions, and automatically propose them to the user while leaving the others out. As soon as one wallet brings support for a required extension, no work is needed from your dApp -> it automatically supports it. Of course, there will always be some little details about one wallet implementing something slightly differently than others. But that is something we should resolve at the specification-level. Improve the CIPs so that they lead to unambiguous implementations.
Freeze (mostly) forever yes. Have any changes be brought be new CIPs. But it's very possible that a CIP extension would override an existing method. Say that tomorrow, we change the interface to There are cases where we might want to make exceptional changes to the base CIP-0030 API; but we will have to be very careful about that to avoid breaking backward-compatibility. This is why I am keen on @Quantumplation's idea (which he proposed on another discussion thread) to have extensions be plain objects with a single key "cip" for now. For example:
This allows to possibly introduce later some extra arguments to extensions without breaking all existing interfaces. |
i find it a bit confusing to have totally different cip numbers that are basically all variations or extensions of the original one. isn't it time to group them under a main cip number and do extentions/variations on that via a minor naming scheme? like 802.11a, 802.11n, 802.11ac, 802.11ax ...? they are all describing wifi specifications/protocols/frequencies etc, but we know they all have to do with network stuff. its just an idea, but maybe we should think about it to group such things. |
b6fccea
to
039312c
Compare
Updated with recent feedback ☝️ |
@KtorZ did that latest force push include some accidental changes to the template / CIP-0001? |
@Quantumplation maybe what look like accidental changes are from these 2 already merged commits which affect incidental material?
... as far as I can tell, these commits appearing in the diff for this branch are a consequence of doing a force-push rather than re-basing this branch on the |
Nothing's accidental. Everything's planned. I think Github is just drunk here in showing the diff. |
Moved my discussion with @KtorZ into a review to track better. |
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.
Bringing in the @KtorZ discussion I was having in the main thread.
@@ -1,72 +1,93 @@ | |||
--- |
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.
@thaddeusdiamond this is kind of a nightmare for developers but it is what it is
How can you say that, and then not see the value of what we are proposing here 😅 ... ? We don't have to replicate a scheme that is a nightmare for people to use. It doesn't have to be "what it is".
Fair, but I don't see how to get past wallet-sniffing for experimental features if you want to support multiple wallets.
@thaddeusdiamond So my proposed solution would be for each addition to the Wallet API go through it's own CIP
Now I've lost you. This is exactly what is being proposed here. With the addition of a mechanism for wallets to clearly indicates which addition they implement (so that there's no reliance on some external service to tell you empirically whether a wallet supports your needed feature).
@thaddeusdiamond completely detached from any dApp developer's knowledge of the API
I think all is really needed to get you onboard here is a clear API reference on a website that fully documents the API, and the known extensions. Take a step back, and imagine that instead of calling the extensions
CIP-0093
andCIP-0123
, we call themv2
andv3
. And we just have an API reference that lists the features of v1, v2 and v3. So as a developer, you know which one you need for your use case.There's not much difference here, except that we don't have a pure sequence, but instead, a core foundation (CIP-0030) and composable extensions that can be added as needed.
This sounds much better than having to call out the extensions as was proposed in the thread above, but I do want sequencing. For instance, v1.2 should be strictly a superset (with potential deprecation) of v0.2.
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 order to have sequencing, you need to have consensus between wallet providers on the features that gets added and this is simply impossible.
Not all wallets may
(a) be okay with a particular API extensions even though it's legit and backed by many,
(b) have time / resources to keep up with every new API change.
A more modular versioning à la carte solves this as people can implement what they truly care about.
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.
Think of this less like versions of an API, and more like permissions you're requesting: some phones have cameras, NFC readers, flexible screens, etc, but others dont; so as a dApp developer, you're negotiating with the wallet for which features you can leverage, which will never be homogeneous.
Some wallets will be unable or unwilling to implement notifications, or fast-signing, or network/account switching, etc. But will be willing and able to implement other CIPs.
The fact that this supports gracefully evolving those apis as well is an additional bonus.
@KtorZ any issues with using https://www.cardano-caniuse.io maybe it can just be the start and extend it with what we need? |
No issue in using it. Rather, about keeping it up to date. In the end, it provides an idea but doesn't necessarily reflect reality. Embedding the version negotiation in the API itself solves this problem as wallet and DApp can dynamically figure out where they stand. I'd imagine cardano-caniuse to still be useful to (a) provide an overview of the extensions available and what wallet implement them and (b) still indicate information about experimental apis of wallets. |
CIP-0030/README.md
Outdated
|
||
In order to initiate communication from webpages to a user's Cardano wallet, the wallet must provide the following javascript API to the webpage. A shared, namespaced `cardano` object must be injected into the page if it did not exist already. Each wallet implementing this standard must then create a field in this object with a name unique to each wallet containing a `wallet` object with the following methods. The API is split into two stages to maintain the user's privacy, as the user will have to consent to `cardano.{walletName}.enable()` in order for the dApp to read any information pertaining to the user's wallet with `{walletName}` corresponding to the wallet's namespaced name of its choice. | ||
|
||
### cardano.{walletName}.enable(): Promise\<API> | ||
#### cardano.{walletName}.enable(extensions: Extension[] = []): Promise\<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.
I strongly recommend
request: {extensions: Extension[]}
instead. Structural arguments are easier to change or extend later than positional arguments
This has been a recurring topic regarding extensions of CIP-0030. Given that this CIP is effectively active (hence the status change in this commit), we can't simply keep modifying it because it then becomes hard for DApps to keep track of what is supported by wallet providers and what isn't. In this commit, I henceforth propose an extension scheme based off CIPs. Wallet providers are expected to support the base API given by CIP-0030 and new extensions or modifications of that API can be brought in through extension CIPs. Upon initialization, Dapps can interrogate the wallet about what CIPs extensions it supports and process or fail accordingly based on whether the wallet provides the desired functionality.
039312c
to
2c3d54f
Compare
|
||
## Data Types | ||
A string representing an address in either bech32 format, or hex-encoded bytes. All return types containing `Address` must return the hex-encoded bytes format, but must accept either format for inputs. |
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.
Why did this PR change the address format for return types from bech32 to hex-encoded? Seems unrelated to this PR
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.
according to how it's "implemented in practice": #446 (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.
Because it was indicated that this is how most wallets have implemented it today. And the goal was to reflect on the current reality.
### cardano.{walletName}.isEnabled(): Promise\<bool> | ||
Upon start, dApp can explicitly request a list of additional functionalities they expect as a list of CIP numbers capturing those extensions. This is used as an extensibility mechanism to document what functionalities can be provided by the wallet interface. CIP-0030 provides a set of base interfaces that every wallet must support. Then, new functionalities are introduced via additional CIPs and may be all or partially supported by wallets. | ||
|
||
DApps are expected to use this endpoint to perform an initial handshake and ensure that the wallet supports all their required functionalities. Note that it's possible for two extensions to be mutually incompatible (because they provide two conflicting features). While we may try to avoid this as much as possible while designing CIPs, it is also the responsability of wallet providers to assess whether they can support a given combination of extensions, or not. Hence wallets aren't expected to fail should they not recognize or not support a particular combination of extensions. Instead, they should decide what they enable and reflect their choice in the `cardano.{walletName}.extensions` field of the Full API. As a result, dApps may fail and inform their users or may use a different, less-efficient, strategy to cope with a lack of functionality. |
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.
cardano.{walletName}.extensions
Is referenced here, but it is never defined in the specification.
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.
ill make a PR to fix.
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.
Actually, I believe to just be a typo, this should reference api.getExtensions()
and not cardano.{walletName}.extensions
.
…eme (cardano-foundation#446) * Upgrade CIP-0030 to latest format & propose version / extension scheme This has been a recurring topic regarding extensions of CIP-0030. Given that this CIP is effectively active (hence the status change in this commit), we can't simply keep modifying it because it then becomes hard for DApps to keep track of what is supported by wallet providers and what isn't. In this commit, I henceforth propose an extension scheme based off CIPs. Wallet providers are expected to support the base API given by CIP-0030 and new extensions or modifications of that API can be brought in through extension CIPs. Upon initialization, Dapps can interrogate the wallet about what CIPs extensions it supports and process or fail accordingly based on whether the wallet provides the desired functionality. * Make 'enable' parameter an object to favor extensibility. * Make CIP-0030 as 'Active' in top-level README.
This has been a recurring topic regarding extensions of CIP-0030.
Given that this CIP is effectively active (hence the status change in
this commit), we can't simply keep modifying it because it then
becomes hard for DApps to keep track of what is supported by wallet
providers and what isn't.
In this commit, I henceforth propose an extension scheme based off
CIPs. Wallet providers are expected to support the base API given by
CIP-0030 and new extensions or modifications of that API can be
brought in through extension CIPs. Upon initialization, Dapps can
interrogate the wallet about what CIPs extensions it supports and
process or fail accordingly based on whether the wallet provides the
desired functionality.