-
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
refactor: split off Fork/ForkSource
from ForkStorage
#556
Conversation
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.
Agree that incremental improvements are better; just don't forget to handle all the TODOs later :)
Overall, great job, lovin' it.
// It is possible that some external nodes do not store protocol versions for versions below 9. | ||
// That's why we assume that whenever a protocol version is not present, it is unsupported by anvil-zksync. | ||
anyhow::bail!( |
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.
Does anvil-zksync support system contracts for all the protocol versions? I was under impression that it only supports the "current" protocol version.
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.
That's a fair point, I was a bit on autopilot when refactoring this part :)
This is what our "stance" used to be before the refactoring so I am just keeping it for now. I added a TODO to figure out what we want to do in reality. Supporting 20 different protocol versions does not seem feasible but likely we should support ProtocolVersion::latest()
and ProtocolVersion::next()
? Let's have a separate discussion.
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 think we should just do what we do in EN: if we encounter a custom protocol version, we fetch the base system contracts from RPC and use them. ABIs are all the same IIRC, so we only need the bytecode. Multivm already supports all the protocol versions and knows how to handle.
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.
(not suggesting to do in in this PR obv)
// TODO: This is currently cached at the `ForkStorage` level but I am unsure if this is a | ||
// good thing. Intuitively it feels like cache should be centralized in a single place. |
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.
Agree
What 💻
Closes #535
This PR basically get rids of the remaining non-inner
fork_storage
usages (there are still some tests that depend on it being exposed, to be refactored) by moving fork-related logic into a different struct as IMO these should not be so tightly coupled together. Also I reused jsonrpsee mocking functionality from zksync-era for our tests thus getting rid ofExternalStorage
.I have tested forking functionality manually and it seems to be working fine.
The PR is not perfect by any means but I have already spent too much time on this so IMO an improvement is better than no improvement.
Why ✋
Forking is the place with the most amount of tech debt so having better code here is very valuable