[clap-v3-utils] Deprecate input validators and add parsers to replace them #33276

samkim-crypto · 2023-09-16T01:01:08Z

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 parse the input any way. If we separate out validation and parsing, then for most arguments, we will be parsing the inputs twice, which could be wasteful.

The solana-clap-v3-utils as well as the solana cli tools that depend on it still uses the deprecated input Arg::validator.

Summary of Changes

I went through each validation function in clap-v3-utils and made analogous parsing functions. I thought I would divide the task up into two separate PRs. I deferred creating parsing functions for validation functions that involve the SignerSource to a follow-up PR.

I tried to break-up the commits into a sequence of steps to make it easier to review as much as possible.

f5a8168: Clap-v3 now automatically implements parsers for types that implement FromStr via the clap::value_parser!(TYPE). This means that is_pubkey or is_hash can be deprecated in favor of the automatic clap parsers. I created tests to demonstrate how this is done.
61878fb: For the pubkey=signature type arguments, I create a new type PubkeySignature with the pubkey and signature components and implemented FromStr for it. The cli code should be able to call matches.get_one::<PubkeySignature>(...) to easily parse this type as shown in the tests.
a41ea33: I created parsing functions for the next few validation functions. For the url validators, I mistakenly deprecated the parse_url and parse_url_or_moniker, but this is fixed in a future commit below 🙏 . With code refactoring, it was a little bit difficult to amend this commit.
775244e: For the amount validators, I created UiTokenAmount and RawTokenAmount structs (similar to how it is done in token-cli). I grepped all usage of is_amount and is_amount_or_all in the monorepo/spl and pretty in all the places, the amount was assumed to be either f64 or ALL, so I made the amount in UiTokenAmount to be f64 for now, but I can also add an extra UiTokenAmount variant that holds u64 as well.
dfd4c9c: Added parser for validators that involve seeds and derivation paths.
5cb906e: I updated the parts in clap-v3-utils that uses the newly deprecated functions to satisfy the CI. I also temporarily removed the deprecation for is_pubkey validation function since it is used in some validator functions that will be deprecated in a follow-up PR.
0d2f8aa: Removed some deprecated components form solana-keygen to satisfy the CI. Once I clean out the deprecated functions from solana-clap-v3-utils, I will remove the rest of the deprecated components from keygen.
2017872: The parsers that are defined in input_validators submodule should really be part of input_parsers submodule. Before moving these parsers, I first refactored the input_parsers submodule to prevent it from getting too long. All the parsing functions that involve the signer in some way, I moved it to a separate submodule.
62afa80: I fixed the deprecation mistake from above.
4c04627: I refactored all the new parsers defined in input_validators.rs to the input_parsers module.

Fixes #

codecov · 2023-09-17T00:23:03Z

Codecov Report

Merging #33276 (4e4efc9) into master (5d11227) will decrease coverage by 0.1%.
The diff coverage is 69.1%.

@@            Coverage Diff            @@
##           master   #33276     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         798      799      +1     
  Lines      217100   217328    +228     
=========================================
+ Hits       177999   178172    +173     
- Misses      39101    39156     +55

clap-v3-utils/src/input_parsers/mod.rs

clap-v3-utils/src/input_parsers/signer.rs

t-nelson

lgtm now. at least we tried to advertise the deprecation!

let's give @CriesofCarrots a day or so to take a look if she likes

CriesofCarrots

Just a couple questions from me! Generally, looks good.

clap-v3-utils/src/input_parsers/mod.rs

CriesofCarrots · 2023-09-27T03:48:38Z

clap-v3-utils/src/input_parsers/mod.rs

+    let value = arg.replace('\'', "");
+    let mut parts = value.split('/');
+    let account = parts.next().unwrap();
+    account
+        .parse::<u32>()
+        .map_err(|e| format!("Unable to parse derivation, provided: {account}, err: {e}"))
+        .and_then(|_| {
+            if let Some(change) = parts.next() {
+                change.parse::<u32>().map_err(|e| {
+                    format!("Unable to parse derivation, provided: {change}, err: {e}")
+                })
+            } else {
+                Ok(0)
+            }
+        })?;
+    Ok(arg.to_string())


There's a lot of shared logic between the new parsers and the deprecated validators. Is there any value to deduping?

Yeah, the parsers that return a String type basically run the original validator and then return the argument as string. It would be nice to just invoke the validator directly and return the argument, but this will trigger a deprecation warning...

I was thinking about moving the shared logic to a helper function that's called from the validator and the parser. But it may not be worth the effort.

clap-v3-utils/src/input_validators.rs

clap-v3-utils/src/input_parsers/mod.rs

CriesofCarrots · 2023-09-28T02:22:34Z

This lgtm now! Give me a nudge when CI is green and I'll approve for merge.

Co-authored-by: Trent Nelson <[email protected]>

CriesofCarrots

samkim-crypto added the work in progress This isn't quite right yet label Sep 16, 2023

samkim-crypto removed the work in progress This isn't quite right yet label Sep 17, 2023

samkim-crypto requested review from t-nelson and CriesofCarrots September 18, 2023 16:47

t-nelson reviewed Sep 20, 2023

View reviewed changes

samkim-crypto requested a review from t-nelson September 20, 2023 19:34

t-nelson previously approved these changes Sep 26, 2023

View reviewed changes

CriesofCarrots reviewed Sep 27, 2023

View reviewed changes

samkim-crypto dismissed t-nelson’s stale review via a865509 September 28, 2023 01:20

samkim-crypto and others added 18 commits September 27, 2023 20:44

add tests for validating Pubkey and Hash

f7128b9

add pubkey signature parser

4ce5270

add parsers for straightforward validators

bd415a4

add parser for token amounts

537e635

add parser for derivation and seeds

7222f22

resolve warnings from deprecations in clap-v3-utils

69a5c62

remove some deprecated functions from solana_keygen

0577954

refactor signer related input parsers into a submodule

6dafe23

fix deprecation notice for utl

33ff50b

refactor parsers in input_validators to input_parsers

b8a0170

cargo fmt

8aa862b

Apply suggestions from code review

05fe037

Co-authored-by: Trent Nelson <[email protected]>

Update clap-v3-utils/src/input_parsers/mod.rs

716e43d

Co-authored-by: Trent Nelson <[email protected]>

mionr fixes to build

46929cd

add deprecation notice for old input_parsers::signer functions

9a5a144

update UiTokenAmount to Amount

60062a6

refactor to-be-deprecated functions back to `input_parsers/mod.rs

fc873e5

fmt

4e4efc9

samkim-crypto requested a review from CriesofCarrots September 28, 2023 04:41

CriesofCarrots approved these changes Sep 28, 2023

View reviewed changes

samkim-crypto merged commit ec36369 into solana-labs:master Sep 28, 2023
16 checks passed

This was referenced Oct 16, 2023

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

Closed

[clap-v3-utils] Deprecate signer source validation #33802

Merged

samkim-crypto mentioned this pull request Mar 27, 2024

clap-utils: Add more compute unit helpers anza-xyz/agave#440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clap-v3-utils] Deprecate input validators and add parsers to replace them #33276

[clap-v3-utils] Deprecate input validators and add parsers to replace them #33276

samkim-crypto commented Sep 16, 2023 •

edited

Loading

codecov bot commented Sep 17, 2023 •

edited

Loading

t-nelson left a comment

CriesofCarrots left a comment

CriesofCarrots Sep 27, 2023

samkim-crypto Sep 28, 2023

CriesofCarrots Sep 28, 2023

CriesofCarrots commented Sep 28, 2023

CriesofCarrots left a comment

[clap-v3-utils] Deprecate input validators and add parsers to replace them #33276

[clap-v3-utils] Deprecate input validators and add parsers to replace them #33276

Conversation

samkim-crypto commented Sep 16, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Sep 17, 2023 • edited Loading

Codecov Report

t-nelson left a comment

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

CriesofCarrots Sep 27, 2023

Choose a reason for hiding this comment

samkim-crypto Sep 28, 2023

Choose a reason for hiding this comment

CriesofCarrots Sep 28, 2023

Choose a reason for hiding this comment

CriesofCarrots commented Sep 28, 2023

CriesofCarrots left a comment

Choose a reason for hiding this comment

samkim-crypto commented Sep 16, 2023 •

edited

Loading

codecov bot commented Sep 17, 2023 •

edited

Loading