-
Notifications
You must be signed in to change notification settings - Fork 117
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
ARC-0027: Provider Message Schema #272
base: main
Are you sure you want to change the base?
ARC-0027: Provider Message Schema #272
Conversation
…stions Co-authored-by: Doug Richar <drichar@gmail.com>
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 a big big fan of this proposal.
Overall, to finalize it, maybe we could include a reference implementation for us to test ourselves?
ARCs/arc-0027.md
Outdated
@@ -0,0 +1,1611 @@ | |||
--- | |||
arc: 27 | |||
title: Web Extension Wallet Interface |
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 do you think of keeping this agnostic to the platform? We could just say Wallet Interface for instance
ARCs/arc-0027.md
Outdated
| Description | Benefits | Drawbacks | | ||
|:-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| The global `window.algorand` object is overwritten by an injected script from a web extension's content script. This overwritten object will provide features that allows the dapp to interact with the wallet which may or may not conform to previous ARCs ([ARC-0005](./arc-0005.md#specification), [ARC-0006](./arc-0006.md#specification), [ARC-0007](./arc-0007.md#specification) or [ARC-0008](./arc-0008.md#specification)). | - Dapps will not have to install any external SDKs<br/>- Dapps do not need to know which wallet the user is using (if the injected wallet conforms to [ARC-0005](./arc-0005.md#specification), [ARC-0006](./arc-0006.md#specification), [ARC-0007](./arc-0007.md#specification) or [ARC-0008](./arc-0008.md#specification)) | - In the instance of multiple web extension wallets, another wallet extension may overwrite this object.<br/>- Once overwritten, the `window.algorand` object will only service the last web extension wallet that overwrote it. | | ||
| A wallet provides a separate SDK that is installed by a dapp to interact with the wallet. | - Allows wallets full control over how dapps interact with wallets, ie. provide a bespoke UX. | - Dapps need to install each SDK separately, thereby increasing the amount of development time, and complexity, needed to implement each wallet separately. | |
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.
Wouldn't this be a single SDK for dapp developers? They would just need to comply with the API, no?
Maybe that's something could be provided in the ARC as a reference implementation. Thoughts?
ARCs/arc-0027.md
Outdated
|
||
The dapp will, itself, listen to broadcasted responses from any providers. Below is a simple diagram for the flow of data for an enable request: | ||
|
||
![Simple BroadcastChannel API using web extensions](../assets/arc-0027/simple_message_bus.png) |
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 like it, is there samples for us to include here and test?
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 am separately developing an npm package, that is essentially an SDK wrapper around this, for wallets/dApps to install for minimum code. Similar to the Algorand Provider I built way back.
The UseWallet implementation of the Kibisis wallet uses it into interact with the Kibisis web extension wallet.
ARCs/arc-0027.md
Outdated
* `reference`: | ||
- **MUST** be `arc0027:sign_txns:response` | ||
|
||
#### Sign Bytes |
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 a fan of us supporting signing for multiple purposes and arbitrary data signing. But signBytes
:
- seems to give freedom to the app to define what
purposes
this can be used for with a simple string. Seems that it can be gamed to trick users to sign things that they shouldn't, even transactions. - Doesn't consider multiple encoding schemes
- Validation of the content to be signed should be matched against a known schema?
That's one of the things i'm trying to address in ARC-52#SignMetadata is to enforce the context of what we are signing and validate it quickly from a list of possible schema templates (i.e authentication requests, messages, identity), so that the user doesn't have to understand msgpack encoding or CBOR and the wallet can still validate what is signing.
It's a longer conversation though.
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 wholeheartedly agree. I have been playing with this with Algorand Provider + Kibisis and, if I recall, Kibisis displays the data if it is:
- A valid UTF-8 string
- A parseable JWT
For example, for JWTs, if it is correctly parsed it shows this screen:
It also shows some warnings if the JWT has expired or the subject/sub of the JWT does not match the account to sign.
As you can workout, I am a fan of JWTs 😅
(It was actually the only reason I built Kibisis as I wanted to implement, for the then, app I was building to use user JWT authentication using an Algorand account)
But you are right, the schema of this function requires more work as it is not fit for consumption.
ARCs/arc-0027.md
Outdated
|
||
## Abstract | ||
|
||
Building off of the previous ARCs relating to; wallet transaction signing ([ARC-0005](./arc-0005.md#specification)), wallet address discovery ([ARC-0006](./arc-0006.md#specification)), wallet transaction network posting ([ARC-0007](./arc-0007.md#specification)) and wallet transaction signing & posting ([ARC-0008](./arc-0008.md#specification)), this proposal aims to extend and comprehensively outline a common interface between multiple web extension wallets and dapps by utilizing the pub/sub semantics of the <a href="https://developer.mozilla.org/en-US/docs/Web/API/Broadcast_Channel_API">BroadcastChannel API</a>. |
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 BroadcastChannel
is an interesting choice and would need a bit more inspection. AFAIK web extensions do not typically inject in iframes, with this approach we may be allowing embedded iframes to trigger a wallet popup as long as that origin is also open in a tab elsewhere.
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 I understand it correctly that if multiple tabs are open and thus content scripts are listening to the BroadcastChannel, then they need to avoid registering multiple times? If registering multiple times is allowed, would the content script need to synchronize internally to ensure that it deals with e.g "signTxn" only once, as to not open 10 popups for each of the 10 tabs that are open?
ARCs/arc-0027.md
Outdated
* `result.host`: | ||
- **RECOMMENDED** a URL that points to a live website | ||
* `result.icon`: | ||
- **RECOMMENDED** be a URI that conforms to <a href="https://www.rfc-editor.org/rfc/rfc2397">[RFC-2397]</a> |
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.
Can we make this mandatory? Having remote URI's here may make it harder to implement, and potentially more dangerous too.
Along with limiting the supported filetypes would be nice? 'svg+xml' | 'webp' | 'png' | 'gif'
are the ones supported by wallet-standard.
- **MUST** be a code of one of the [errors](#errors) | ||
* `message`: | ||
- **SHOULD** be human-readable to allow for display to a user | ||
* `providerId`: |
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.
Can we clarify if the providerId
is static value or whether this is allowed to be a random value every time it is registered? If it is a static value, will we maintain a list somewhere of the "taken" values which we'll respect on a gentlemen's agreement, even though collisions are very rare.
ARCs/arc-0027.md
Outdated
* `result.networks.methods`: | ||
- **SHOULD** be one or all of `enable`, `postTxns`, `signAndPostTxns`, `signBytes` or `signTxns` | ||
- **MAY** be empty | ||
* `result.host`: |
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 we mandate that this must be "https://"?
ARCs/arc-0027.md
Outdated
```json | ||
{ | ||
"$schema": "https://json-schema.org/draft/2020-12/schema", | ||
"$id": "/schemas/request-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.
Small Nit: Looks like this one should be response-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.
Thank you! One can miss stuff like this in a copy + paste 😅
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.
No worries! Trying out the specification in Kotlin/Java and was doing a lot of copy + paste myself!
@kieranroneill How far are we to move this ARC to the review stage? |
@SudoWeezy Sounds good! I have been battle testing this proposed spec in Kibisis, through AVM Web Provider and UseWallet, so I am happy to give feedback and discuss it further. |
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.
Some feedback from an issue I encountered adding a second ARC-27 extension wallet to https://github.com/TxnLab/use-wallet
ARCs/arc-0027.md
Outdated
- **OPTIONAL** if omitted, the provider **SHOULD** assume the "default" network | ||
- **MUST** be a base64 encoded hash of the genesis block of the network | ||
* `providerId`: | ||
- **OPTIONAL** if omitted, all providers **MAY** respond |
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 providerId
param needs to be required, otherwise you'll have wallets responding to enable
requests not intended for them. I can't think of a situation where this would be the desired behavior. Enable requests should be 1-to-1.
``` | ||
where: | ||
* `providerId`: | ||
- **OPTIONAL** if omitted, all providers **MAY** respond |
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.
Similar to enable
requests, disable
requests should require a providerId
and wallets should only respond to disable
requests intended for them.
Summary
This proposal intends to outline a common message schema to be used in dApp/provider (wallet) communication.
The proposal attempts to build off the fundamental work of the previous ARCs; wallet transaction signing (ARC-0005), wallet address discovery (ARC-0006), wallet transaction network posting (ARC-0007) and wallet transaction signing & posting (ARC-0008), to comprehensively outline a common message schema, and to extend this schema with an aim to allow more robustness with multiple AVM (Algorand Virtual Machine) chains.
Aside from extending the schema of the previous ARCs, this proposal also adds a couple of new methods that aim to "fill the gap". These include: