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-2015: Wallet Update Chain Method #2015

Merged
merged 7 commits into from May 14, 2019

Conversation

Projects
None yet
5 participants
@pedrouid
Copy link
Contributor

commented May 12, 2019

pedrouid added some commits May 12, 2019

Show resolved Hide resolved EIPS/eip-2015.md Outdated
@danfinlay

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Thanks for posting this, @pedrouid! Tracking at MetaMask here: MetaMask/metamask-extension#5101

@pedrouid

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Thanks for posting this, @pedrouid! Tracking at MetaMask here: MetaMask/metamask-extension#5101

Great, just created an issue on WalletConnect as well which can be tracked here: WalletConnect/walletconnect-monorepo#122

@pedrouid pedrouid referenced this pull request May 13, 2019

Open

Implement EIP-2015 #123

Show resolved Hide resolved EIPS/eip-2015.md
@pedrouid

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Question: Normally Ethereum JSON-RPC requests and responses are hex encoded. Should we change the chainId and networkId parameters to be hexadecimal strings instead? cc @danfinlay

Show resolved Hide resolved EIPS/eip-2015.md Outdated
@danfinlay

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Normally Ethereum JSON-RPC requests and responses are hex encoded. Should we change the chainId and networkId parameters to be hexadecimal strings instead?

You're right, most are hex encoded, but we've already violated that policy within the wallet_ namespace for EIP-747, so maybe we just say wallet methods are less strict?:
https://github.com/estebanmino/EIPs/blob/master/EIPS/eip-747.md

Show resolved Hide resolved EIPS/eip-2015.md
@pedrouid

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Normally Ethereum JSON-RPC requests and responses are hex encoded. Should we change the chainId and networkId parameters to be hexadecimal strings instead?

You're right, most are hex encoded, but we've already violated that policy within the wallet_ namespace for EIP-747, so maybe we just say wallet methods are less strict?:
https://github.com/estebanmino/EIPs/blob/master/EIPS/eip-747.md

I would signal to update EIP-747 to be hex encoded, developers can still keep their codebases with decimal values but web3.js (or any other middleware) should parse and encode any decimals values left into hex. IMO it's better to be consistent with JSON-RPC spec and pass values as hex encoded.

Show resolved Hide resolved EIPS/eip-2015.md Outdated
Show resolved Hide resolved EIPS/eip-2015.md Outdated
Show resolved Hide resolved EIPS/eip-2015.md Outdated
Show resolved Hide resolved EIPS/eip-2015.md Outdated
Show resolved Hide resolved EIPS/eip-2015.md Outdated
@axic

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@pedrouid I don't have any more comments regarding basic formatting and would be OK to merge as drafts once those are fixed. We can also keep it open as PR longer if you prefer.

@wighawag

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Nice proposal.

@pedrouid It would be better to make discussions-to link to a separate thread on ethereum magicicans or elsewhere to focus the discussion on the specificities of this proposal (as opposed to "wallet_" namespace). no ?

pedrouid added some commits May 14, 2019

@pedrouid

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Thanks @axic for the review, I've included all the formatting changes you pointed out. I've also updated the discussions-to link to a separate thread to discuss this EIP as proposed by @wighawag. Good to merge now, we will continue the discussions around this EIP on the ETH Magicians thread! 👍

@axic

axic approved these changes May 14, 2019

@axic axic merged commit 53dfa16 into ethereum:master May 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@axic

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.