Change net.version to eth.chainId for default transaction params #1378
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I am not sure why in |
pipermerriam
approved these changes
Jul 11, 2019
|
Thank you @cducrest! |
- Adapt chainId validation for transactions - Adapt test to use chainId instead of net_version for chain id
ghost
mentioned this pull request
saschagoebel
pushed a commit
to trustlines-protocol/blockchain
that referenced
this pull request
Jul 26, 2019
ethpm has been integrated into web3. That means we can get rid of the workaround for missing dependencies. Before upgrading to this version, the standalone ethpm package has to be manually uninstalled. The following PR has been merged into 5.0.0b4, which is the primary reason for the upgrade: ethereum/web3.py#1378
saschagoebel
added a commit
to trustlines-protocol/blockchain
that referenced
this pull request
Jul 26, 2019
* Add missing gas price parameter * Make loglevel configurable via environment * Config file is optional, because the application can be configured via Environment as well. * Improve parameter validation & implicitly typecast int and float values. Invalid values will throw a ValueError anyway. * Make RPC timeouts configurable * Sort config keys * Update Example Config with Ropsten Test Values * Formatting * Fix tests * Fix float check & change poll interval to int * Make event fetcher a bit more verbose on info level * Update Test-Token Address * Use Checksummed address * Add missing library versions to constraints.txt * Rename token_contract_address to foreign_chain_token_contract_address * Fix Nonce Calculation, GasPrice & ChainId * Cleanup pre-commit, so that just python3 is required * Fix tests, but this will likely not work in production (chainId) * Update example to reflect new config parameter name for token bridge * Formatting * Upgrade web3 from 5.0.0b2 to 5.0.0b4 (#263) ethpm has been integrated into web3. That means we can get rid of the workaround for missing dependencies. Before upgrading to this version, the standalone ethpm package has to be manually uninstalled. The following PR has been merged into 5.0.0b4, which is the primary reason for the upgrade: ethereum/web3.py#1378
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was wrong?
On a chain using a different network id and chain id, the default parameters for a transaction would be incorrectly filled when using Parity / Geth. It would use the
net_versionRPC instead ofeth_chainIdParity's net_version RPC: https://wiki.parity.io/JSONRPC-net-module#net_version
Parity's eth_chainId RPC: https://wiki.parity.io/JSONRPC-eth-module#eth_chainid
Geth net_version RPC: https://github.com/ethereumproject/go-ethereum/wiki/JSON-RPC#net_version
Geth eth_chainId RPC: https://github.com/ethereumproject/go-ethereum/wiki/JSON-RPC#eth_chainId
Related to Issue #1293
How was it fixed?
Change the
TRANSACTION_DEFAULTSto use web3.eth.chainIdThis works for Parity and Geth afaik. It does not comply with EIP1474: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1474.md#description-4
But as noted in the related issue, the EIP1474 should be updated and the EIP is in draft state.
I could not write a proper test for it since I would need a test chain with a different network and chain id, and could not find a relatively quick way to do that.
Cute Animal Picture
Edit: I know I messed up and wrote
chaindIdinstead ofchainId, also looking into fixing some tests.