-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Adds snapshot map to anvil metadata #6364
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.
LGTM functionality wise, I think it's fine on this endpoint. Deferring to @mattsse
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.
reasonable,
smol nit so json object layout is deterministic
crates/anvil/core/src/types.rs
Outdated
@@ -217,6 +219,7 @@ pub struct AnvilMetadata { | |||
pub latest_block_number: u64, | |||
pub latest_block_hash: H256, | |||
pub forked_network: Option<ForkedNetwork>, | |||
pub snapshots: HashMap<U256, (u64, H256)>, |
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 should be BtreeMap
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.
Updated. but curious, why is this important? or is it just for visual consistency?
(a bit more nitpicky, but perhaps the whole Backend::active_snapshots
be a btree from the start, to avoid conversion here, and perhaps to slightly optimize its cleanup?
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.
or is it just for visual consistency?
just for that yes
but perhaps the whole Backend::active_snapshots
yeah that'd be easier, but since we only need this for this rpc call, should be fine to collect
test failures unrelated #6371 |
Motivation
I need external access to the list of anvil snapshots
my use case: I want to expose snapshot/revert functionality on Iron wallet, but it should be able to support snapshots created outside of it too. there's currently no way to get a list of snapshots unless I control their creation too
Solution
Adding them to the
anvil_metadata
endpoint seemed like the intuitive solution. Not sure if there are unintended side-effects behind this? I can move to a dedicated endpoint if needed