-
Notifications
You must be signed in to change notification settings - Fork 79
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(cli): adds --dump-state
cli cmd
#496
Conversation
Also, would love to see an e2e test for this |
@itegulov added e2e tests for |
ae74565
to
dd53c60
Compare
dd53c60
to
127327e
Compare
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.
Alright looks good to me. Happy to merge this if you want, all my suggestions are small and can come in a separate PR
tempdir = "0.3.7" | ||
maplit = "1.0.2" | ||
zksync-web3-rs = "0.1.1" | ||
ethers = { version = "2.0.4", features = ["rustls"] } |
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.
Didn't realize we are still using ethers somewhere, that's interesting...
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.
Yes, its used in unit tests in core.
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.
dump_state, | ||
dump_interval, | ||
preserve_historical_states, | ||
)); |
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.
You don't have to spawn it, you can just use it directly in select!
as it is a future. This way you will be able to use state_dumper.dump()
after the select like you were doing it before my changes.
Also, I have just realized I made the same mistake with block_producer_handle
below, if you don't mind removing tokio::task::spawn
there too that would be great
@@ -10,7 +10,7 @@ categories = ["cryptography"] | |||
publish = false | |||
|
|||
[dependencies] | |||
alloy-zksync = { git = "https://github.com/itegulov/alloy-zksync.git", rev = "692c5c2ca5defc88ac542f420d97c6756dadf9df" } | |||
alloy-zksync = { git = "https://github.com/popzxc/alloy-zksync.git", rev = "6a2e3cfe41cae9f92dc6672dbd0ce56ccc37e9b3" } |
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.
👍
@@ -10,7 +10,7 @@ categories = ["cryptography"] | |||
publish = false | |||
|
|||
[dependencies] | |||
alloy-zksync = { git = "https://github.com/itegulov/alloy-zksync.git", rev = "692c5c2ca5defc88ac542f420d97c6756dadf9df" } | |||
alloy-zksync = { git = "https://github.com/popzxc/alloy-zksync.git", rev = "6a2e3cfe41cae9f92dc6672dbd0ce56ccc37e9b3" } | |||
alloy = { version = "0.6", features = ["full", "rlp", "serde", "sol-types", "getrandom", "provider-anvil-api", "json-rpc"] } |
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.
Created an issue for upgrading alloy to match alloy-zksync #504
(nothing actionable here just noting)
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.
Will pick #504 once this is merged
|
||
const SOME_ORIGIN: HeaderValue = HeaderValue::from_static("http://some.origin"); | ||
const OTHER_ORIGIN: HeaderValue = HeaderValue::from_static("http://other.origin"); | ||
const ANY_ORIGIN: HeaderValue = HeaderValue::from_static("*"); | ||
|
||
use anvil_zksync_core::node::VersionedState; | ||
use std::{ fs, convert::identity, thread::sleep, time::Duration }; | ||
use tempdir::TempDir; |
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.
Please move these above the constants. Also seems like rustfmt+clippy are not enforced here so I created a separate issue #505
In general I recommend enabling rustfmt-based auto-formatting in your editor (not sure what you use but there should be an option for almost anything)
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.
provider.tx().finalize().await?; | ||
|
||
// Allow some time for the state to be dumped | ||
sleep(Duration::from_secs(2)); |
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 this wouldn't be necessary if you added state_dumper.dump()
at the end of the program like I suggested in that other thread, right?
|
||
let dumped_data = fs::read_to_string(&dump_path)?; | ||
let state: VersionedState = serde_json::from_str(&dumped_data) | ||
.map_err(|e| anyhow::anyhow!("Failed to deserialize state: {}", e))?; |
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.
nit: use .with_context
for better readability
|
||
let dumped_data = fs::read_to_string(&dump_path)?; | ||
let state: VersionedState = serde_json::from_str(&dumped_data) | ||
.map_err(|e| anyhow::anyhow!("Failed to deserialize state: {}", e))?; |
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.
nit: same thing here
"state_dump.json should contain at least one block" | ||
); | ||
assert!( | ||
!state.transactions.is_empty(), |
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 would also assert that it contains that one transaction we executed at the beginning
"state_dump_fork.json should contain at least one block" | ||
); | ||
assert!( | ||
!state.transactions.is_empty(), |
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.
Same here
Will make changes in subsequent PR here: #502 |
What 💻
--dump-state
cli cmd--state-interval
cli cmdWhy ✋
Evidence 📷
Include screenshots, screen recordings, or
console
output here demonstrating that your changes work as intendedUsage
Run the following command, wait ~60 secs and view
state.json
file:Forking: