Add wallet_rpc spec for wallet<>dapp communication #203
Conversation
wallet-api/wallet_rpc.json
Outdated
| "code": 112, | ||
| "message": "Network details are incorrect" | ||
| }, | ||
| "USER_REFUSED": { |
There was a problem hiding this comment.
Any additional errors we should return?
@dhruvkelawala @avimak @PhilippeR26 @tabaktoni @ArielElp @naorye
@stanleyyconsensys
There was a problem hiding this comment.
@amanusk , for metamask, there will be a install process, user can refuse to install, i dont have any suggestion on that error code, as it is quite unique for metamask, and there may be some more metamask "SNAP" error case
There was a problem hiding this comment.
@stanleyyconsensys I think user refused or unexpected error can be enough to cover this case. We might want to add other errors in the future
There was a problem hiding this comment.
pls mark "non generic" errors as optional
amanusk
changed the title
wallet-api/wallet_rpc.json
Outdated
| }, | ||
| "errors": [ | ||
| { | ||
| "$ref": "#/components/errors/INCORRECT_NETWORK" |
There was a problem hiding this comment.
What's the difference between an "inconsistent network" and an "incorrect network"?
Same for wallet_addStarknetChain
There was a problem hiding this comment.
I'll change the description to only return true if user agreed and network added succesfuly
wallet-api/wallet_rpc.json
Outdated
| ] | ||
| }, | ||
| { | ||
| "name": "wallet_requestAccounts", |
There was a problem hiding this comment.
Thanks looks like there is a duplicate. I'll remove
wallet-api/wallet_rpc.json
Outdated
| ], | ||
| "result": { | ||
| "name": "response", | ||
| "summary": "If adding a token is agreed, return true", |
There was a problem hiding this comment.
or if the token is already displayed.
wallet-api/wallet_rpc.json
Outdated
| ], | ||
| "result": { | ||
| "name": "response", | ||
| "summary": "If agreed, return true. Inconsistent return false", |
There was a problem hiding this comment.
or if network already listed in the wallet.
wallet-api/wallet_rpc.json
Outdated
| } | ||
| } | ||
| }, | ||
| "required": ["contract_address", "calldata", "entry_point"] |
wallet-api/wallet_rpc.json
Outdated
| "$ref": "#/components/errors/USER_REFUSED" | ||
| }, | ||
| { | ||
| "$ref": "#/components/errors/UNEXPECTED_ERROR" |
There was a problem hiding this comment.
Possibily add a specific error in case of class already declared (as the nodes returns a specific message for this case).
There was a problem hiding this comment.
Yes, I agree to @PhilippeR26 point
There was a problem hiding this comment.
The dApp could find it on the chain by itself; forwarding specific errors will open the door to a deep rabbithole :)
wallet-api/wallet_rpc.json
Outdated
| ], | ||
| "result": { | ||
| "name": "response", | ||
| "summary": "If wallet agrees to change network, return true. If the current network is selected, return true. If network is inconsistent, return false", |
There was a problem hiding this comment.
What do you mean by inconsistent network?
There was a problem hiding this comment.
Something in the network configuration format is incorrect. Happy to fix to a better summary :)
wallet-api/wallet_rpc.json
Outdated
| "$ref": "#/components/errors/USER_REFUSED" | ||
| }, | ||
| { | ||
| "$ref": "#/components/errors/UNEXPECTED_ERROR" |
There was a problem hiding this comment.
Yes, I agree to @PhilippeR26 point
wallet-api/wallet_rpc.json
Outdated
| }, | ||
| { | ||
| "name": "wallet_requestAccounts", | ||
| "summary": "Get the account addresses of the wallet active account", |
There was a problem hiding this comment.
- ", If the dApp isn't yet permissioned to receive this information, a permission request will be displayed on the wallet UI."
wallet-api/wallet_rpc.json
Outdated
| "params": [ | ||
| { | ||
| "name": "silent_mode", | ||
| "summary": "if true, the wallet will not show the wallet-unlock UI in case of a locked wallet, nor the dApp-approve UI in case of a non-allowed dApp", |
There was a problem hiding this comment.
the , nor the dApp-approve UI in case of a non-allowed dApp is misleading; this is how a dapp requests/receives the accounts permission
wallet-api/wallet_rpc.json
Outdated
| ], | ||
| "result": { | ||
| "name": "response", | ||
| "summary": "If wallet agrees to change network, return true. If the current network is selected, return true. If network is inconsistent, return false", |
There was a problem hiding this comment.
and if the user rejects the request throw an exception?
wallet-api/wallet_rpc.json
Outdated
| ], | ||
| "result": { | ||
| "name": "response", | ||
| "summary": "If adding a token is agreed, return true", |
There was a problem hiding this comment.
perhaps it would be clearer to explicitly mention that the wallet will display the approval UI here.
| }, | ||
| "errors": [ | ||
| { | ||
| "$ref": "#/components/errors/NOT_ERC20" |
There was a problem hiding this comment.
Do we return it for:
- Invalid request payload information (e.g. missing
address) - Non-ERC20 on-chain contract
- Non-deployed contract
- All cases?
There was a problem hiding this comment.
I think 2+3, we should add another error for invalid payload
wallet-api/wallet_rpc.json
Outdated
| }, | ||
| { | ||
| "name": "wallet_addStarknetChain", | ||
| "summary": "Add a new network in the list of networks of the wallet", |
There was a problem hiding this comment.
Perhaps it would be clearer to explicitly mention that the wallet will display the approval UI here
wallet-api/wallet_rpc.json
Outdated
| "summary": "Add a new network in the list of networks of the wallet", | ||
| "params": [ | ||
| { | ||
| "name": "Starknet Chain Id to add", |
There was a problem hiding this comment.
| "name": "Starknet Chain Id to add", | |
| "name": "Starknet Chain to add", |
wallet-api/wallet_rpc.json
Outdated
| "params": [ | ||
| { | ||
| "name": "invoke_transaction", | ||
| "description": "The information needed to invoke the function (or account, for version 1 transactions)", |
There was a problem hiding this comment.
not sure if I understand the or account, for version 1 transactions part
wallet-api/wallet_rpc.json
Outdated
| }, | ||
| "UNEXPECTED_ERROR": { | ||
| "code": 163, | ||
| "message": "An unexpected error occurred", |
There was a problem hiding this comment.
pls define "unexpected", and could we generalize it like "An error occurred" (UNKNOWN_ERROR) and then use it for all non-contextualized errors?
wallet-api/wallet_rpc.json
Outdated
| "type": "object", | ||
| "properties": { | ||
| "types": { | ||
| "description": "primaryType represents the top-level type of the object in the message", |
wallet-api/wallet_rpc.json
Outdated
| "NOT_ERC20": { | ||
| "code": 111, | ||
| "message": "Asset is not an ERC20" | ||
| }, | ||
| "INCORRECT_NETWORK": { | ||
| "code": 112, | ||
| "message": "Network details are incorrect" | ||
| }, |
There was a problem hiding this comment.
could be generalized under a single "Invalid request" error which could also cover for other messages like add_invoke/add_declare ..
* Change all prefixes to wallet_ * Remove deployAccount trasnaction and component * Add error for invalid input parameters
* return padded felt for transaction hash * Accept a named selector for contract calls
| } | ||
| } | ||
| }, | ||
| "required": ["contract_address", "entry_point"] |
There was a problem hiding this comment.
why not required calldata? it can be empty
| ] | ||
| }, | ||
| { | ||
| "name": "wallet_addDeclareTransaction", |
There was a problem hiding this comment.
consider renaming to wallet_declare
Co-authored-by: Toni Tabak <[email protected]>
Co-authored-by: Toni Tabak <[email protected]>
wallet-api/wallet_rpc.json
Outdated
| "types": { | ||
| "description": "Defines the types of the typed data object", | ||
| "type": "array", | ||
| "itmes": { | ||
| "$ref": "#/components/schemas/STARKNET_TYPE" | ||
| } | ||
| }, |
There was a problem hiding this comment.
types should be an object with arbitrary keys where each key maps to a StarknetType array (SNIP-12 example).
Not sure if this suggestion code is a valid way to define it (I can't findadditionalProperties on open-rpc.org but it can be found on json-schema.org and is used in one other place in the spec):
| "types": { | |
| "description": "Defines the types of the typed data object", | |
| "type": "array", | |
| "itmes": { | |
| "$ref": "#/components/schemas/STARKNET_TYPE" | |
| } | |
| }, | |
| "types": { | |
| "type": "object", | |
| "additionalProperties": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/STARKNET_TYPE" | |
| } | |
| } | |
| }, |
There was a problem hiding this comment.
Thanks @penovicp! applied this and your suggestions for SNIP 12
wallet-api/wallet_rpc.json
Outdated
| "$ref": "#/components/schemas/STARKNET_TYPE" | ||
| } | ||
| }, | ||
| "primary_type": { |
There was a problem hiding this comment.
Changing the primary type property from camel case to snake case would mean that it wouldn't align with the SNIP-12 specification.
There was a problem hiding this comment.
Same is for STARKNET_DOMAIN chain_id
| "STARKNET_CHAIN_ID": { | ||
| "title": "Starknet Chain id", | ||
| "description": "Only officially supported Starknet IDs", | ||
| "allOf": [ | ||
| { | ||
| "type": "string", | ||
| "enum": ["0x534e5f4d41494e", "0x534e5f474f45524c49", "0x534e5f5345504f4c4941"] | ||
| }, | ||
| { | ||
| "$ref": "#/components/schemas/CHAIN_ID" | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
| "STARKNET_CHAIN_ID": { | |
| "title": "Starknet Chain id", | |
| "description": "Only officially supported Starknet IDs", | |
| "allOf": [ | |
| { | |
| "type": "string", | |
| "enum": ["0x534e5f4d41494e", "0x534e5f474f45524c49", "0x534e5f5345504f4c4941"] | |
| }, | |
| { | |
| "$ref": "#/components/schemas/CHAIN_ID" | |
| } | |
| ] | |
| }, |
wallet-api/wallet_rpc.json
Outdated
| "title": "Address", | ||
| "$ref": "#/components/schemas/FELT" | ||
| }, | ||
| "SYMBOL": { |
There was a problem hiding this comment.
I would recommend renaming to TOKEN_SYMBOL to fix collision with js Symbol (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol)
| "SYMBOL": { | |
| "TOKEN_SYMBOL": { |
wallet-api/wallet_rpc.json
Outdated
| "components": { | ||
| "contentDescriptors": {}, | ||
| "schemas": { | ||
| "TXN_HASH": { |
There was a problem hiding this comment.
Should we call this padded TX hash to differentiate from api TXN_HASH which is based on FELT ?
If agreed it needs to be renamed on usage lines.
| "TXN_HASH": { | |
| "PADDED_TXN_HASH": { |
* Add revisions to TYPED_DATA * Change SYMBOL to TOKEN_SYMBOL * Change TXN_HASH to PADDED_TXN_HASH * Rename chain_id to chainId and primary_type to primaryType to fit SNIP 12 definitions
* Add error for api version not supported * Add error for account already deployed
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 52 unresolved discussions (waiting on @amanusk, @avimak, @dan-ziv, @dhruvkelawala, @naorye, @penovicp, @PhilippeR26, @stanleyyconsensys, @tabaktoni, and @vladutjs)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 51 unresolved discussions (waiting on @amanusk, @avimak, @dan-ziv, @dhruvkelawala, @naorye, @penovicp, @PhilippeR26, @stanleyyconsensys, @tabaktoni, and @vladutjs)
There was a problem hiding this comment.
Dismissed @vladutjs, and @vladutjs from 43 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dan-ziv)
This change is