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

3085: Move to Review #3325

Merged
merged 3 commits into from
Mar 6, 2021
Merged

3085: Move to Review #3325

merged 3 commits into from
Mar 6, 2021

Conversation

rekmarks
Copy link
Contributor

@rekmarks rekmarks commented Mar 4, 2021

Updates the status of EIP-3085 to "Review".

@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

  • Trying to change EIP 3085 state from Draft to Review

@rekmarks
Copy link
Contributor Author

rekmarks commented Mar 4, 2021

@MicahZoltu for your consideration.

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

Only blocking issue is the reference to a draft EIP. The rest is feedback that can be applied after merging to Review (or before if you desire).


Remove the last line of the abstract, as it is implicit in how all EIPs are structured.

Important cautions for implementers of this method are included in the Security Considerations section.


A request for a chain that was already added SHOULD be considered successful.

should be

A request to add a chain that was already added SHOULD be considered successful.

Also, I think this should actually say:

A request to add a chain that was already added MUST be considered successful if the chains match (defining "match" is up to the provider), and MUST return an error otherwise.

Stronger wording in specifications is almost always preferred over weaker wording, otherwise it turns into a "best practices" document and not a standard. Also, I think that the matching component should be a hard requirement with a well defined error.


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

Why is this not MUST NOT? Feels like it is asking for problems to allow it. Side note: I feel like hasEthereumChain should come first, as without that it means this specification cannot reasonably put a MUST here even though the spec should put a MUST here and could put a MUST here if hasEthereumChain was an existing standard.


Recommend including the code and message for standardized errors. Having consistent error messaging makes building dapps way easier than if you have to try to guess what went wrong or give the user a generic error message (that likely isn't helpful in troubleshooting).


Verify that the specified chain ID matches the return value of eth_chainId from the endpoint, as described in this specification.

should be

Verify that the specified chain ID matches the return value of eth_chainId from the endpoint, as described above.

This makes it more clear that you aren't linking out to an external specification.


Remove reference to EIP-1102 as it is still just a draft. Alternatively, push EIP-1102 through to Review first, then move this to review.

@rekmarks
Copy link
Contributor Author

rekmarks commented Mar 5, 2021

@MicahZoltu, I implemented all of the suggestions except:

  • Mandating that a request for a chain that was previously added MUST be considered a success.
    • It was originally a requirement, but I softened it to a SHOULD for reasons I'll divulge in the discussion.
  • Standardizing errors.
    • I'll do that separately.

Finally, up to you whether we want to wait for #3331 before merging this.

@rekmarks
Copy link
Contributor Author

rekmarks commented Mar 5, 2021

Also regarding this point:

Side note: I feel like hasEthereumChain should come first, as without that it means this specification cannot reasonably put a MUST here even though the spec should put a MUST here and could put a MUST here if hasEthereumChain was an existing standard.

I think we can mandate that the same chain ID should not be added more than once without specifying hasEthereumChain, and I did make that a requirement as opposed to a recommendation. Referring to the chain ID as opposed to "the chain" was my way of getting out of any hasEthereumChain-related headaches.

Although, I do see the challenges posed to both specifications by not ensuring that the meaning of "having" a chain is the same for both, even if it's just "if hasEthereumChain with params XYZ returns true, addEthereumChain with the same parameters must reject with an error".

Basically, you're suggesting that we wait for hasEthereumChain to be created and moved to Review before moving this to Review?

@MicahZoltu MicahZoltu merged commit 1192e96 into ethereum:master Mar 6, 2021
@MicahZoltu
Copy link
Contributor

This spec doesn't have to depend on hasEthereumChain, so no need to hold off Review status. However, this spec may not be useful until hasEthereumChain is specced out as well if you cannot do a check-then-set-if-missing of the proposed chain.

@rekmarks rekmarks deleted the 3085-review branch March 6, 2021 23:41
phi-line pushed a commit to phi-line/EIPs that referenced this pull request Apr 29, 2021
* Update 3085 status to Review

* Remove reference to EIP-1102

* Address review feedback
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.

3 participants