Add EIP-1344 Chain ID #1803
Add EIP-1344 Chain ID #1803
Conversation
c61ad37 to
acaf9a0
Compare
|
@pipermerriam got it working, just missing test cases, and the updated test suite which includes EIP-1344 tests (which is WIP IIRC) How do you feel about this approach? |
5e4442f to
accf6ad
Compare
There was a problem hiding this comment.
Brian, I just landed #1805 which is going to cause you a merge conflict. Let me know if you need help sorting it out.
One thing you'll need to do is create a ChainContextAPI class in the eth.abc module and use that in all the places you have type hints. Note that the BaseVM no longer exists and you'll want migrate your changes over to eth.abc.VirtualMachineAPI
d2a5b0c to
cb14d1b
Compare
|
Refactored and it works just fine. Not sure if a failure was expected, but MyPy made it really easy work. |
|
from gitter:
Rather than wait on state tests to merge, can you add some local tests, like #1829 does? Then we can get it merged up, and add in the state tests as they come in. Don't forget to add a release note to mention that the EIP is added to Istanbul (there's also an example of that in #1829). |
|
@carver I added a simple test and release notes for the feature. Any further suggestions to get this merged? Also, any suggestions on what to clean up with the commit history, if necessary? |
eth/vm/chain_context.py
Outdated
|
|
||
| if chain_id is None: | ||
| chain_id = 0 # Default value (invalid for public networks) | ||
| validate_uint256(chain_id) |
There was a problem hiding this comment.
Note: Should be uint64, which is the suggested maximum due to the mathematical configuration of EIP-155 (a hack made UINT256_MAX/2 - 36 the maximum number that could be used for the chain ID field)
refactor: Add ChainContext to hold/retreive chain_id bug: Forgot to add ChainContext to Chain.get_vm() bug: Modified test cases that initialize VM class bug: Forget to add chain ID to class vars refactor: Made ChainContext and immutable object - also added docstrings and some additional validation bug: Handle optional chain id config refactor: Removed unused Base objects refactor: Using ChainContextAPI ABC now test: Added test for Chain ID doc: Added release note
85b2e28 to
ea2bac1
Compare
|
In the interest of time, I just added a few little tweaks (and handled the rebase). If anything looks off to you, let me know, and we can do a follow-up. |
|
The tweaks look good! |
What was wrong?
Fixes #1817
How was it fixed?
Added a new
ChainContextclass, which allows the chain configuration (and other external parameters) to inform the VM class of that outside data. Routed the Chain ID configuration parameter through this new class.Cute Animal Picture