Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

@0x/utils: fix wrong RPC method in getChainIdAsync() #2270

Merged
merged 5 commits into from
Oct 26, 2019

Conversation

feuGeneA
Copy link
Contributor

It was using net_version, but it should be using the eth_chainId method introduced in EIP-695. I'm not sure whether/how the network ID differs from the chain ID on mainnet and the testnets, but in Ganache in particular, the network ID is 50 while the chain ID is 1337, and this difference was causing problems for Python tests. Specifically, the Web3.py interface Web3.eth.chainId invokes eth_chainId, and the result feeds into the order hash, which wasn't lining up with the non-Python side of things.

It was using net_version, but it should be using the eth_chainId method
introduced in EIP-695.  I'm not sure whether/how the network ID differs
from the chain ID on mainnet and the testnets, but in Ganache in
particular, the network ID is 50 while the chain ID is 1337, and this
difference was causing problems for Python tests.  Specifically, the
Web3.py interface `Web3.eth.chainId` invokes the eth_chainId method, and
the result feeds into the order hash, which wasn't lining up with the
non-Python side of things.
Copy link
Contributor

@dorothy-zbornak dorothy-zbornak left a comment

Choose a reason for hiding this comment

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

👀👍

@dekz
Copy link
Member

dekz commented Oct 18, 2019

We're passing in the chainId as a parameter to Exchange deployment. So this should've been passing in 50 inside the ganache snapshot. Exchange contract keeps this value around and it doesn't actually represent the executing chainId, rather the cached value at deployment time (as a parameter). So it's possible to deploy on mainnet with chainId 1337 even though this shouldn't be possible right?

Pedro's article on chainId versus networkId helps understand the real world difference between the two.

@coveralls
Copy link

coveralls commented Oct 22, 2019

Coverage Status

Coverage remained the same at 75.286% when pulling a04da32 on fix/3.0/chainId-is-not-net-version into 59210f5 on 3.0.

@@ -1,6 +1,6 @@
{
"name": "@0x/utils",
"version": "4.6.0-beta.0",
"version": "4.6.0-beta.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't bump the version here, this is done by Lerna at publish time.

@dekz
Copy link
Member

dekz commented Oct 23, 2019

Should we bump contract-addresses to use 1337 over 50 in this PR? Also all of the dependencies relying on lookup by networkId? https://github.com/0xProject/0x-monorepo/blob/development/packages/contract-wrappers/src/contract_wrappers.ts#L114

@feuGeneA feuGeneA merged commit 0067f10 into 3.0 Oct 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants