solana icon indicating copy to clipboard operation
solana copied to clipboard

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

Open samkim-crypto opened this issue 1 year ago • 4 comments

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 https://github.com/solana-labs/solana/pull/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 https://github.com/solana-labs/solana/pull/33276#discussion_r1338018139 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 avatar Oct 01 '23 23:10 samkim-crypto

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     

codecov[bot] avatar Oct 02 '23 17:10 codecov[bot]

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

t-nelson avatar Oct 20 '23 20:10 t-nelson

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!

CriesofCarrots avatar Oct 20 '23 21:10 CriesofCarrots

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

samkim-crypto avatar Oct 20 '23 21:10 samkim-crypto

Subsumed by https://github.com/solana-labs/solana/pull/33802, https://github.com/solana-labs/solana/pull/34678, and https://github.com/solana-labs/solana/pull/34989.

samkim-crypto avatar Jan 30 '24 06:01 samkim-crypto