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] Add functions to parse directly from SignerSource #34678

Merged
merged 5 commits into from
Jan 24, 2024
Merged

[clap-v3-utils] Add functions to parse directly from SignerSource #34678

merged 5 commits into from
Jan 24, 2024

Conversation

samkim-crypto
Copy link
Contributor

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

Problem

As described in #33478, the Arg::validator is deprecated in clap-v3. The approach specified in #33478 is to naturally use SignerSource as an intermediate type so that applications can call matches.get_one::<SignerSource>(...) to idiomatically validate and parse signer source.

A previous PR #33802 added a way to flexibly parse SignerSource. What we need now is to parse Signer, Keypair, or Pubkey from a source.

Summary of Changes

  • 87f0cb1: Added _from_source variants of the functions that parse Signer, Keypair, or Pubkey from "path".
  • a00e8fc: Moved the private function parse_signer_source to be an associated function of SignerSource to make the code cleaner.
  • 31f1183: Made SignerSource a public type and moved it into input_parsers::signer.
  • a300fdb: Made the _from_source functions public so that downstream cli projects can use it to parse Signer, Keypair, or Pubkey.

Combined with #33802, this PR replaces the original PR #33478.

Fixes #

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

codecov bot commented Jan 7, 2024

Codecov Report

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

Comparison is base (f9bfb60) 81.7% compared to head (7b3f85b) 81.7%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34678     +/-   ##
=========================================
- Coverage    81.7%    81.7%   -0.1%     
=========================================
  Files         826      826             
  Lines      223371   223411     +40     
=========================================
+ Hits       182578   182588     +10     
- Misses      40793    40823     +30     

@samkim-crypto samkim-crypto removed the work in progress This isn't quite right yet label Jan 23, 2024
@@ -371,148 +367,6 @@ impl DefaultSigner {
}
}

#[derive(Debug, Clone)]
Copy link
Contributor

Choose a reason for hiding this comment

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

moving all of these public symbols without leaving re-exports will break all consumers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry if I am misunderstanding this, but Is this the case even if SignerSource is currently restricted within the clap-v3-utils crate?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah i missed that they were both moved and promoted to public simultaneously. generally prefer not to do cosmetic changes like reorging private symbols in the same change set as functionals

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

I'm kicking myself for putting SignerSource in clap-utils at all. The clap dependencies could've been genericized over a couple of closures 🤦

@@ -371,148 +367,6 @@ impl DefaultSigner {
}
}

#[derive(Debug, Clone)]
Copy link
Contributor

Choose a reason for hiding this comment

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

ah i missed that they were both moved and promoted to public simultaneously. generally prefer not to do cosmetic changes like reorging private symbols in the same change set as functionals

@samkim-crypto samkim-crypto merged commit 3004eaa into solana-labs:master Jan 24, 2024
36 checks passed
@samkim-crypto samkim-crypto added the v1.18 PRs that should be backported to v1.18 label Feb 29, 2024
Copy link
Contributor

mergify bot commented Feb 29, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify bot pushed a commit that referenced this pull request Feb 29, 2024
…34678)

* add `_from_source` function variants for signer, keypair, and pubkey

* make `parse_signer_source` an associated function of `SignerSource`

* refactor `SignerSource` into `input_parsers::signer`

* make `_from_source` functions public

* remove unnecessary import

(cherry picked from commit 3004eaa)
samkim-crypto added a commit that referenced this pull request Mar 1, 2024
…urce` (backport of #34678) (#35384)

[clap-v3-utils] Add functions to parse directly from `SignerSource` (#34678)

* add `_from_source` function variants for signer, keypair, and pubkey

* make `parse_signer_source` an associated function of `SignerSource`

* refactor `SignerSource` into `input_parsers::signer`

* make `_from_source` functions public

* remove unnecessary import

(cherry picked from commit 3004eaa)

Co-authored-by: samkim-crypto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.18 PRs that should be backported to v1.18
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants