-
Notifications
You must be signed in to change notification settings - Fork 322
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 | Events API #151
Conversation
The events API was removed from CIP-30 before merging with the plan to add it in a later revision. This commit simply contains the scaffolding that was removed prior to merging cardano-foundation#88 so we can start the discussion again.
@@ -84,7 +83,6 @@ APIError { | |||
* InvalidRequest - Inputs do not conform to this spec or are otherwise invalid. | |||
* InternalError - An error occurred during execution of this API call. | |||
* Refused - The request was refused due to lack of access - e.g. wallet disconnects. | |||
* AccountChange - 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.
Do we want to treat changing accounts as still a single session with a single api
object that will just suddenly switch from which account they are pulling data from? That was the current state of the events API before. On one and it'd be easier to handle account switches, but on the other it might make gotchas for dApps if they don't listen to events well and forget data from a previous account. If we go the other way then account changes/disconnects would be basically handled the same (with different events emitted/error codes when trying to use the old api
object) but with the difference that wallets should not re-ask for user permission if the api
object invalidation was due to an account change, but would if it was due to a disconnect (depending on how the wallet implements their whitelist I guess).
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.
Imo having events would be much easier to handle. I could just subscribe to account/network change, and act accordingly (refresh the API, and get all needed data as balance, assets....). This is what I was doing for Nami and it worked really nice.
Right now with ccvault wallet following CIP-0030, I cant subscribe. And what happens is that API returns the 'account changed' error when trying to do something if I change the account with the dApp openned and with old state. I have to manage this anyways, but it is harder, as I need to add protections in every API call to handle this error (that should not be an error imo. Changing account or network is a normal behaviour from users).
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 need to add protections in every API call
In the spec so far in this PR, if we remove this error code, could the situations where you would have had to add protections potentially be a source of bugs in dApps if they're starting to mix up data between the old and new account? I made this comment just trying to find a good balance between the dev experience but also in reducing potential for confusion/bugs in dApps using this API.
What about in-progress queries (especially ones requiring user input like signing)? Should those get rejected (which error code?) or still be allowed to complete as normal?
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.
What about in-progress queries (especially ones requiring user input like signing)? Should those get rejected (which error code?) or still be allowed to complete as normal?
I think this is exactly what this error should be for.
I would like to include an Copied from Nami issue MotivationMy website https://native-asset.com/ would like access to such an endpoint since that would allow me to update the GUI whenever assets have been successfully minted/burned. For example, the app currently doesn't allow you to burn more tokens than you have in your wallet. However, this value becomes invalid after minting/burning and currently you need to refresh the entire app to get the correct values. A Proposed Function Signaturecardano.onBalanceChange((balance : value) => void) |
@honungsburk Thanks for the suggestion. When exactly would this be sent off? Would it be not until it is confirmed on-chain? Would we include pending utxo set changes? Both of these questions are also relevant for the getBalance/getUtxos endpoints too. Would we just define it to be consistent with the getbalance/getutxos calls? Would this double for just a general "the UTXO set has changed" (e.g. get sent off for any utxo changes even if the overall balance stays the same)? Also related #171 we have some discussion there (and in the original #88 PR) about what balance/utxos entail, about whether we want multiple endpoints or a parameter for reward balance/deposit/spendable balance/etc. |
I think an I would like the API to be such that you can define Example
(MIght be errors in the code but you get the gist of it) Another principle I'd like is for |
I agree. Those two are potentially going to change due to the discussion I linked before(here is an exact link to the initial discussion in the original merged PR) which could give finer granularity to dApp devs about those endpoints which is something to keep in mind.
Do we think it's necessary to have separate events or should we just have one single one? They would both be dispatched at the same time. Also as for the utxo endpoint there's the pagination issue (there's discussion in #171 about potentially changing that) about whether it makes sense to return all of them as part of the event. I guess as far as the events API goes there's no final decision about how they will be implemented so it's possible we could go with a way that doesn't return the changed utxo set and instead just returns that it has changed and lets the dApp user use the regular endpoints to figure it out, although I'm not sure if that would be significantly less convenient for dApp devs or not. My first reaction is that if we end up having multiple balance/utxo endpoints or have them have a parameter to specify that extra granularity (e.g. include reward? pending? etc) that it might make sense to have a single event that says that the UTXO set has changed and then from there you can call the relevant endpoint with the relevant parameter to get the info you want. We'll obviously want to have a general discussion on this though and have other devs (especially dApp devs) opine on this. |
Yeah, an onUtxoChange event might be a good place to start. It is easier to add endpoints rather than remove them. Something like this would work for my use case:
Should it trigger when a user change from testnet to mainnet? Should it trigger on account change? I guess they should. Out of convenience, we should keep onNetworkChange and onAccountChange since those events include more changes than just the utxo set. |
Here's my proposal: Add The actul events I would add are: For the EDIT: Events can easily be implemented with either |
@@ -84,7 +83,6 @@ APIError { | |||
* InvalidRequest - Inputs do not conform to this spec or are otherwise invalid. | |||
* InternalError - An error occurred during execution of this API call. | |||
* Refused - The request was refused due to lack of access - e.g. wallet disconnects. | |||
* AccountChange - 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.
What about in-progress queries (especially ones requiring user input like signing)? Should those get rejected (which error code?) or still be allowed to complete as normal?
I think this is exactly what this error should be for.
|
||
In addition to the API methods that are actively called, the connector also must emit the following events. All methods events are required to be implemented. | ||
|
||
TODO: event emission method? Possible methods: |
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 it's best to emit wallet_disconnected
on the wallet
object. I also think there has to be a method to unsubscribe.
Regarding the method/naming:
- To not confuse with DOM events, I would rule out
window.addEventListener
. - To not confuse with cross-window/context communication, I would rule out
postMessage
This leaves us with the following options:
wallet.on(eventName, handler): void
andwallet.off(eventName, handler): void
- probably the most used pattern besides addEventListener/removeEventListener. I personally like this one best.wallet.on(eventName, handler): Unsubscribe
,type Unsubscribe = () => void
wallet.onEvent(handler): Unsubscribe
Also, I suggest documenting that wallets will automatically unsubscribe all event listeners when disconnected (so dApps know they don't have to do that).
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.
wallet.on(eventName, handler): void and wallet.off(eventName, handler): void - probably the most used pattern besides addEventListener/removeEventListener. I personally like this one best.
👍 for this suggestion, on/off
or even addListener/removeListener
are indeed the most commonly seen practices and easy to remember.
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.
wallet.on(eventName, handler): Unsubscribe, type Unsubscribe = () => void
has a nice advantage, though - doesn't need referencing handler
to unsubscribe, which leads to an API that is easier to use and easier to follow (IMO) as there is no second method that widens initial API surface. Effectively, it is at least harder to unsubscribe if there was no subscription in the first place. Additionally - in case of testing functions depending on this API - it will be easier to fake it, e.g. by wrapping an observable.
And there are quite known APIs that utilize similar pattern:
- React in its
useEffect
hook - rxjs in
Observable
constructor
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.
@kapke imo that doesn't make sense. The reason why React and Observable does it like that is because they housekeep the event, which makes sense if you are observing the values. In React, for example, you can only (unconditionally) call useEffect
in a component/hook, which is why they need to do the clean up in that context. These limitations does not apply to the Cardano Bridge API, which is only limited to the scope and life-cycle of the window. The way that nami implements the events makes much more sense to me, by just re-using the standard Event
object.
1) Emitted event via `window.addEventListener(eventName , e => void)` | ||
2) Emitted message via `window.postMessage({eventName: string}, ...)` | ||
3) Some kind of callback registration i.e. `wallet.onDisconnect(() => void)` or `wallet.onEvent(eventName => void)` | ||
4) A combination of the two (event/message but with callback on `wallet` object as well |
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 it would be useful to have an event on cardano
object. I suggest something like this:
// cardano.{walletName}.name: String
// key is the "walletName" here
type NewWallet = { apiVersion: string; name: string; key: string; icon: string; }
cardano.on('wallet-connected', (newWallet: NewWallet => void);
alongside with:
cardano._addWallet(newWallet: NewWallet)
(to be used by wallets if they find thatwindow.cardano
is already defined, for wallet coexistence)
|
||
Emitted when the user disconnects (not just changes) their current account from the page. This means that all `api` methods will return with an `APIError.Refused` error and a new `api` object must be obtained from `wallet.enable()` to continue to interact with the user's wallet. | ||
|
||
### account_changed |
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.
We could treat account change as disconnect and connect - one thing less to handle for dApps. Wallets would have to ensure that wallet key is unique for each account.
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 argue that form the app perspective, the user is still "connected", just with different data. An account-change event would be enough to let developers know that the UI should be updated.
Also thinking of a disconnect/connect perspective, devs would show/hide connect
buttons and special related UI. If an account change would trigger that, the UI could be confusing
|
||
### wallet_disconnected | ||
|
||
Emitted when the user disconnects (not just changes) their current account from the page. This means that all `api` methods will return with an `APIError.Refused` error and a new `api` object must be obtained from `wallet.enable()` to continue to interact with the user's wallet. |
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.
Also, when removing the dApp from the whitelist, implementation should make sure to emit the event to the page only if the page origin (host) matches the removed host.
App developers should only face disconnect
events that concern their app host, and not third party hosts.
@@ -247,6 +245,30 @@ Errors: `APIError`, `TxSendError` | |||
|
|||
As wallets should already have this ability, we allow dApps to request that a transaction be sent through it. If the wallet accepts the transaction and tries to send it, it shall return the transaction id for the dApp to track. The wallet is free to return the `TxSendError` with code `Refused` if they do not wish to send it, or `Failure` if there was an error in sending it (e.g. preliminary checks failed on signatures). | |||
|
|||
|
|||
|
|||
## Events |
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 these events be exposed as an enum or constants as well? Similarly to how errors are
|
||
### account_changed | ||
|
||
Emitted when the user changes accounts (i.e. different root key pair and/or network id). The same `api` object will continue to work but will now return results based on the new account. After this event is received dApps should check `api.getNetworkId()` as changing accounts can also change the network. |
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.
After this event is received dApps should check
api.getNetworkId()
as changing accounts can also change the network.
This is extremely weird and confusing to me along with the rest of the network-id handling in the spec. Shouldn't the dapp be the one that can dictate the connector which network it works on?
Simple example, imagine a dex dapp that displays on its main page the listing of active offers existing on the chain from other users. Now to do that the dapp must know which offers to select from its database to display them. Imagine I am a developer of that dapp and I have two versions of the dapp one for the mainnet network and one for the testnet network as a dev/test version. Now on the mainnet version of the dapp I am obviously only listing the mainnet offers from the mainnet chain. And then suddenly I get an event that the user has connected or switched to a testnet account. What am I supposed to do in this situation? Redirect the user to a completely different version of the dapp that lists stuff from the testnet? What if I don't have this version publicly available? Or what if my design it to only request the connection on the latest step when the user already selected which offer they want to purchase, and then suddenly I am presented with the fact that the user account is not on the same network as the offer they have just selected? This adds incredible awkwardness to the entire communication and puts the dapps into an awful position of having to deal with the situation when the user can select the network the dapp have not been expecting, while ideally it would be encapsulated in the wallet itself.
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.
Hi. It's been a year since this PR was opened/actively discussed, and as someone trying to interface with a lot of different wallet providers, I would say events are really needed. @alessandrokonrad's suggestion makes sense, but at the very minimum, it would be great if What are the blockers here, and how could we move this forward? Is it lacking consensus, is it hard to implement, do we need a new CPS? I'm not a wallet provider, but a consumer, so not sure how I'd be able to help out, but I would really like to see this move forward, so if there's anything that could be done, please let me know. |
The events API was removed from CIP-30 before merging with the plan to
add it in a later revision. This commit simply contains the scaffolding
that was removed prior to merging #88 so we can start the discussion
again.