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] Deprecate signer validation and update parsing for clap-v3 #33478

Closed
wants to merge 17 commits into from
Closed

[clap-v3-utils] Deprecate signer validation and update parsing for clap-v3 #33478

wants to merge 17 commits into from

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Oct 1, 2023

Problem

In clap-v3, the Arg::validator is deprecated and replaced with Arg::value_parser. The idea is that when we validate an argument input, we generally parse the input, so dividing validation and parsing in separate steps forces you to parse input twice.

A previous PR #33276 deprecated most of the validation functions in clap-v3-utils and replaced them with parsing functions. This PR is a follow-up that deprecates validation functions related to the signer.

Summary of Changes

Sorry this follow-up PR took such a long time. I actually had this PR open for a couple weeks, but I never asked for a review.

To make clap-v3-utils implement clap parsers idiomatically, I made some non-trivial amount of changes including making the SignerSource a public type. I am very open to the reviewers' opinions and am happy to revert any of the changes I made in this PR, so please let me know what you think.

  • 703c5a5: The most idiomatic way to define a parser for signer related arguments was to make SignerSource be a public type and let applications call matches.get_one::<SignerSource>(...) to validate and parse the signer source. To define a parser for SignerSource (to call clap::builder::ValueParser::from(...) on the SignerSource type), the SignerSource has to implement Clone. I thought adding Clone to SignerSource was okay since sensitive information is not actually parsed with SignerSource, but later when we call signer_from_path or keypair_from_path. Please let me know otherwise.
  • bf4e1f7: I thought this was an appropriate place to use a builder pattern to build a SignerSource parser, so I added it.
  • 7fb9565, 36f5f20, 4fb271f, 11500e7, 089c97b: I deprecated validation functions related to keypairs. These functions were refactored into parser::signer from the previous PR. Since they are deprecated anyway, I moved them back to their original place (as suggested [clap-v3-utils] Deprecate input validators and add parsers to replace them #33276 (comment) previously). I should have put them back in the previous PR 🙏 ...
  • cb698b8, 76d5f62: I refactored SignerSource into parser::signer and made it into a public type.
  • 81ca157, 27518c1, 22561e1, 370c43f, 7f09abc: I deprecated some of the signer related functions and replace them with associated functions to SignerSource. I also returned these deprecated functions back to its original place... sorry for the mess.
  • 01f5656: I added #[allow(deprecated)] in solana-keygen and solana-zk-keygen. These two crates will be addressed on follow-up to remove deprecated functions. I also either removed deprecated functions in clap-v3-utils or added #[allow(deprecated)] to already deprecated functions for the CI.

Fixes #

@samkim-crypto samkim-crypto added the work in progress This isn't quite right yet label Oct 1, 2023
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

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

Comparison is base (c0fbfc6) 81.8% compared to head (dc3ed30) 81.7%.
Report is 636 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #33478     +/-   ##
=========================================
- Coverage    81.8%    81.7%   -0.1%     
=========================================
  Files         806      806             
  Lines      217850   218107    +257     
=========================================
+ Hits       178269   178402    +133     
- Misses      39581    39705    +124     

@samkim-crypto samkim-crypto removed the work in progress This isn't quite right yet label Oct 16, 2023
@t-nelson
Copy link
Contributor

this is a really huge change set. are there sane places to break it up? <500lns total changes per would be great

@CriesofCarrots
Copy link
Contributor

Yeah, apologies. I've been dragging my feet on reviewing because I don't want to miss something. If there's any way to break it up, that would make me much more confident!

@samkim-crypto
Copy link
Contributor Author

Yes, sorry it is my fault. A lot of the line changes are (un)doing refactors, so I can divide that part up.

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 6, 2023
@samkim-crypto samkim-crypto removed the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 8, 2023
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 23, 2023
@samkim-crypto samkim-crypto removed the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 29, 2023
@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Dec 14, 2023
@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Jan 1, 2024
@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Jan 16, 2024
@samkim-crypto
Copy link
Contributor Author

Subsumed by #33802, #34678, and #34989.

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