-
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
Mwa 2.0 spec #521
Mwa 2.0 spec #521
Conversation
spec/spec.md
Outdated
@@ -23,6 +23,7 @@ This specification uses [semantic versioning](https://en.wikipedia.org/wiki/Soft | |||
| Version | Description | | |||
| ------- | ----------------------------------------------------------------------------------------------------------------------------------- | | |||
| 1.0.0 | Initial release version of the Mobile Wallet Adapter specification (identical to pre-release version 0.9.1) | | |||
| 2.0.0 | Initial draft of Mobile Wallet Adaper 2.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.
NIT: we should only formally rev the version on the main
branch when this is no longer in draft
spec/spec.md
Outdated
- `chain`: (optional) if set, the [chain identifier](#chain-identifiers) for the chain with which the dapp endpoint intends to interact; supported values include `solana:mainnet`, `solana:testnet`, `solana:devnet`. If not set, defaults to `solana:mainnet`. | ||
- `auth_token`: (optional) an opaque string previously returned by a call to [`authorize`](#authorize), or [`clone_authorization`](#clone_authorization). When present, the wallet endpoint should attempt to reauthorize the dapp endpoint silently without prompting the user. | ||
- `sign_in_payload`: (optional) a value object containing the payload portion of a [Sign In With Solana message](https://siws.web3auth.io/spec). If present, the wallet endpoint should present the SIWS message to the user and return the resulting signature and signed message, as described below. | ||
- `cluster`: (deprecated) (optional) if set, the Solana network cluster with which the dapp endpoint intends to interact; supported values include `mainnet-beta`, `testnet`, `devnet`. This parameter is deprecated but maintained for backwards compatibility with previous versions of the spec, and will be ignored if the `chain` parameter is present. |
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.
MAYBE: to simplify this, what if we make cluster
an alias for chain, and if both are present, cluster
is ignored? They can then take the same values (both new and legacy chain descriptors), which would simplify implementation logic
spec/spec.md
Outdated
- `address`: the address of the account that was signed in. The address of the account may be different from the provided input address, but must be the address of one of the accounts returned in the `accounts` field. | ||
- `signed_message`: the base64-encoded signed message payload | ||
- `signature`: the base64-encoded signature | ||
- `signature_type`: (optional) the type of the message signature produced. If not provided, the signature must be `"ed25519"`. |
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.
If not provided
Provided in the request, or in the response?
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 the response. Will update
spec/spec.md
Outdated
|
||
The returned `auth_token` is an opaque string with meaning only to the wallet endpoint which created it. It is recommended that the wallet endpoint include a mechanism to authenticate the contents of auth tokens it issues (for e.g., with an HMAC, or by encryption with a secret symmetric key). This `auth_token` may be used to [`reauthorize`](#reauthorize) future sessions between these dapp and wallet endpoints. | ||
If the `sign_in_payload` parameter was included but invalid: |
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 is a lot of errors, and the chances that a client can do something intelligent with these automatically isn't great. WDYT about just having a INVALID_SIWS_PAYLOAD
error, and using the error message to convey exactly what went wrong?
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.
yeah I like that
spec/spec.md
Outdated
- `max_transactions_per_request`: (optional) if present, the max number of transaction payloads which can be signed by a single [`sign_transactions`](#sign_transactions) or [`sign_and_send_transactions`](#sign_and_send_transactions) request. If absent, the implementation doesn't publish a specific limit for this parameter. | ||
- `max_messages_per_request`: (optional) if present, the max number of transaction payloads which can be signed by a single [`sign_messages`](#sign_messages) request. If absent, the implementation doesn't publish a specific limit for this parameter. | ||
- `supported_transaction_versions`: the Solana network transaction formats supported by this wallet endpoint. Allowed values are those defined for [`TransactionVersion`](https://solana-labs.github.io/solana-web3.js/modules.html#TransactionVersion) (for e.g., `"legacy"`, `0`, etc). | ||
- `features`: a list of [feature identifiers](#feature-identifiers) for the optional features supported by this wallet endpoint. |
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.
optional features
Does this mean that the solana:
namespaced features won't be in this list?
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.
that was my thought, yes. that mandatory features would not be listed here.
should mandatory features have a specific namespace? or do you think explicitly calling out mandatory vs optional features is sufficient?
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 agree, I don't think mandatory features need to be included in this list. If they are, you get into a situation where a mandatory feature is missing - in that case, what does a client do? Better if the mandatory features are assumed (based on the negotiated protocol version), since there is nothing a client can realistically do if they are missing.
spec/spec.md
Outdated
|
||
Immediately after sending the `HELLO_RSP` message, the wallet endpoint should follow up with a `SESSION_PROPS` message to convey information mandatory to the proper operation of the session, such as the negotiated version. This message consists of a JSON payload as described above.This payload is only valid during session establishment; a client receiving this at any other time should discard it and close the session. | ||
|
||
A wallet may only send this payload if the `v=` query parameter is present in the association URI presented by the client (to avoid sending it to a client that does not understand it). If the connecting dapp endpoint is using a legacy connection (no `v=` parameter present during association), the wallet should not send the `SESSION_PROPS` message to the client. If a client supports legacy connection establishment, failure to receive this session properties message indicates that the connection is a legacy connection. |
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.
A wallet may only send this payload if the
v=
query parameter is present in the association URI presented by the client (to avoid sending it to a client that does not understand it). If the connecting dapp endpoint is using a legacy connection (nov=
parameter present during association), the wallet should not send theSESSION_PROPS
message to the client. If a client supports legacy connection establishment, failure to receive this session properties message indicates that the connection is a legacy connection.
I had a hard time interpreting whether this is required or not. I'd recommend restructuring to be of the form:
"If a v=
query parameter is present in the association URI presented by the dapp endpoint, the wallet endpoint MUST send a SESSION_PROPS
message to the client immediately after the HELLO_RSP
message. If the connecting dapp endpoint runs software for a legacy version of this standard (i.e. no v=
parameter present during association), the wallet MUST NOT send the SESSION_PROPS
message to the client, and must assume that the connection is a legacy connection. If the wallet endpoint does not support legacy connections, it should close the connection immediately upon receipt of the HELLO_REQ
message."
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 update the lead-in materials of the spec to note that this is v2.0.0-DRAFT (or something similar)
TBDs: