Skip to content
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

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

naps62
Copy link
Contributor

@naps62 naps62 commented Nov 20, 2023

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

Copy link
Member

@DaniPopes DaniPopes left a 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

Copy link
Member

@mattsse mattsse left a 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

@@ -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)>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be BtreeMap

Copy link
Contributor Author

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?

Copy link
Member

@mattsse mattsse Nov 20, 2023

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

@mattsse
Copy link
Member

mattsse commented Nov 20, 2023

test failures unrelated #6371

@mattsse mattsse merged commit 6280cd4 into foundry-rs:master Nov 20, 2023
13 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants