-
Notifications
You must be signed in to change notification settings - Fork 106
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
Align with wallet-standard WalletAccount interface #563
Conversation
spec/spec.md
Outdated
- `chains`: a list of [chain identifiers](#chain-identifiers) supported by this account. These should be a subset of the chains supported by the wallet. | ||
- `features`: (optional) a list of [feature identifiers](#feature-identifiers) that represent the features that are supported by this account. These features must be a subset of the features returned by [`get_capabilities`](#get_capabilities). If this parameter is not present the account has access to all available features (both mandatory and optional) supported by the wallet. | ||
- `label`: (optional) a human-readable string that describes the account. Wallet endpoints that allow their users to label their accounts may choose to return those labels here to enhance the user experience at the dapp endpoint. | ||
- `icon`: (optional) a relative path (from `wallet_uri_base`) to an image asset file of an icon for the account. This may be displayed by the app. |
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.
using a relative uri here like we do with the dapp identity. Wallet-standard uses a data uri with a base64 encoded image (svg, png, etc). should we align with ws and use a data uri?
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.
The icon may not be available at a URL from the base URL (it could be dynamically generated in the wallet, an NFT, etc). You may also not want it to be a URL because this allows tracking. This is why we require data URIs in the WS.
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 update the spec to allow (but not require) data URIs for all icons. We've avoided breaking backward compat so far. We can require data URIs in a future (breaking) version of the spec.
spec/spec.md
Outdated
- `chains`: a list of [chain identifiers](#chain-identifiers) supported by this account. These should be a subset of the chains supported by the wallet. | ||
- `features`: (optional) a list of [feature identifiers](#feature-identifiers) that represent the features that are supported by this account. These features must be a subset of the features returned by [`get_capabilities`](#get_capabilities). If this parameter is not present the account has access to all available features (both mandatory and optional) supported by the wallet. | ||
- `label`: (optional) a human-readable string that describes the account. Wallet endpoints that allow their users to label their accounts may choose to return those labels here to enhance the user experience at the dapp endpoint. | ||
- `icon`: (optional) a relative path (from `wallet_uri_base`) to an image asset file of an icon for the account. This may be displayed by the app. |
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 update the spec to allow (but not require) data URIs for all icons. We've avoided breaking backward compat so far. We can require data URIs in a future (breaking) version of the spec.
spec/spec.md
Outdated
- `address`: a base64-encoded public key for this account. | ||
- `display_address`: (optional) the address for this account. The format of this string will depend on the chain, and is specified by the `display_address_format` field | ||
- `display_address_format`: (optional) the format of the `display_address`. | ||
- `public_key`: (optional) a base64-encoded public key for this account. This is an alias for `address` with more accurate terminology. If present, this param should be preferred over `address`. |
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'm not sure we should add this as an alias. There's no backward-compat concern at the moment (like with the chain
/cluster
parameter to authorize
). If we do a breaking spec revision in the future, we can rename it then.
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.
removed. (and fixed the display_*
params)
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 should add some commentary about address
, in either the description or non-normative section. Could I lean on you @sdlaver for some help with the language here?
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.
How about something like:
There are 3 parameters in the result that are derived from the unique identity of the account:
address
,display_address
, anddisplay_address_format
.address
contains the public key of the account (for whatever cryptographic system is used as the authority for the account). For example, in the Solana blockchain, this is the Ed25519 public key.address
should always be encoded with base64, as this parameter is inteded for programmatic consumption rather than human consumption. In contrast, thedisplay_address
anddisplay_address_format
are representations of the account appropriate for human consumption. The contents of these two fields are chain-specific; for the Solana blockchain,display_address_format
will bebase58
, anddisplay_address
will be the public key for the account, encoded with base58. Other chains may use different encodings and/or different methods for obtaining a human-consumable address.
As an aside, this doesn't address whether the response should include a version of display_address
that is appropriate for programmatic consumption. Not every blockchain uses this (i.e. Solana does not), so it can always be addressed on a per-chain basis with spec extensions. If we find the need for it is common, we can always add it into future spec versions.
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.
Random aside - I noticed there is an extra backtick at line 452
spec/spec.md
Outdated
@@ -461,13 +467,15 @@ The `chain` parameter allows the dapp endpoint to select a specific chain with w | |||
|
|||
If both an `auth_token` and `chain` are provided by the dapp endpoint, the wallet must verify that the specified chain is the same that was associated with the specified `auth_token`. If the `chain` was not previously authorized for use with this `auth_token`, the wallet must request authorization for the new chain from the user and issue a new `auth_token` associated with the new chain. | |||
|
|||
There are 3 parameters in the result that are derived from the unique identity of the account: address, display_address, and display_address_format. address contains the public key of the account (for whatever cryptographic system is used as the authority for the account). For example, in the Solana blockchain, this is the Ed25519 public key. address should always be encoded with base64, as this parameter is inteded for programmatic consumption rather than human consumption. In contrast, the display_address and display_address_format are representations of the account appropriate for human consumption. The contents of these two fields are chain-specific; for the Solana blockchain, display_address_format will be base58, and display_address will be the public key for the account, encoded with base58. Other chains may use different encodings and/or different methods for obtaining a human-consumable address. |
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.
Need a few backticks here to add context to when a word (i.e. address
) is referring to a specific parameter
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.
on it!
rationale:
address
as a base-64 encoded public key is only applicable to Solana. Ethereum addresses use hex strings for example (0x{hexString}
)address_format
field disambiguates how the address is encoded. we can default tobase64
for backwards compatibly here.public_key
aligns more with howaddress
has been used in 1.0. loosening the required encoding foraddress
provides better multichain support.icon
good change for UX IMO. i meant to add this originally and forgot. optional so this is benign