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.

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

Add wallet_rpc spec for wallet<>dapp communication #203

Merged
merged 17 commits into from
May 27, 2024

Conversation

amanusk
Copy link
Collaborator

@amanusk amanusk commented Mar 28, 2024

This change is Reviewable

@amanusk amanusk marked this pull request as draft March 31, 2024 06:47
Copy link
Collaborator Author

@amanusk amanusk left a comment

Choose a reason for hiding this comment

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

Please review the current PR in general, hope it covers all, but could have some things missing/inaccurate
Please ACK the comments you are tagged in, we discussed this in the call, but the changes are not yet reflected

wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Show resolved Hide resolved
"code": 112,
"message": "Network details are incorrect"
},
"USER_REFUSED": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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

Copy link

Choose a reason for hiding this comment

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

pls mark "non generic" errors as optional

@amanusk amanusk requested review from dan-ziv and ArielElp April 1, 2024 11:12
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
@amanusk amanusk changed the title Add wallet rpc for wallet dapp communication Add wallet_rpc spec for wallet<>dapp communication Apr 2, 2024
},
"errors": [
{
"$ref": "#/components/errors/INCORRECT_NETWORK"

Choose a reason for hiding this comment

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

What's the difference between an "inconsistent network" and an "incorrect network"?
Same for wallet_addStarknetChain

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll change the description to only return true if user agreed and network added succesfuly

]
},
{
"name": "wallet_requestAccounts",

Choose a reason for hiding this comment

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

Is it the same in line 56?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks looks like there is a duplicate. I'll remove

],
"result": {
"name": "response",
"summary": "If adding a token is agreed, return true",

Choose a reason for hiding this comment

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

or if the token is already displayed.

],
"result": {
"name": "response",
"summary": "If agreed, return true. Inconsistent return false",

Choose a reason for hiding this comment

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

or if network already listed in the wallet.

}
}
},
"required": ["contract_address", "calldata", "entry_point"]

Choose a reason for hiding this comment

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

calldata is optional

"$ref": "#/components/errors/USER_REFUSED"
},
{
"$ref": "#/components/errors/UNEXPECTED_ERROR"

Choose a reason for hiding this comment

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

Possibily add a specific error in case of class already declared (as the nodes returns a specific message for this case).

Choose a reason for hiding this comment

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

Yes, I agree to @PhilippeR26 point

Copy link

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
],
"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",

Choose a reason for hiding this comment

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

What do you mean by inconsistent network?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something in the network configuration format is incorrect. Happy to fix to a better summary :)

wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
"$ref": "#/components/errors/USER_REFUSED"
},
{
"$ref": "#/components/errors/UNEXPECTED_ERROR"

Choose a reason for hiding this comment

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

Yes, I agree to @PhilippeR26 point

wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
},
{
"name": "wallet_requestAccounts",
"summary": "Get the account addresses of the wallet active account",
Copy link

Choose a reason for hiding this comment

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

  • ", If the dApp isn't yet permissioned to receive this information, a permission request will be displayed on the wallet UI."

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

Choose a reason for hiding this comment

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

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

],
"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",
Copy link

Choose a reason for hiding this comment

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

and if the user rejects the request throw an exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed

],
"result": {
"name": "response",
"summary": "If adding a token is agreed, return true",
Copy link

Choose a reason for hiding this comment

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

perhaps it would be clearer to explicitly mention that the wallet will display the approval UI here.

},
"errors": [
{
"$ref": "#/components/errors/NOT_ERC20"
Copy link

Choose a reason for hiding this comment

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

Do we return it for:

  1. Invalid request payload information (e.g. missing address)
  2. Non-ERC20 on-chain contract
  3. Non-deployed contract
  4. All cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think 2+3, we should add another error for invalid payload

},
{
"name": "wallet_addStarknetChain",
"summary": "Add a new network in the list of networks of the wallet",
Copy link

Choose a reason for hiding this comment

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

Perhaps it would be clearer to explicitly mention that the wallet will display the approval UI here

"summary": "Add a new network in the list of networks of the wallet",
"params": [
{
"name": "Starknet Chain Id to add",
Copy link

Choose a reason for hiding this comment

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

Suggested change
"name": "Starknet Chain Id to add",
"name": "Starknet Chain to add",

"params": [
{
"name": "invoke_transaction",
"description": "The information needed to invoke the function (or account, for version 1 transactions)",
Copy link

Choose a reason for hiding this comment

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

not sure if I understand the or account, for version 1 transactions part

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad, irrelevant

},
"UNEXPECTED_ERROR": {
"code": 163,
"message": "An unexpected error occurred",
Copy link

Choose a reason for hiding this comment

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

pls define "unexpected", and could we generalize it like "An error occurred" (UNKNOWN_ERROR) and then use it for all non-contextualized errors?

"type": "object",
"properties": {
"types": {
"description": "primaryType represents the top-level type of the object in the message",
Copy link

Choose a reason for hiding this comment

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

typo?

Copy link

Choose a reason for hiding this comment

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

(e.g. this is types)

wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
Comment on lines 707 to 714
"NOT_ERC20": {
"code": 111,
"message": "Asset is not an ERC20"
},
"INCORRECT_NETWORK": {
"code": 112,
"message": "Network details are incorrect"
},
Copy link

Choose a reason for hiding this comment

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

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
wallet-api/wallet_rpc.json Show resolved Hide resolved
wallet-api/wallet_rpc.json Show resolved Hide resolved
}
}
},
"required": ["contract_address", "entry_point"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not required calldata? it can be empty

]
},
{
"name": "wallet_addDeclareTransaction",
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider renaming to wallet_declare

wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Outdated Show resolved Hide resolved
wallet-api/wallet_rpc.json Show resolved Hide resolved
wallet-api/wallet_rpc.json Show resolved Hide resolved
Comment on lines 603 to 609
"types": {
"description": "Defines the types of the typed data object",
"type": "array",
"itmes": {
"$ref": "#/components/schemas/STARKNET_TYPE"
}
},

Choose a reason for hiding this comment

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

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

Suggested change
"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"
}
}
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @penovicp! applied this and your suggestions for SNIP 12

"$ref": "#/components/schemas/STARKNET_TYPE"
}
},
"primary_type": {

Choose a reason for hiding this comment

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

Changing the primary type property from camel case to snake case would mean that it wouldn't align with the SNIP-12 specification.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same is for STARKNET_DOMAIN chain_id

Comment on lines +409 to +421
"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"
}
]
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"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"
}
]
},

"title": "Address",
"$ref": "#/components/schemas/FELT"
},
"SYMBOL": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Suggested change
"SYMBOL": {
"TOKEN_SYMBOL": {

"components": {
"contentDescriptors": {},
"schemas": {
"TXN_HASH": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
"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
@amanusk amanusk marked this pull request as ready for review April 25, 2024 13:47
Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

:lgtm:

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)

Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

Dismissed @vladutjs, and @vladutjs from 43 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dan-ziv)

@ArielElp ArielElp merged commit 304b952 into starkware-libs:master May 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants