-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
EIP-3085: wallet_addEthereumChain #3085
Conversation
It might be nice to include a "request to switch network" method as part of the same proposal? Seems like only with both can we eliminate the situation where dapps have to instruct users how to make network-related selections within their wallet. |
@danfinlay, @MicahZoltu has convinced me that granularity is a virtue when it comes to EIPs, and I think the network switching should be in its own one. 2015 can then extend/bundle the two, as necessary. I noticed that the next free EIP number appears to be 2017 😉 |
Co-authored-by: Dan Finlay <542863+danfinlay@users.noreply.github.com>
I've opened a PR to update EIP-2015 to have closer standardization with this EIP Something that I'm reconsidering whether or not it's a reasonable parameter is Applications can permissionessly interface with each other given that standards are followed and become interoperable. However this wouldn't be true for What is the value of providing that url if you are not aware of the API for that website? How is the wallet going to simply link transactions, adresses, tokens, contracts, etc?? ExamplesBlocksBlockscout (chainId=100) -> https://blockscout.com/poa/xdai/blocks/<BLOCK_HASH_OR_HEIGHT> Etherscan and Etherchain are in consensus for blocks endpoint but Blockscout uses TransactionsBlockscout (chainId=100) -> https://blockscout.com/poa/xdai/tx/<TX_HASH> All block explorers are in consensus for transactions endpoint. AccountsBlockscout (chainId=100) -> https://blockscout.com/poa/xdai/address/<ACCOUNT_ADDRESS> All block explorers are in consensus for accounts endpoint except Etherchain which uses ERC-20 TokensBlockscout (chainId=100) -> https://blockscout.com/poa/xdai/tokens/<TOKEN_ADDRESS> Etherscan and Etherchain are in consensus but Blockscout uses CONCLUSION: Some block explorers do share the same API but there are still inconsistencies and in order to make the EDIT: This standardization will also make future L2 solutions also have consistent endpoints BlocksMatic -> https://explorer.matic.network/blocks/<BLOCK_HEIGHT_OR_HASH> TransactionsMatic -> https://explorer.matic.network/tx/<TX_HASH> AccountsMatic -> https://explorer.matic.network/address/<ACCOUNT_ADDRESS> ERC-20 TokensMatic -> https://explorer.matic.network/tokens/<TOKEN_ADDRESS> |
Anyway instead of talking... I've created a draft for this issue which I believe will be very valuable for Wallet development and additionally for this EIP |
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 left a bunch of comments, but nothing blocking getting this merged.
I am a little concerned about the cherry picked EIP number. We have had problems with users picking EIP numbers in the past as people do bad things in order to get vanity numbers so my preference is to disallow any number picking. That being said, I don't have a specific argument against this particular selection and in the past the editors have ruled on the side of being more permissive than I about EIP number selection.
If you want this to be merged by me right away, change the EIP number to 3085 (in the frontmatter and in the file name). If you really are set on 2016, you can wait until I get more feedback from other editors.
EIPS/eip-2016.md
Outdated
|
||
The method takes a `chainId` and some chain metadata, returning `true` if the chain was added to the wallet, and an error otherwise. | ||
|
||
The wallet **MAY** reject the request for any reason. The wallet **MUST** reject the request if the `chainId` is improperly formatted. |
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.
Consider adding:
The wallet MUST reject the request if the wallet does not have the capacity to connect to the requested
chainId
.
EIPS/eip-2016.md
Outdated
#### Parameters | ||
|
||
```typescript | ||
interface AddEthereumChainParams { |
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 bit confusing to read. The section is titled Parameters
but then it only lists a single interface. I think what is meant is that the following function overload exists on the ethereum
object: function request(method: 'wallet_addEthereumChain', options: AddEthereumChainParams): Promise<boolean>
but that isn't immediately clear.
EIPS/eip-2016.md
Outdated
- If provided, **MUST** specify the URL of the RPC endpoint used to communicate with the chain. | ||
|
||
All URL string fields **MUST** include the protocol component of the URL. | ||
If the protocol of the URL is HTTP, HTTPS **SHOULD** be used. |
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 the protocol of the URL is HTTP, HTTPS **SHOULD** be used. | |
HTTPS **SHOULD** be used. |
I'm not sure if this is what you meant or not, but it definitely seems wrong to say that "if HTTP is explicitly supplied, ignore it and use HTTPS instead". If that is really what you mean, why not instead say that HTTPS **MUST** be used if available, even if the URL provided is HTTP
or something?
EIPS/eip-2016.md
Outdated
{ | ||
"chainId": "0x5", | ||
"chainName": "Goerli", | ||
"rpcUrl": "https://goerli.infura.io/v3/406405f9c65348f99d0d5c27104b2213", |
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 isn't a valid API key is it? If so, you should probably roll your keys. 😬
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 it is, it's not mine 😅
I believe I copypasted it from @pedrouid's examples in EIP-2015.
EIPS/eip-2016.md
Outdated
"nativeCurrency": { | ||
"name": "Goerli ETH", | ||
"symbol": "gorETH" | ||
}, |
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.
decimals
isn't optional in the spec, but it is left out here so at the moment this is invalid.
EIPS/eip-2016.md
Outdated
For requests of this feature in the community, see e.g. [this GitHub issue](https://github.com/MetaMask/metamask-extension/issues/3604). | ||
|
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.
For requests of this feature in the community, see e.g. [this GitHub issue](https://github.com/MetaMask/metamask-extension/issues/3604). |
This is just history, not of value to a reader 5-10 years in the future reading this specification (target audience). Also, would be more appropriate in Motivation not Rationale (though not really appropriate anywhere IMO).
EIPS/eip-2016.md
Outdated
The wallet should: | ||
|
||
- Ensure that a submitted chain ID is valid. | ||
- It should be submitted as a `0x`-prefixed hexadecimal string per [EIP-695](./eip-695.md), and parse to an integer number. |
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 should be in the Rationale, not Security Considerations.
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'll just remove it, since the definition of a valid chainId
is already specified in the "Parameters" section.
EIPS/eip-2016.md
Outdated
- Only use the submitted chain ID to sign transactions, **never** a chain ID received from an RPC endpoint. | ||
- A malicious or faulty endpoint could return arbitrary chain IDs, and potentially cause the user to sign transactions for unintended chains. | ||
- Verify that the specified chain ID matches the return value of `eth_chainId` from the endpoint. | ||
- This should be done before any other interactions with the 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.
It feels like this should be done before responding to wallet_addEthereumChain
. Consider moving this up to the specification as a hard requirement.
EIPS/eip-2016.md
Outdated
At present, such community resources include: | ||
|
||
- [chainId.network](https://chainid.network) | ||
|
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.
At present, such community resources include: | |
- [chainId.network](https://chainid.network) |
The chances that this website will stand the test of time are low, so it really shouldn't be included in the EIP. Content like this should be made available to users elsewhere on the internet, just not in the specification document. Keep in mind, the EIP is purely a specification, it is not a best practices, tutorial, help site, FAQ, etc. The EIP should be kept as simple as possible and focus only on the specification itself and not worry itself with assisting users in actually implementing anything (that is better suited for a blog or forum post).
EIPS/eip-2016.md
Outdated
## References | ||
|
||
- [chainId.network](https://chainid.network) | ||
- Related EIPs | ||
- [EIP-155: Simple replay attack protection](./eip-155.md) | ||
- [EIP-695: Create `eth_chainId` method for JSON-RPC](./eip-695.md) | ||
- [EIP-1102: Opt-in Account Exposure](./eip-1102.md) | ||
- [EIP-2015: Wallet Update Chain JSON-RPC Method (`wallet_updateChain`)](./eip-2015.md) | ||
|
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.
## References | |
- [chainId.network](https://chainid.network) | |
- Related EIPs | |
- [EIP-155: Simple replay attack protection](./eip-155.md) | |
- [EIP-695: Create `eth_chainId` method for JSON-RPC](./eip-695.md) | |
- [EIP-1102: Opt-in Account Exposure](./eip-1102.md) | |
- [EIP-2015: Wallet Update Chain JSON-RPC Method (`wallet_updateChain`)](./eip-2015.md) |
This is not an official EIP section, and we tend to avoid non-standard sections whenever possible. I believe all of these are already linked inline, so this section doesn't provide any additional value to the EIP and the content probably should be removed.
Just got EIP-3091 merged... I will include it as requirement for EIP-2015... You should also include it for EIP-2016 @rekmarks |
All feedback addressed, @MicahZoltu and @pedrouid. If I didn't respond with a comment, I agree with the feedback and attempted to fix the problem directly in the EIP. |
Hmm, something seems to be broken with GitHub status checks. Travis is green, yet GitHub says it is pending. Will check again later and kick it again if it doesn't self resolve. |
Any chance you could add an OpenRPC definition of the method? I need json schemas for this to add it. for example,
|
A wallet that implements `wallet_addEthereumChain` should expect to encounter requests for chains completely unknown to the wallet maintainers. | ||
That said, community resources exist that can be leveraged to verify requests for many Ethereum chains. | ||
The wallet should maintain a list of known chains, and verify requests add chains against that list. | ||
Indeed, unless there are good reasons not to, the wallet should _prefer its own chain metadata_ over anything submitted with a `wallet_addEthereumChain` request. |
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.
Possible amendment:
Indeed, unless there are good reasons not to, the wallet should _prefer its own chain metadata_ over anything submitted with a `wallet_addEthereumChain` request. | |
Indeed, unless there are good reasons not to, the wallet should _make clear to the user the existence of any previously configured chain metadata_ over anything submitted with a subsequent `wallet_addEthereumChain` request. |
This
- Avoids being overly zealous about wallet defaults, and leaves a friendly path to users adding new custom providers.
- Extends the concern to include any pre-loaded chain metadata, not just wallet-developer provided metadata.
Rendered file: https://github.com/rekmarks/EIPs/blob/2016/EIPS/eip-3085.md This PR adds EIP-3085, which introduces the wallet_addEthereumChain RPC method. It's basically EIP-2015, without requiring the network to be switched. I think 2015 is a great idea, but wallets may desire an RPC method that only adds a chain, without requiring that the network is changed. For example, allowing dapps to request network changes is bound to cause UX issues, especially as we see dapps moving to different layer 2 chains.
Rendered file: https://github.com/rekmarks/EIPs/blob/2016/EIPS/eip-3085.md
This PR adds EIP-3085, which introduces the
wallet_addEthereumChain
RPC method. It's basically EIP-2015, without requiring the network to be switched.I think 2015 is a great idea, but wallets may desire an RPC method that only adds a chain, without requiring that the network is changed. For example, allowing dapps to request network changes is bound to cause UX issues, especially as we see dapps moving to different layer 2 chains.
2015 and 3085 can proceed independently of each other.
Co-author credits go to @pedrouid