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

EIP-3085 Discussion: wallet_addEthereumChain #3086

Closed
rekmarks opened this issue Nov 2, 2020 · 33 comments
Closed

EIP-3085 Discussion: wallet_addEthereumChain #3086

rekmarks opened this issue Nov 2, 2020 · 33 comments

Comments

@rekmarks
Copy link
Contributor

rekmarks commented Nov 2, 2020

Discussion for EIP-3085: wallet_addEthereumChain

The EIP was briefly known as EIP-2016.

Initial PR: #3085

@wighawag
Copy link
Contributor

wighawag commented Nov 2, 2020

Great to see some EIP materializing to help apps make it easy for user to use other chains that what wallet provider offer by default.

I got few comments:

A general one: in my opinion we should avoid as much as possible the use of should or may and make the standard more strict

Regarding The wallet application may arbitrarily refuse or accept the request.
This should be accompanied with a an error code, so it should not be completely arbitrary in so far as a error code should be provided, to ensure the apps can explain its users the reason. As such the EIP should define the set of error code allowed for rejection.
We obviously cannot prevent the possibility of a unexpected error, but my point is that if we expect wallet to reject for different reason, the standard should aims to define these.

Regarding all parameters except chainId are listed as optional, even though a wallet may require them in practice.
I am not sure it is a great idea to have such ambiguous standard. Why not explicitly define which parameter are optional and which one are mandatory
If we think this is not possible, we should at least define a set of error code to explain to the apps the reason of rejection.

Regarding

If a chain with the provided parameters was already added, the request is considered successful, and the method **MUST** return the boolean value `true`.`

The wallet **MAY** define what it means for a chain to be "already added", but care should be taken to protect user security.
The wallet **SHOULD NOT** allow the same `chainId` to be added multiple times.

As mentioned as general comment, we should not use MAY or SHOULD

Also regarding The wallet **SHOULD NOT** allow the same chainId to be added multiple times.

what happen in the following scenario

  • App A request to add chain 6 with their own url endpoint
  • time passes and App A url endpoint is shutdown
  • user go to App B that also want to connect chain 6, it request to add chain 6 but this time with a different working endpoint url

what about chain 1 (that is expected to be supported by most wallet)?

Some more consideration:

Could the standard not add a way for the app to check if the wallet already support a chain ?
Or is it a privacy concern ?

Regarding security, I don't think this standard by being just a "request to add" is avoiding the security consideration mentioned in EIP-2015 (see https://ethereum-magicians.org/t/eip-2015-wallet-update-chain-json-rpc-method-wallet-updatechain/3274/13)

There are dangerous scenario waiting to happen for wallet that accept the addition of chain. Letting the app decide the currency and name, etc... is not a good idea.

As such I think the proposal should focus solely on the ability for an app to ask the wallet to use a specific rpc url for a specific chain Id. All other data should be handled by the wallet itself. If the wallet do not know a chainId, it MUST reject. Else it might display false information to the user without knowing

This brings me to EIP-2015, as the latter seems a better solution in general, as if the only purpose of 2016 is to associate a rpc url to a chainId. EIP-2015 could do that while switching the user to that new chain in the same action.

EIP-2016 also brings a potential concern for application that might not wish to make it easy for other apps to user their endpoint.

@rekmarks rekmarks changed the title EIP-2016 Discussion EIP-3085 Discussion (previously EIP-2016) Nov 9, 2020
@rekmarks rekmarks changed the title EIP-3085 Discussion (previously EIP-2016) EIP-3085 Discussion: wallet_addEthereumChain Nov 9, 2020
@rekmarks
Copy link
Contributor Author

rekmarks commented Nov 9, 2020

Thanks for your feedback @wighawag, I always appreciate it! I'm going to try to respond to each point in turn. Note that the EIP will be numbered 3085 instead of 2016, after feedback from editors.

First, regarding MAY and SHOULD, they have a purpose wherever I use them, although I try to minimize the use of MAY especially. Nevertheless, whether we should use them at all is a topic that I don't want to get into here, unless there's more widespread feedback concerning my style of specification writing.

Second, regarding the ambiguity about how the wallet should process the request, that is a deliberate choice. Wallets (and dapps) need standardized formats for different requests. Dapps need to know whether a request succeeded or failed, and if the latter, why. The specification establishes these things (we'll get to errors shortly).

As stated in the security considerations, and as you yourself point out, wallet_addEthereumChain is a powerful method, with significant UX and security implications. Wallets need to, and will, exercise their discretion in handling the method, picking metadata to use, and managing e.g. the same chainId being added twice. I have opinions about all of those things, but for the most part, they have no place in the specification.

Third, regarding errors and error codes, the specification states that the wallet must return an error if the request fails. Since the wallet can reject requests for arbitrary reasons, the specification can't make any definitive statement about what the errors are. Error standards are specified elsewhere, most prominently EIP-1474 and 1193/2696. Implementers can find suitable error codes there.

Fourth, regarding the following:

  • App A request to add chain 6 with their own url endpoint
  • time passes and App A url endpoint is shutdown
  • user go to App B that also want to connect chain 6, it request to add chain 6 but this time with a different working endpoint url

what about chain 1 (that is expected to be supported by most wallet)?

I know how MetaMask is likely to handle those cases (we won't allow chain ID 0x1 to be added by request, for example), but your examples show why we need to avoid specifying how the wallet should handle the request. We don't know, and we can't control it!

Fifth, regarding:

Could the standard not add a way for the app to check if the wallet already support a chain ?
Or is it a privacy concern ?

I believe that method should exist, but as part of a different standard. There is a clear and long-standing need for a standardized way of asking a provider "what can you do?", but that EIP has yet to be written.

Sixth, regarding the security of accepting externally added metadata, I absolutely agree. That's why the security considerations recommend against letting dapps specify any chain metadata other than the chainId and rpcUrl. MetaMask will likely take this route, and rely on chainId.network for our chain metadata.

That said, adding an unknown chain with externally submitted metadata can be acceptable if the user is informed of the risks. In any event, we can't prevent wallets from allowing it if they want to.

Seventh, and finally, regarding this:

EIP-2016 also brings a potential concern for application that might not wish to make it easy for other apps to user their endpoint.

Perhaps, but that's a concern for the proprietors of those endpoints.

@wighawag
Copy link
Contributor

wighawag commented Nov 9, 2020

Thanks @rekmarks for your reply.

here are further comments :

Regarding point 2, 4 and 7, there is an issue in that the current EIP allows an app to request adding a broken endpoint, that user might require manual operation to remove

Indeed, what if an app request to add endpoint for a specific chain
and that endpoint misbehave sometime (maybe because the service shutdown), causing a different app to fails. If the EIP allow for wallet to decide what to do if another app request to setup a chain already set, this could cause the app to break down because of a previous app. (if a wallet reject the addition of chain already added by another app)

The issue could be alleviated if the addition of a chain was sandboxed to the origin requesting it. This seems to be a good solution. What do you think?

Note though that even if the rpc url was sandboxed, the app might want to update it to a new url (they might have changed their domain name for example)

I see several options

a) reuse wallet_addEthereumChain and allow an app to always be able to replace the rpc url (even if it is chainId 0x01). If we have an rpc method to check if a wallet already support a specific chain, then I do not see why apps would request to use a specific url, but if they do, such rpc url will be sandboxed anyway. Plus users can always reject such request.
It might be better to rename such rpc method to wallet_set_chain_rpc_url or something like that.

b) separate the addition of a chain from the updating of its rpc url.
In that case wallet_addEthereumChain if called on a chain that is already set would fails with a specific error.
and a new method like wallet_set_chain_rpc_url would replace the rpc url but fails if the chain was never added

Both options would make the EIP more explicit and avoid potential differences between wallets, making the life of applications much easier

This is in my opinion an important point and as currently defined, the EIP ambiguity will likely cause trouble to app developers

Regarding point 5, I was not asking for a rpc method to check if the wallet can switch chain, but an rpc that check whether a chain is already added. I think having it as part of the standard is better than having to wait for yet another. If we really want them separate, then I would argue that they should come together with EIP-3085 being dependent on the other one.

Such method will be a necessary rpc method for apps to offer a proper user experience.

Regarding point 6, I strongly believe that having a standard allowing unsafe metadata to be displayed is a really bad idea. Especially when such data is shared across apps. A user might visit an app, accept the request thinking lightly of the consequence because the user did not intend to interact with it. then forgot about it and visit another app not realising it is being displayed wrong information.

From a security standpoint, no wallet should allow an app to make the user believe false information when it can technically prevent it.

Allowing it as part of a standard run the risk of creating a race to the bottom where unsafe wallet will be used because they offer a convenience to app/users.

Also note that as you said, nothing prevent such wallet to have their own api to allow for that if they really want to. But as for standard goes we should not make it easy.

Since Metamask already believe that they won't be allowing such ability, I do not see the argument for why metadata should be allowed to be specified by the app.

Not allowing it will also simplify the EIP which is an extra benefit.

This could be as follow :

interface AddEthereumChainParameter {
  chainId: string;
  rpcUrl: string;
}

Note that now the rpcUrl is mandatory as adding a chainId without any info would be pointless.

This is also mean that we can now remove some of the MAY and SHOULD

We could also define the different errors:

  1. CHAIN_ID_NOT_SUPPORTED : when the wallet have no info about it and cannot display meaningful information
  2. CHAIN_ALREADY_EXISTS: the chain already exist
  3. INVALID_RPC_URL
    ... maybe other?
    ...n) other : other Error as specified in EIP-1474 and 1193/2696

Regarding CHAIN_ALREADY_EXISTS, as mentioned this only make sense for option b) mentioned above

for option a) there would be no such error for wallet_addEthereumChain

Regarding error (point 3) it is I think beneficial to define the potential type of error in the EIP as it helps define the protocol more acurately.

@BelfordZ
Copy link

BelfordZ commented Nov 30, 2020

As per recommendation (thank you @MicahZoltu) Ive copied my request from the PR:

Any chance you could add an OpenRPC definition of the method? I need json schemas for this to add it. for example,

{
  "name": "wallet_addEthereumChain",
  "params": [
    {
      name: "chainId",
      schema: {  
        type: "string",
         ...
       },
      ....
    },
    returns: { ... }
  ]
}

This will allow me to easily add this method to http://ethereum-json-rpc-specification.org/ and downstream tooling.

Thanks.

@tze42
Copy link

tze42 commented Jan 2, 2021

Hi @rekmarks, @pedrouid I stumbled over this EIP and would like to suggest some additions to this RPC method proposal.

It is becoming more and more common that wallets can subscribe via websocket to events in order to reduce the polling towards RPC nodes. Therefore, it would be good to see something like wssUrl as another option in the spec.

Another typical problem is the network specific icon and considering that this method is used for wallets needing to display also an icon for the network, it would make sense to add an option like iconUrl to provide that asset right away.

During the Minerva mobile wallet development, we encountered problems with RPC nodes and therefore I wanted to ask if that specification allows to add more than one rpcURL?

Thanks and happy new year.

@PeterTheOne
Copy link
Contributor

Maybe there is some synergy with #3012 that I proposed. If there where RPC methods for chainName, nativeCurrency, blockExplorerUrl, etc. and not only chainId then one would only need to pass the RPC url here. Consider this a sidenote.

@rekmarks
Copy link
Contributor Author

rekmarks commented Jan 3, 2021

@BelfordZ, have you guys ever created a meta EIP for specifying RPC methods using OpenRPC? I'm not opposed to it by any means, but I don't want to add OpenRPC definitions to EIPs willy-nilly. Meanwhile, I created this, where I'm about to ping you on an issue with some questions: https://github.com/rekmarks/ts-to-open-rpc


@tze42, I like your suggestion of supporting WebSockets, and multiple RPC URLs. We could kill two birds with stone by replacing the rpcUrl: string field with rpcUrls: string[], and let both https and wss URLs be specified (and potentially other protocols, as appropriate). What do you think of that?

There's then a question of what we do with the other URL fields, and iconUrl if we add it. We could either treat rpcUrls as a special case, or let multiple URLs be specified everywhere.

cc: @pedrouid


@PeterTheOne, I think what you're suggesting in #3012 makes a lot of sense, and the suggested methods could definitely be beneficial to implementers of 3085. There are some security concerns with getting chain data from endpoints, as elaborated on in 3085.

@pedrouid
Copy link
Contributor

pedrouid commented Jan 3, 2021

Previously I've leaned more towards JSON schemas and OpenRPC but currently I think that we could save more time by writing proto files instead. Mostly motivated by how lacking typing is for JSON schema and how verbose it is. That being said @rekmarks's library looks great


I like the suggestion for making all url fields to have string arrays. We can even simplify keys to rpc and icons instead of mentioning Urls for both

@tze42
Copy link

tze42 commented Jan 3, 2021

@rekmarks, I'm not a dev, but the suggestion of rpcUrls: string[] looks good to me. It leaves room for future protocols and improvements provided over time, without the need to change the logic on the wallet side.

blockExplorerUrl could also be blockExplorerUrls: string[], because it is not uncommon to have several block explorers available and if it can be easily called, a wallet would be able to offer the user a selection of explorers.

For the iconUrl I don't see the need to have many, but on the other side it might not hurt to have the option of providing more than one asset link.

@rekmarks
Copy link
Contributor Author

rekmarks commented Jan 5, 2021

Here's the updated params interface, merged via #3190:

interface AddEthereumChainParameter {
  chainId: string;
  blockExplorerUrls?: string[];
  chainName?: string;
  iconUrl?: string;
  nativeCurrency?: {
    name: string;
    symbol: string;
    decimals: number;
  };
  rpcUrls?: string[];
}

@rekmarks
Copy link
Contributor Author

rekmarks commented Jan 5, 2021

I decided to keep Urls in the name for the block explorer and RPC fields, by the principle that explicit names are better. As for iconUrl, a single SVG should always suffice, and as a wallet developer, I would probably ignore everything but the first valid URL in the array anyway. 🤷‍♂️

@tze42
Copy link

tze42 commented Jan 6, 2021

I agree regarding iconUrl. It would need more metadata specification and actual use cases to define it. So it is better to improve the standard when there is really a need for it, imo.

@pedrouid
Copy link
Contributor

Actually I would disagree with the iconUrl personally because WalletConnect metadata has always included an array for icons which was useful in occasions where another party wanted a different image format. Example: providing images as JPG, PNG and SVG urls

@tze42
Copy link

tze42 commented Jan 11, 2021

That is a very good point and we would definitely prefer to get a SVG over PNG or JPG, if it is available.

@rekmarks
Copy link
Contributor Author

Yeah, I agree that we should do iconUrls: string[] instead. This PR should automatically merge in a moment: #3227

For any dapp developers reading this, MetaMask won't do anything with this property in our first iteration of this feature, and once we do use it, we will pick the first SVG that we find and ignore anything else.

@wighawag
Copy link
Contributor

Hey all, @rekmarks, any feedback on my comments? I would love to finally have applications be able to use different chains without requiring user to manually enter information, but this should not come at the cost of decreased security.
I also think this EIP, by not having a mechanism to switch chain programmatically will result in sub-par user experience.
See my comment above : #3086 (comment)

@tze42
Copy link

tze42 commented Jan 30, 2021

Hey @wighawag, I see it from a wallet perspective and the Minerva wallet we work on, uses all supported chains simultaneously. Adding a new chain via the proposed interface is a big improvement, as it avoids us to integrate it natively and allows a higher convenience for the user. So UX is actually very much improved. Also we could use that interface to get updated information, avoiding manual code intervention.
On the other side, it could also increases the security risk (e.g. through a malicious RPC endpoint), but that must be up to the App dev, if they are willing to expose their users to it and how they intend to mitigate it. Therefore, I see that beyond the scope of this EIP.

@wighawag
Copy link
Contributor

wighawag commented Jan 30, 2021

@tze42 thanks for your feedback, some comments:

"Adding a new chain via the proposed interface is a big improvement, as it avoids us to integrate it natively and allows a higher convenience for the user. So UX is actually very much improved. Also we could use that interface to get updated information, avoiding manual code intervention."

I understand, I am not against the benefits. I have been asking for this feature for a while myself as an app developer. I am against its current form. In my comment above, I mention security but I also mention the following issues:

  • potential issues with added network url whose backend are not maintained.
    For this I mention a potential solution through the sandboxing of "added networks" per origin so a previous app do not affect others.

  • the lack of method for apps to check if a network was already added/present, this has consequences for the user experience too. Apps do not want to show a popup straight in the face of their user. They want to first allow the user to make the request. And if they can check that the network is already added, they can skip that step, providing a smoother experience

  • the disadvantage this EIP has over EIP-2015 in term of user experience: It still require the user to switch to a network. They are not incompatible but we could imagine a version of EIP-2015 that make EIP-3085 irrelevant: the network is added automatically (and temporarly) upon request. And I would have preferred such EIP to take precedence.

On the other side, it could also increases the security risk (e.g. through a malicious RPC endpoint), but that must be up to the App dev, if they are willing to expose their users to it and how they intend to mitigate it. Therefore, I see that beyond the scope of this EIP.

The security issue I mention are about the metadata. With the current design, that allows application to provide the metadata, it allows application to mislead users into thinking they are on a testnet for example, not realizing they are operating on an actual mainnet they interacted before and where their tokens are at risk.

I strongly believe that wallets' main role is to protect users and this include ensuring the validity of the information displayed to them. This proposal fails in this regard. In one earlier reply, @rekmarks mentioned that Metamask will actually ensure the metadata provided is valid but as I mentioned, some wallet (like your it seems) might not do the check for "user convenience". An EIP should prevent such wallet to do that to ensure user safety.

The EIP actually mention the following :

For security reasons, a wallet should always attempt to validate the chain metadata provided by the dapp, and may choose to fetch the metadata elsewhere entirely. Either way, there’s no way to know what the wallet will need with respect to the chain metadata. Therefore, all parameters except chainId are listed as optional, even though a wallet may require them in practice.

To me it should be a MUST and removing the ability to specify metadata make it so while at the same time simplifying the proposal.

If adding extra network to your wallet code is a difficult operation, you can always implement that through an external service, like metamask is planning to do. This would already be better than letting malicious app doing it for you.
As a wallet user, though I would prefer wallets that offer such protection built-in and not rely on external services not being compromised. Note that adding support for a chain is simply to have dictionary of chainId -> metadata (name, coin name, ....) to ensure the wallet can display valid information to their users. This is because the rpcUrl is still provided by the applications.

@rekmarks
Copy link
Contributor Author

rekmarks commented Jan 30, 2021

@wighawag, I apologize for my tardy response. I think you make some excellent points, some that I believe disagree with for technical / UX reasons, and others that I disagree with on principle. The disagreements are not insubstantial, and I've been putting off my response, but here goes. I will try to summarize your points and respond to them in turn.

I will preface this by noting that:

  • I will use the terms "chain" and "network" interchangeably throughout.
  • My views are mine alone, and I am not speaking on behalf of MetaMask or my employer, ConsenSys.

Whether EIP-3085 Switches the Currently Selected Network

@wighawag:

the disadvantage this EIP has over EIP-2015 in term of user experience: It still require the user to switch to a network.

This is not true. Rather, EIP-3085 is agnostic on the matter. The specification mentions this:

Note that this method makes no statement about whether the wallet should change the user's currently selected chain, if the wallet has a concept thereof.

In MetaMask's implementation, the network is switched when the user adds the network.

I'm open to recommending (i.e., using SHOULD) that the wallet switches to a network when it is added. I don't think that it ought to be required, since a wallet implementer may not want to for various reasons.

Edit: More than anything, I feel like the spec can remain agnostic on the matter, and people will do what makes the most sense, which is probably switching the network.

Regarding Necessary Parameters

@wighawag:

Note that now the rpcUrl is mandatory as adding a chainId without any info would be pointless.

I believe that, regardless of any proposed implementations, the chainId remains the only strictly required parameter, because a wallet could have a predefined list of allowed chains and their metadata. Such a wallet would only need the chainId in order to add the network. (I have no interest in doing it that way — I explain why below — but there's no reason for the standard to forbid it.)

Regarding the Addition of a "wallet_hasEthereumChain" RPC Method

@wighawag

I was not asking for a rpc method to check if the wallet can switch chain, but an rpc that check whether a chain is already added.

Such a method would enable UX improvements, but is by no means required in order to grant dapps the ability to ask the user to add a network to their wallet. While implementing such a method sounds reasonable to me, it is a different RPC method, it should be defined in its own EIP, and I consider it out of scope for this discussion.

Regarding Errors

@wighawag:

Regarding error (point 3) it is I think beneficial to define the potential type of error in the EIP as it helps define the protocol more accurately.

Yeah, I agree that it makes sense for errors (specifically, error codes) to be defined. Those errors should probably be in the EIP-1193 provider error range. The only problem is that there's no EIP defining the space of all provider errors, although that does not prevent errors from being defined here.

There's already an "invalid params" error in the JSON-RPC error range that can be used for a lot of cases, but there's at least one error we should standardize, namely CHAIN_ALREADY_EXISTS. That's a failure mode that can't be covered well by more generic errors codes.

Incidentally, that error would give us some of the UX benefits we're looking for with the proposed wallet_hasEthereumChain RPC method.

Regarding Associating RPC URLs with Origins

@wighawag:

The issue could be alleviated if the addition of a chain was sandboxed to the origin requesting it. This seems to be a good solution. What do you think?

I think this is a great idea. I don't think there's anything in EIP-3085 preventing a wallet from implementing the method in this way, and I'll add it to security considerations. The idea reminds me of EIP-1102 (eth_requestAccounts) and EIP-2255 (wallet_requestPermissions).

Regarding Failing Endpoints

@wighawag:

there is an issue in that the current EIP allows an app to request adding a broken endpoint, that user might require manual operation to remove ... this could cause the app to break down because of a previous app. (if a wallet reject the addition of chain already added by another app)

I agree that this is a UX issue we are likely to see in practice over time, but like the proposed wallet_hasEthereumChain method, it is a UX improvement, not a necessity.

EIP-3085 leaves the path wide open for wallets to manage multiple RPC endpoints per chain, but it does not mandate that they support it. If any issues arise with a particular RPC endpoint, the user can as you say manually remove it, and re-add a different one. This is fine for now, and can be improved in the future.

Regarding Malicious Behavior

@wighawag:

I strongly believe that having a standard allowing unsafe metadata to be displayed is a really bad idea. ... From a security standpoint, no wallet should allow an app to make the user believe false information when it can technically prevent it.

In order for this method to be useful, it has to expose the user to the risk of adding a malicious or falsely represented RPC endpoint. The risks are a feature, not a bug.

Just like a web browser cannot and does not protect you from most kinds of malicious website behavior, an Ethereum wallet cannot protect you from most kinds of malicious behavior by dapps and RPC endpoints. For example, MetaMask takes steps to protect our users from malicious actors or unsafe behavior — e.g. by maintaining blocklists of known fraudulent websites and Ethereum addresses — but we can't stop you from spending your entire ETH balance on a random shitcoin.
Our responsibility is to inform the user of the risks they are exposed to by taking a certain action, not to prevent them from taking it. Ascertaining the trustworthiness of actors in a decentralized system belongs to a related but ultimately distinct problem space from accessing such a system.

The nature of the problem is why I believe that lists of pre-approved chains are not a viable solution. My experience of helping maintain a similar list for ERC20 tokens at MetaMask is one of the reasons why I decided to write this EIP. I argue that no wallet has or will have the resources to adequately maintain an allow-list unless it is trivially short. I have no interest in doing that. Rather, I want people to be able to access the decentralized web by whatever means they wish, without anyone's approval.

All implementers of EIP-3085 should do exactly what it recommends, which is to perform basic request validation and then adequately inform users of the risks they're exposed to. Wallets can used community-sourced lists like chainId.network to detect discrepancies, but other than that, it is ultimately the user's responsibility to do their due diligence. There's certainly more that this ecosystem (and MetaMask) can do to facilitate this due diligence, but I believe that that's out of scope for this discussion as well.

@rekmarks
Copy link
Contributor Author

rekmarks commented Jan 30, 2021

To summarize action items from my above post synthesized from @wighawag's feedback, I think we should:

  • Add a CHAIN_ALREADY_EXISTS error to EIP-3085 following the pattern established by EIP-1193
  • Recommend sandboxing RPC URLs / chains added using wallet_addEthereumChain in the EIP-3085 security considerations

I'm happy to take on those two.

In addition, if someone feels so inclined, they should create a new EIP (or just discussion for now) for wallet_hasEthereumChain to allow dapps to check for a chain without bothering the user. I think dapps will be fine with the proposed error, but, it sounds useful.

As an aside I want to note that, in an ideal world, wallets get rid of the notion of a "currently selected chain", parameterize the chain ID for incoming requests, and thereby solve all issues related to "switching the network".

@rekmarks
Copy link
Contributor Author

rekmarks commented Feb 1, 2021

I added a PR for the previously discussed error: #3237

While adding this to the specification, I also changed the specification such that the method must return this error if a wallet receives a request to add a chain that was already added. This way, the method call is only regarded as a success if the call itself caused the chain to be added to the wallet. That means there's less room for ambiguity, which should be good for dapp developers.

Since it is a change in behavior, comments are appreciated.

@wighawag
Copy link
Contributor

wighawag commented Feb 1, 2021

Thanks @rekmarks for your reply. It gave me a better understanding of the current proposal's perspective that I did not fully consider. I still have some remark to make:

Whether EIP-3085 Switches the Currently Selected Network

This is not true. Rather, EIP-3085 is agnostic on the matter...
...More than anything, I feel like the spec can remain agnostic on the matter, and people will do what makes the most sense, which is probably switching the network.

I indeed read that the spec mention that switching is possible, but as an app developer if this is not a guarantee, this is not the same as EIP-2015, hence my comment. I am still happy with EIP-3085 approach though, and as you mentioned in your last comment, if we can move to a non-switching chain system where the chainId is specified as part of other rpc call, then EIP-3085 will be indeed all we need. I hope we get there soon :)

Regarding the Addition of a "wallet_hasEthereumChain" RPC Method

Such a method would enable UX improvements, but is by no means required in order to grant dapps the ability to ask the user to add a network to their wallet. While implementing such a method sounds reasonable to me, it is a different RPC method, it should be defined in its own EIP, and I consider it out of scope for this discussion.

I strongly disagree on this. I see an EIP as a set of rules for a single purpose with consideration for everything that this purpose entails, including UX consideration. Let's not constrain ourselves to an adhoc rule of "1 rpc method = 1 EIP". While in some case it might be a good rule, I see it as a heuristic not as a hard rule.

I worry that separating these 2 rpc call will result in wallet implementing one and not the other. It will actually de-facto allow them to do that and this would be bad for application's user experience.

If you too agree that this will lead to a better UX, why not make "wallet_hasEthereumChain" EIP a dependency to "wallet_addEthereumChain" or simply add it to the proposal.
Is that solely because of the adhoc rule mentioned above and some sense of EIP pureness ?

I think it is important to consider the UX consequences when writing EIP like this one and saying we can have improvements later, while we could already foresee the issues, is to me a bad idea. So unless there is strong reason to not bake-in this improvement, it should in my opinion, be part of the proposal.

Regarding Associating RPC URLs with Origins + Regarding Failing Endpoints

I think this is a great idea. I don't think there's anything in EIP-3085 preventing a wallet from implementing the method in this way, and I'll add it to security considerations. The idea reminds me of EIP-1102 (eth_requestAccounts) and EIP-2255 (wallet_requestPermissions).

If you agree with the issue of "failing endpoints" and the idea of associating rpc url to origins to solve it, why not make it part of the EIP ?

As alluded above, saying it is an UX improvement and not a necessity does not make a strong argument to not have it. This EIP's purpose itself is to improve the current UX, to allow application to automate the addition of chains, instead of relying on user to add them manually by following instruction. If we could help make it even easier for the user, we should.

Regarding Malicious Behavior

I understand better the perspective but still consider it a potential security issue. The analogy to web browser is I think not accurate. The analogy to ERC20 sounds accurate but the security risk is present there too and I would have made the same comment if I had the opportunity :)

It might be true though that letting wallet decide how to handle the addition of metadata might well be the best we can do to ensure we do not segregate valid chain / ERC20 tokens . Your experience seems to indicate it and I do not have any evidence of a better mechanism working.

Further comments

It is not clear to me what should / must happen when adding a chain whose chainId already exist. You now mention the idea of using "CHAIN_ALREADY_EXISTS " which was suggested as part of mechanism b) ( see comment: #3086 (comment)) where a new rpc method was proposed to allow the updating of rpc url.

Is that something considered too ? something like wallet_updateEthereumChain to allow app to update the rpc Url or add more url.
If not, the issue about failing rpc is even more salient as it would require as you mentioned, a manual operation from the user to remove the chain to allow an application to add it again (with a working rpc)

@wighawag
Copy link
Contributor

wighawag commented Feb 1, 2021

@rekmarks

In addition, if someone feels so inclined, they should create a new EIP (or just discussion for now) for wallet_hasEthereumChain to allow dapps to check for a chain without bothering the user. I think dapps will be fine with the proposed error, but, it sounds useful

I just realised from this comment that you might not fully grasp the issue about the lack of "wallet_hasEthereumChain" and its consequences for the applications and their users.

The error "CHAIN_ALREADY_EXISTS " you mentioned would not solve it as this error happen only if you call wallet_addEthereumChain which will still trigger the request popup if the chain was not already added.

And like it is today best practise to not request access to the wallet without having the user make an interaction first, you would not want to trigger a popup for chain addition without first notifying the user of the need for it.

To show the issue here are the step that would happen for an application that cannot rely on the existence of the "wallet_hasEthereumChain" method:

  1. call "wallet_hasEthereumChain"
  2. if it succeed, great,
    • if the result of that success is a "yes", continue with the application flow,
    • else indicate to the user that it need to add a chain. The application wait for acknowledgement. let say a button that then trigger "wallet_addEthereumChain". This trigger the wallet popup, All good.
  3. But if instead it fails ("wallet_hasEthereumChain" is not implemented), then the application cannot know whether the chain has already been added (unless it is already connected to it). It has then no other choice to tell the user that it need to add/check a chain even if it was already added and so it need to do like step 2 and show a button, adding an extra step for the user even though no popup will be triggered if the chain was already added.

The alternative as mentioned above would be to always call "wallet_addEthereumChain" but this would result in throwing a wallet popup in the user face when the chain was never added, a bad user experience

if "wallet_addEthereumChain" implementation was a requirement to EIP-3085, then step 3, would not need to be considered and we could directly continue with the application flow after checking the chain was already added. This would enable a more natural flow for the user.

hence why I strongly believe "wallet_hasEthereumChain" should be part of EIP-3085 or a dependency of it

@rekmarks
Copy link
Contributor Author

rekmarks commented Feb 1, 2021

@wighawag thanks for continuing to contribute to this discussion! I am thanks to your arguments convinced that we need wallet_hasEthereumChain, and I'll write the specification. It's just a question of where the specification will live.

My inclination is still to put wallet_hasEthereumChain into its own EIP, and have each separate EIP reference the other (e.g. "implementers of this method SHOULD also implement wallet_(add|has)EthereumChain"). I don't see any reason for us to prevent a wallet from implementing one method but not the other, and I don't think implementers will suffer from the specifications living in two separate documents. Also, in my experience, the EIP editors prefer smaller specs, and shipping two separate EIPs increases the likelihood of finalizing both specifications sooner.


Regarding sandboxing a chainId / RPC URL combo to a specific origin, the EIP is explicitly agnostic about what it means to "add" a chain to a wallet in order to avoid making assumptions about wallet implementation details, which are likely to change in fundamental ways over the course of time. I think recommending it in the security considerations section is good enough, and also all that this specification can feasibly do.

@wighawag
Copy link
Contributor

wighawag commented Feb 2, 2021

Hey @rekmarks glad to be contributing to this must needed EIP :)

Regarding the separation of the 2 RPC in 2 different EIPs. I do not really mind, except that the relation should be a MUST and not a SHOULD for the reason I mentioned. Otherwise applications will have to consider the case where wallets do not implement wallet_hasEthereumChain and the issue it poses for the user experience.

As such if there are 2 EIPs, then EIP-2085 will need to depend on the wallet_hasEthereumChain EIP (and not the other way around).
I feel having both RPC methods in one EIP will be more logical but that is my personal preference.

This is a separate topic but the issue about failing rpc, might require the need for yet another rpc call: wallet_updateEthereumChain to allow application to specify a different rpc url (or add an extra one).
And so, if we go with one rpc call per EIP then this would require yet another dependent EIP

In some ways, this EIP (EIP-3085) is about a resource and not about a specific rpc call. as such having the usual CRUD methods form a logical whole (I don't think we need the DELETE operation though). That is why I personally prefers all of these method to be part of the same EIP.

@MicahZoltu
Copy link
Contributor

I agree with @wighawag that wallet_hasEthereumChain should probably be a dependency of wallet_addEthereumChain and wallet_updateEthereumChain since to use either of the latter two in a reasonable environment requires the former (otherwise you have to use exception handling for code flow, which is bad). However, I generally agree that they should be separate EIPs for the reasons @rekmarks mentioned.

@wighawag
Copy link
Contributor

wighawag commented Feb 3, 2021

Thanks @MicahZoltu for sharing your opinion.

However, I generally agree that they should be separate EIPs for the reasons @rekmarks mentioned.

What are the reasons exactly? Is that solely the following or are there other reasons ?

  • the EIP editors prefer smaller specs
  • shipping two separate EIPs increases the likelihood of finalizing both specifications sooner.

If so, this does not sounds strong arguments by themselves to me.

And while I do not really mind how EIP are structured, I want to make sure that a wallet cannot claim to implement EIP-3085 while failing to implement the required rpc calls. This would badly impact application developers that would then need to cover the resulting edge cases.

To me having the (potentially 3) rpc method in one EIP is the most logical. It is also the most simple. As soon as there are cyclic dependency between them, which is definitely the case if we add updateEthereumChain to the lot, we would need to introduce an overall EIP that mention all 3 to avoid the cyclic references. This would make each independent EIP incomplete though, which is even more worrying.

The argument that having them in the same EIP would make it longer to have them finalized is probably only true for rpc calls that are truly independent. This is not the case here.

@MicahZoltu
Copy link
Contributor

If it ends up being the case that there is no use in implementing any of the 3 EIPs standalone, then having a single EIP starts to make sense. However, I believe we can falsify this statement with wallet_hasEthereumChain. A wallet that implements wallet_hasEthereumChain but does not implement the other two would still be useful because dapps could query the wallet about a specific chain and then present the user with appropriate error messaging rather than failing on a query sometime later.

To me, this is enough to extract out that EIP, which leaves wallet_addEthereumChain and wallet_updateEthereumChain. If we assume that both have a hard dependency on wallet_hasEthereumChain, is there a reasonable scenario under which a wallet might implement one but not the other? If so, then separate EIPs make sense. If not, then a single EIP for those two might make more sense.

I believe that it would not be unreasonable for a wallet to implement wallet_addEthereumChain but not wallet_updateEthereumChain. An example would be a wallet that allows dapps to call wallet_addEthereumChain to add chains the wallet doesn't already have, and then those chains would be scoped to the dapp and have a session lifetime. In such a situation, wallet_updateEthereumChain would not serve any purpose because the wallet would not allow a dapp to update a baked in or manually added global chain (like mainnet) and the dapp would be setting a chain itself at launch every time the page was loaded, so updating doesn't make a lot of sense.

TL;DR: I think separate EIPs are a good idea here because there are imaginable situations where a wallet will implement a subset of all of the endpoints.

@wighawag
Copy link
Contributor

wighawag commented Feb 4, 2021

thanks @MicahZoltu
I agree with your arguments in regard to wallet_hasEthereumChain being standalone.

As for wallet_addEthereumChain without wallet_updateEthereumChain I disagree as this pose issues for wallet that keep the association for a longer lifetime (which the EIP allows) as mentioned in my previous comment.

Wallets that want to keep the association for only a session lifetime, they can simply reject the call to wallet_updateEthereumChain, but I also do not see any issue allowing the app to update while the session is live. I would say it would be preferable as this will ensure application can always relies on that rpc call.

Now we could argue that we could have wallet_updateEthereumChain without wallet_addEthereumChain but that does not sound very useful. This could solve the cyclic dependency though with

wallet_addEthereumChain -> wallet_updateEthereumChain -> wallet_hasEthereumChain

(arrow meaning "depends on")

@MicahZoltu
Copy link
Contributor

they can simply reject the call to wallet_updateEthereumChain

If we just define this rejection to be a JSON-RPC error for method not found then we end up with 3 separate EIPs. Keep in mind, any wallet can choose to implement both wallet_addEthereumChain and wallet_updateEthereumChain if they want. Being separate EIPs just means that each wallet gets to make that decision separately. For wallets that save added chains for long periods of time, add + update makes sense, and for wallets that have session scoping for added chains, only add may make sense.

@wighawag
Copy link
Contributor

wighawag commented Feb 6, 2021

Keep in mind, any wallet can choose to implement both wallet_addEthereumChain and wallet_updateEthereumChain if they want.

That's what I want to avoid (I want to make sure both are implemented), as this otherwise lead to issue for applications when dealing with wallet that chose to not implement wallet_updateEthereumChain even so they keep chain info for longer lifetime.

It is easier to enforce implementation even for wallet that keep chain info only for a session, than letting any wallet to not implement it.

Now it is true we cannot avoid implementation to reject a call for whatever reason, so the issue mentioned is not entirely avoidable, but at least the wallet would have an error for the application to display. And rejecting for a "not found" reason would simply be a the detriment of the wallet. Hence why the EIP should not mention it as a possibility. The EIP should make wallet_updateEthereumChain a dependency of wallet_addEthereumChain

It is also worth noting that at the point an application will call wallet_updateEthereumChain it would already have prepared the user for the request. And as such any failure at that point would not be a great experience. As such if we really want to make wallet_updateEthereumChain optional, we should then have a mechanism for applications to know whether wallet_updateEthereumChain is implemented so they do not learn it when making the call.

@MicahZoltu
Copy link
Contributor

As such if we really want to make wallet_updateEthereumChain optional, we should then have a mechanism for applications to know whether wallet_updateEthereumChain is implemented so they do not learn it when making the call.

I 💯 agree with this. The JSON-RPC is in dire need of a capabilities discovery/negotiation mechanism. e.g., a way to ask window.ethereum.request('provider_getCapabilities') and get back a list of all supported capabilities (e.g., EIPs) or something like ('provider_checkMethodAvailability', ['wallet_updateEthereumChain', 'wallet_hasEthereumChain', 'eth_sendTransaction']).

@rekmarks
Copy link
Contributor Author

rekmarks commented Mar 4, 2021

At the request of the Catherders, I'm moving discussion for this EIP here: https://ethereum-magicians.org/t/eip-3085-wallet-addethereumchain/5469

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

No branches or pull requests

10 participants
@PeterTheOne @wighawag @MicahZoltu @BelfordZ @pedrouid @tze42 @rekmarks and others