cli: Fix multisig parsing #72
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
#### Problem As pointed out in solana-program#58, the CLI currently fails when just running `create-account <MINT_ADDRESS>`, because a multisig argument is expected during all commands. Clap v3's version of `values_of` gives an error if the arg is not configured in the command. This was probably missed during the port over to clap v3. #### Summary of changes Do the same thing as elsewhere, and change `values_of` to `try_get_many(...).ok().flatten()`. This is the last place using `values_of` in the CLI that could fail.
clients/cli/src/config.rs
Outdated
| @@ -48,9 +48,9 @@ fn signers_of( | |||
| name: &str, | |||
| wallet_manager: &mut Option<Rc<RemoteWalletManager>>, | |||
| ) -> Result<Option<SignersOf>, Box<dyn std::error::Error>> { | |||
| if let Some(values) = matches.values_of(name) { | |||
| if let Some(values) = matches.try_get_many(name).ok().flatten() { | |||
There was a problem hiding this comment.
Suggested change
| if let Some(values) = matches.try_get_many(name).ok().flatten() { | |
| if let Some(values) = matches.try_get_many::<String>(name).ok().flatten() { |
In clap-v3, we generally always have to specify the type that we are parsing for. We should parse this to a proper type rather than a String, but that will require update the updating the validation function is_valid_signer, which will follow after solana-labs/solana-program-library#7447. So let's just update it to String here for now.
clients/cli/src/config.rs
Outdated
| let mut results = Vec::new(); | ||
| for (i, value) in values.enumerate() { | ||
| for (i, value) in values.copied().enumerate() { |
There was a problem hiding this comment.
Suggested change
| for (i, value) in values.copied().enumerate() { | |
| for (i, value) in values.enumerate() { |
We are parsing for an owned String type, so we can remove the copied().
|
Ok great, thanks for the suggestions -- this should be good for another look! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
As pointed out in #58, the CLI currently fails when just running
create-account <MINT_ADDRESS>, because a multisig argument is expected during all commands. Clap v3's version ofvalues_ofgives an error if the arg is not configured in the command.This was probably missed during the port over to clap v3.
Summary of changes
Do the same thing as elsewhere, and change
values_oftotry_get_many(...).ok().flatten().This is the last place using
values_ofin the CLI that could fail.Closes #58