-
Notifications
You must be signed in to change notification settings - Fork 82
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.
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
Feat(engine): EIP-2930 support #181
Conversation
…t work properly without upstream changes. Also need to add tests
Started a review, but I'll get around to it first thing in the morning. Bit tired right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make a PR targeting this that is WIP.
evm = { git = "https://github.com/aurora-is-near/sputnikvm.git", rev = "09d4fe09dcb5fcabed8c1076699c8a2e70f14c23", default-features = false } | ||
evm-core = { git = "https://github.com/aurora-is-near/sputnikvm.git", rev = "09d4fe09dcb5fcabed8c1076699c8a2e70f14c23", default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously, this is temporary. An issue should be raised regarding this to track it.
src/precompiles/mod.rs
Outdated
addresses: vec![ | ||
ECRecover::<AuroraStackState>::ADDRESS, | ||
SHA256::<AuroraStackState>::ADDRESS, | ||
RIPEMD160::<AuroraStackState>::ADDRESS, | ||
ExitToNear::<AuroraStackState>::ADDRESS, | ||
ExitToEthereum::<AuroraStackState>::ADDRESS, | ||
] | ||
.into_iter() | ||
.map(Address) | ||
.collect(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will end up being pointless to do and store and may end up costing the user unnecessary gas. Only needs to be done for Berlin. I would change these for each hard fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there will be any impact on gas because
(a) the ones which are never used will be optimized out of the wasm artifact on compilation (since we minimize code size), and
(b) making Istanbul addresses is not necessary, but I don't think it will ever be done in production because this code will likely only be deployed after we actually switch to Berlin.
I included the addresses in the other hard forks for conceptual completeness. That said, if you don't think that conceptual completeness is adding value to people looking at our codebase then I'm fine removing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gives me an idea of how we possibly should be doing these. Ala, builders with different EIPs. That would be a fantastic improvement and allow us to be more adaptable with other projects i.e BSC if they choose to have their own improvements to their protocol, or not include some of Ethereum's.
I am working on a bit of a change to this, I'm about 95% happy with it, but an obvious choice for me would be to:
a) Have precompiles in it's own lib
b) Have builders which allows us to choose our own fork or individual EIPs
That would in the end save us time if we choose to connect to ETH forks or other EVMs.
src/precompiles/mod.rs
Outdated
addresses: vec![ | ||
ECRecover::<AuroraStackState>::ADDRESS, | ||
SHA256::<AuroraStackState>::ADDRESS, | ||
RIPEMD160::<AuroraStackState>::ADDRESS, | ||
Identity::<AuroraStackState>::ADDRESS, | ||
modexp::ADDRESS, | ||
bn128::addresses::ADD, | ||
bn128::addresses::MUL, | ||
bn128::addresses::PAIR, | ||
Blake2F::<AuroraStackState>::ADDRESS, | ||
ExitToNear::<AuroraStackState>::ADDRESS, | ||
ExitToEthereum::<AuroraStackState>::ADDRESS, | ||
] | ||
.into_iter() | ||
.map(Address) | ||
.collect(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up being repeated code from Istanbul. Though admittedly, trying to come up with a nice looking clean solution is proving to be difficult...
It's a good idea to push this in and have it followed up with #182. I got a little bit carried away admittedly. |
* Add initial work * Rest of changes * More verbose
Oops, forgot that I meant to target |
* JSON: fix bugs and add unit tests. (#141) * Add storage layout debug support for `EvmErc20.sol`. (#178) * ERC-20: forbid using invalid NEP-141 AccountID for mapping. (#179) * Add EIP-2930 support. (#181, #182) * Migrate all workflows to self-hosted runners. (#185) * Speed up the workflow using build caching. (#189) * Use the new math API host functions. (#190) * Fix `clippy::enum_variant_names` warning. (#192) * Add different networks to the Makefile. (#193) * Update the network status in the README. (#194) * Remove the toolchain installation step in workflows. (#195) * Run all tests for all networks in CI. (#196) * Optimize for performance instead of code size. (#197) * Parallelize the test suites. (#198) * Add build-caching to the testing workflow. (#201) * Refactor tests to use Signer. (#203) * Add options to the bench profile. (#204) * Remove a duplicate test. (#205) * Add a sanity test for access list handling. (#206) * Update nearcore to the latest branch. * Add feature gates to the SDK's new host functions. Co-authored-by: Ahmed Ali <ahmed@aurora.dev> Co-authored-by: Dmitry Strokov <dmitry@aurora.dev> Co-authored-by: Evgeny Ukhanov <evgeny@aurora.dev> Co-authored-by: Joshua J. Bouw <joshua@aurora.dev> Co-authored-by: Kirill <kirill@aurora.dev> Co-authored-by: Michael Birch <michael@aurora.dev>
See https://eips.ethereum.org/EIPS/eip-2930 for spec
Note: there is a PR for upstream changes, but it is not yet merged, so I depend on our fork directly for the time being. We'll have to fix this once upstream changes are accepted.