-
Notifications
You must be signed in to change notification settings - Fork 246
Allow saving network in snfoundry.toml
#4019
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
base: master
Are you sure you want to change the base?
Conversation
…rknet-foundry into allow-using-url-from-config-when-using-account-create-or-import
…thub.com/foundry-rs/starknet-foundry into allow-network-in-config
…thub.com/foundry-rs/starknet-foundry into allow-network-in-config
cptartur
left a comment
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.
Left a comment
…t` (#4013) <!-- Reference any GitHub issues resolved by this PR --> **Stack** - #4020 - #4019 - #4017 - #4013 ⬅ Closes # ## Introduced changes When using e.g. `account create` without explicitly passing `--url` or `--network` and having `url` defined in `snfoundry.toml`, we should use the one from config. This PR fixes that. ## Checklist <!-- Make sure all of these are complete --> - [ ] Linked relevant issue - [x] Updated relevant documentation - [x] Added relevant tests - [x] Performed self-review of the code - [x] Added changes to `CHANGELOG.md`
<!-- Reference any GitHub issues resolved by this PR --> **Stack** - #4020 - #4019 - #4017 ⬅ - #4013 Closes # ## Introduced changes <!-- A brief description of the changes --> - ## Checklist <!-- Make sure all of these are complete --> - [ ] Linked relevant issue - [ ] Updated relevant documentation - [x] Added relevant tests - [x] Performed self-review of the code - [ ] Added changes to `CHANGELOG.md`
… into allow-network-in-config
…rs/starknet-foundry into allow-network-in-config
… into allow-network-in-config
| pub fn validate(&self) -> anyhow::Result<()> { | ||
| match (&self.url, &self.network) { | ||
| (Some(_), Some(_)) => { | ||
| anyhow::bail!("Only one of `url` or `network` may be specified") | ||
| } | ||
| _ => Ok(()), | ||
| } |
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.
Maybe let's add an todo for your issue somewhere in code? #4027
| } else { | ||
| unreachable!( | ||
| "`--url` or `--network` must be provided or url must be set in snfoundry.toml" | ||
| "`--url` or `--network` must be provided or url or network fields must be set in snfoundry.toml" |
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.
| "`--url` or `--network` must be provided or url or network fields must be set in snfoundry.toml" | |
| "`--url` or `--network` must be provided or one of url or network field must be set in snfoundry.toml" |
|
|
||
| #[derive(ValueEnum, Clone, Copy, Debug, PartialEq)] | ||
| #[derive(ValueEnum, Clone, Copy, Debug, PartialEq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "lowercase")] |
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.
Why do we need this change now?
| url = "http://some.url" | ||
| network = "sepolia" |
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.
Is having both correct? The validation we have for configs says it shouldn't be.
| account: account_name.into(), | ||
| accounts_file: accounts_file.into(), | ||
| keystore, | ||
| keystore: keystore.map(std::convert::Into::into), |
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.
Why do we need mapping here?
| let (url, network) = if rpc_args.url.is_some() || rpc_args.network.is_some() { | ||
| (rpc_args.url.clone(), rpc_args.network) | ||
| } else { | ||
| (config.url.clone(), config.network) | ||
| }; |
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.
Is the case where network is in config and url in rpc_args and vice versa not possible?
Stack
--add-profileflag #4020networkin snfoundry.toml #4019 ⬅url::Urlinstead of string across sncast #4017urlfrom config foraccount createandaccount import#4013Closes #3250
Introduced changes
Checklist
CHANGELOG.md