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

#1718 - adds option to write anvil config to json file #1854

Merged
merged 5 commits into from
Jun 7, 2022

Conversation

robertabbott
Copy link
Contributor

Adds the --config-out <out_file> option to anvil. This writes the same info that anvil writes to stdout, in a more machine-friendly format, as json to the file specified by the user.

Motivation

#1718

Solution

pretty straightforward, if you include --config-out <out_file> it writes the relevant config info to <out_file>. note: it will still write the file it silent and --config-out are provided since i assumed silent is really referring to stdout/err

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Minor nits to make it more idiomatic - lgtm otherwise!

nice first issue, almost joined the Citadel ✅

let mut available_accounts = Vec::with_capacity(self.genesis_accounts.len());
let mut private_keys = Vec::with_capacity(self.genesis_accounts.len());

for (_, wallet) in self.genesis_accounts.iter().enumerate() {
Copy link
Member

Choose a reason for hiding this comment

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

Don't need the iter/enumerate, can just do "for wallet in &self.genesis_accounts"

wallet_description.insert("mnemonic".to_string(), phrase);
};

if let Some(fork) = fork {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of declaring it at the beginning of the function, you can do:

let config_as_json = if foo { 1 } else { 2};.

More idiomatic, saves you from having to declare the type too and more compact

Also given we know it's a json from the type, you can just call it "config "

@@ -272,6 +438,13 @@ impl NodeConfig {
self
}

/// Sets file to write config info to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Sets file to write config info to
/// Sets the file path to write the Anvil node's config info to.

let config_out = self.config_out.as_deref().unwrap();
let f =
File::create(config_out).expect("Unable to create anvil config description file");
let mut f = BufWriter::new(f);
Copy link
Member

Choose a reason for hiding this comment

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

I think you probably can instead avoid the "serde_json::to string" in as_json, and return the serde_json::Value type and here do serde_json::to_writer. Basically same thing but without you needing to manage the file descriptor and do write_all.

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

💯

@gakonst gakonst merged commit 75d366d into foundry-rs:master Jun 7, 2022
@robertabbott robertabbott deleted the bobby-anvil-out-file branch June 7, 2022 19:24
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.

2 participants