Skip to content

Undoing the mixup between Chain ID and Network ID #777

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 23, 2018
Merged

Undoing the mixup between Chain ID and Network ID #777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
946c4ab
Undoing the mixup between Chain ID and Network ID
carver Apr 20, 2018
323c621
clarify chainId validator intention
carver Apr 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions docs/web3.net.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ The following properties are available on the ``web3.net`` namespace.

.. py:method:: Net.chainId(self)

* Delegates to ``net_version`` RPC Method
This method is not implemented. You must manually determine your chain ID for now.

Returns the current network chainId/version and is an alias of ``web3.net.version``.
It will always return `None`, which is a valid chainId to specify in the transaction.
It means the transaction (may be) replayable on other forks of the network.

.. code-block:: python

>>> web3.net.chainId
1

None

.. py:method:: Net.version(self)

Expand Down
20 changes: 10 additions & 10 deletions tests/core/contracts/test_contract_buildTransaction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def test_build_transaction_with_contract_no_arguments(web3, math_contract, build
'data': '0xd09de08a',
'value': 0,
'gasPrice': 1,
'chainId': 1
'chainId': None,
}


Expand All @@ -46,7 +46,7 @@ def test_build_transaction_with_contract_fallback_function(web3, fallback_functi
'data': '0x',
'value': 0,
'gasPrice': 1,
'chainId': 1
'chainId': None,
}


Expand All @@ -65,7 +65,7 @@ def test_build_transaction_with_contract_class_method(
'data': '0xd09de08a',
'value': 0,
'gasPrice': 1,
'chainId': 1
'chainId': None,
}


Expand All @@ -79,7 +79,7 @@ def test_build_transaction_with_contract_default_account_is_set(
'data': '0xd09de08a',
'value': 0,
'gasPrice': 1,
'chainId': 1
'chainId': None,
}


Expand All @@ -93,7 +93,7 @@ def my_gas_price_strategy(web3, transaction_params):
'data': '0xd09de08a',
'value': 0,
'gasPrice': 5,
'chainId': 1
'chainId': None,
}


Expand Down Expand Up @@ -121,31 +121,31 @@ def test_build_transaction_with_contract_to_address_supplied_errors(web3,
(
{}, (5,), {}, {
'data': '0x7cf5dab00000000000000000000000000000000000000000000000000000000000000005', # noqa: E501
'value': 0, 'gasPrice': 1, 'chainId': 1
'value': 0, 'gasPrice': 1, 'chainId': None,
}, False
),
(
{'gas': 800000}, (5,), {}, {
'data': '0x7cf5dab00000000000000000000000000000000000000000000000000000000000000005', # noqa: E501
'value': 0, 'gasPrice': 1, 'chainId': 1
'value': 0, 'gasPrice': 1, 'chainId': None,
}, False
),
(
{'gasPrice': 21000000000}, (5,), {}, {
'data': '0x7cf5dab00000000000000000000000000000000000000000000000000000000000000005', # noqa: E501
'value': 0, 'gasPrice': 21000000000, 'chainId': 1
'value': 0, 'gasPrice': 21000000000, 'chainId': None,
}, False
),
(
{'nonce': 7}, (5,), {}, {
'data': '0x7cf5dab00000000000000000000000000000000000000000000000000000000000000005', # noqa: E501
'value': 0, 'gasPrice': 1, 'nonce': 7, 'chainId': 1
'value': 0, 'gasPrice': 1, 'nonce': 7, 'chainId': None,
}, True
),
(
{'value': 20000}, (5,), {}, {
'data': '0x7cf5dab00000000000000000000000000000000000000000000000000000000000000005', # noqa: E501
'value': 20000, 'gasPrice': 1, 'chainId': 1
'value': 20000, 'gasPrice': 1, 'chainId': None,
}, False
),
),
Expand Down
15 changes: 4 additions & 11 deletions tests/core/eth-module/test_transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,13 @@
'make_chain_id, expect_success',
(
(
lambda web3: web3.version.network,
lambda web3: web3.net.chainId,
True,
),
(
lambda web3: int(web3.version.network),
True,
),
(
lambda web3: int(web3.version.network) + 1,
False,
),
(
lambda web3: str(int(web3.version.network) + 1),
pytest.param(
lambda web3: 999999999999,
False,
marks=pytest.mark.xfail,
Copy link
Contributor

Choose a reason for hiding this comment

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

@carver why did you mark this as xfail?

from web3.auto import w3
transaction = {'to': w3.eth.accounts[0], 'chainId': 9999999, 'from': w3.eth.accounts[1]}
w3.personal.unlockAccount(w3.eth.accounts[1], 'passphrase')
w3.eth.sendTransaction(transaction)

shouldn't the above code give ValidationError for chainId? In fact validation middleware doesn't seem to do what it is expected to do.
I'm not very sure if this is a regression but I'm fixing this in #756.

),
),
)
Expand Down
7 changes: 3 additions & 4 deletions web3/middleware/validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@

@curry
def validate_chain_id(web3, chain_id):
if chain_id == web3.version.network:
return None
if chain_id == web3.net.chainId:
return chain_id
else:
raise ValidationError(
"The transaction declared chain ID %r, "
"but the connected node is on %r" % (
chain_id,
web3.version.network,
"UNKNOWN",
)
)

Expand All @@ -33,7 +33,6 @@ def transaction_normalizer(transaction):

def validation_middleware(make_request, web3):
transaction_validator = apply_formatters_to_dict({
'chainId': validate_chain_id(web3),
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this is the cause for using xfail!
Read the description in the PR. However If validate_chain_id is not needed anymore, why is the code left behind?
Is this something you plan to return back to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this something you plan to return back to?

Yes. Once we can reliably discover the chainID from the connected node, then we should validate chainID again.

})

transaction_sanitizer = compose(transaction_normalizer, transaction_validator)
Expand Down
2 changes: 1 addition & 1 deletion web3/net.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def peerCount(self):

@property
def chainId(self):
return self.version
return None

@property
def version(self):
Expand Down
2 changes: 1 addition & 1 deletion web3/utils/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
'data': b'',
'gas': lambda web3, tx: web3.eth.estimateGas(tx),
'gasPrice': lambda web3, tx: web3.eth.generateGasPrice(tx) or web3.eth.gasPrice,
'chainId': lambda web3, tx: int(web3.net.version),
'chainId': lambda web3, tx: web3.net.chainId,
}


Expand Down