Implement EIP-2124#11456
Conversation
|
It looks like @vorot93 signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
ordian
left a comment
ordian
left a comment
There was a problem hiding this comment.
This looks like a great start!
The thing I'm not sure about is where it should live in a separate crate or be part of util/network-devp2p.
We also want to add integration with config like this https://github.com/ethereum/go-ethereum/blob/976a0f5558e20ed7cb7ba2cd68d7429d1ef01db9/core/forkid/forkid.go#L205 so we can use the test cases from https://eips.ethereum.org/EIPS/eip-2124.
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
A few comments, looks good.
As for @ordian's question if this merits its own crate or not, I guess it depends a bit on what the rest of the code turns out to be. I'm not against mini-crates, but maybe it's just easier to keep this with the rest of the networking code. If your plan is to continue with the rest of the implementation in this PR then I guess it'll pan out where it should live as work continues?
| "chainspec", | ||
| "ethcore/wasm/run", | ||
| "evmbin", | ||
| "util/EIP-2124" |
There was a problem hiding this comment.
The only reason crates get listed here is if they provide binaries that need to be built as part of the whole, like evmbin, chainspec etc.
Other crates in the repo are dependencies of one or more of the above so they don't need to appear here.
(It's a perfectly fine thing to do while working on it ofc)
There was a problem hiding this comment.
I agree and it is only listed here to make Cargo work before it is included in parity dependency tree.
There was a problem hiding this comment.
this can be removed when it's used in the main binary
|
This is just a draft PR, since the actual compatibility check is WIP. I didn't know where to place the code, devp2p or higher up the stack, so temporarily settled on making it a separate crate. |
|
It's done, the only question is where to move it to. |
ordian
left a comment
ordian
left a comment
There was a problem hiding this comment.
The code looks good, but we should have some docs.
ordian
added
A8-looksgood 🦄
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
LGTM.
Don't forget to cleanup Cargo.toml! :)
CI seems to have some kind of problem atm, I kicked off a retry.
As for the continuation of this work, perhaps it would be useful if you wrote up an issue with a tentative implementation plan so we have a place for discussion, wdyt?
|
Do you want it to stay as a separate crate or should I move the code elsewhere? |
I think it can be where it is for the time being. If it fits better elsewhere we can move it to the proper place in the next PR. Works for you? |
|
@dvdplm Sure! |
…pstream * master: upgrade some of the dependencies (#11467) Some more release track changes to README.md (#11465) Update simple one-line installer due to switching to a single stable release track (#11463) update Dockerfile (#11461) Implement EIP-2124 (#11456) [eth classic chainspec]: remove `balance = 1` (#11459) just to make sure we don't screw up this again (#11455) backwards compatible call_type creation_method (#11450) gcc to clang (#11453) Avoid copies (#11451) Cargo.lock: cargo lock translate (#11448)
* master: Use parity-crypto updated to use upstream rust-secp256k1 (#11406) Cleanup some code in Aura (#11466) upgrade some of the dependencies (#11467) Some more release track changes to README.md (#11465) Update simple one-line installer due to switching to a single stable release track (#11463) update Dockerfile (#11461) Implement EIP-2124 (#11456)
4 participants
Closes #11116