solana
solana copied to clipboard
[clap-v3-utils] Deprecate signer validation and update parsing for clap-v3
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 callmatches.get_one::<SignerSource>(...)
to validate and parse the signer source. To define a parser forSignerSource
(to callclap::builder::ValueParser::from(...)
on theSignerSource
type), theSignerSource
has to implementClone
. I thought addingClone
toSignerSource
was okay since sensitive information is not actually parsed withSignerSource
, but later when we callsigner_from_path
orkeypair_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
intoparser::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)]
insolana-keygen
andsolana-zk-keygen
. These two crates will be addressed on follow-up to remove deprecated functions. I also either removed deprecated functions inclap-v3-utils
or added#[allow(deprecated)]
to already deprecated functions for the CI.
Fixes #
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
this is a really huge change set. are there sane places to break it up? <500lns total changes per would be great
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!
Yes, sorry it is my fault. A lot of the line changes are (un)doing refactors, so I can divide that part up.
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.