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

[clap-v3-utils] Replace pubkeys_sigs_of with try_pubkeys_sigs_of #34801

Merged
merged 2 commits into from
Jan 20, 2024
Merged

[clap-v3-utils] Replace pubkeys_sigs_of with try_pubkeys_sigs_of #34801

merged 2 commits into from
Jan 20, 2024

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Jan 17, 2024

Problem

Due to the differences in the way value_of and is_present functions behave in clap-v2 and clap-v3, the try_ versions of different key parsers were added in #33184. However, that PR missed a point in the logic in signer_from_path_with_config where pubkeys_sigs_of is still used instead of try_pubkeys_sigs_of. The panicking behavior of pubkeys_sigs_of when called on a non pre-specified argument, it is blocking the token-cli from being upgraded to clap-v3.

Summary of Changes

I replaced the pubkeys_sigs_of with try_pubkeys_sigs_of when parsing SignerSourceKind::Pubkey.

Technically, to be consistent with the rest of the module, we should call try_pubkeys_sigs_of(matches, SIGNER_ARG.name)?. However, this changes the behavior of the function. In clap-v2, if pubkeys_sigs_of(matches, SIGNER_ARG.name) is called when SIGNER_ARG.name is not pre-specified as a clap argument, then it returns None. Therefore, instead of returning error, I invoked .ok().flatten() on the resulting result value.

There is a bigger PR in the works #34678 to clean up parsing of signer source. However, I just wanted to make sure that this change goes into 1.18, hence a smaller PR here.

Fixes #

@samkim-crypto samkim-crypto added the work in progress This isn't quite right yet label Jan 17, 2024
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (8f9d915) 81.8% compared to head (032e2bc) 81.7%.
Report is 27 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34801     +/-   ##
=========================================
- Coverage    81.8%    81.7%   -0.1%     
=========================================
  Files         825      825             
  Lines      223269   223274      +5     
=========================================
- Hits       182635   182588     -47     
- Misses      40634    40686     +52     

@samkim-crypto samkim-crypto removed the work in progress This isn't quite right yet label Jan 17, 2024
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks!

@samkim-crypto samkim-crypto merged commit c071cf5 into solana-labs:master Jan 20, 2024
35 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