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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

CIP-0030 | Events API #151

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions CIP-0030/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ APIErrorCode {
InvalidRequest: -1,
InternalError: -2,
Refused: -3,
AccountChange: -4,
}
APIError {
code: APIErrorCode,
Expand All @@ -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.
Copy link
Contributor Author

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).

Copy link

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josemmrocha

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?

Copy link

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.


### DataSignError

Expand Down Expand Up @@ -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
Copy link

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


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:
Copy link

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 and wallet.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).

Copy link

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.

Copy link

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

Copy link

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
Copy link

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 that window.cardano is already defined, for wallet coexistence)

5) Other?

### 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.
Copy link

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.


### account_changed
Copy link

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.

Copy link

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


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.
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

#209




# Implementations

[nami-wallet](https://github.com/Berry-Pool/nami-wallet/blob/master/src/pages/Content/injected.js)