[clap-v3-utils] Remove deprecated functions #34989

samkim-crypto · 2024-01-28T08:21:43Z

Problem

Solana clap-v3-utils still uses deprecated functions from clap-v2.

Summary of Changes

Replaced a number of deprecated functions like possible_values, value_of, and multiple_occurrences with the updated functions from clap-v3.
Deprecated some existing parser functions that can be replaced by existing clap-v3 functions
Deprecated some panicking functions like keypair_of, keypairs_of, etc. in favor of their try_ variants

Clap-v3 has a feature deprecated that, when included, gives warnings on deprecated functions. With this PR, all the deprecated functions should either be removed or explicitly allowed (on deprecated functions). However, if I include the deprecated feature, it seems to turn this feature on in the rest of the repo on CI checks, so I left it out for now.

Fixes #

codecov · 2024-01-29T02:13:54Z

Codecov Report

Attention: Patch coverage is 57.95455% with 37 lines in your changes are missing coverage. Please review.

Project coverage is 81.8%. Comparing base (7399178) to head (838fd65).
Report is 11 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #34989    +/-   ##
========================================
  Coverage    81.7%    81.8%            
========================================
  Files         836      836            
  Lines      224834   225069   +235     
========================================
+ Hits       183847   184136   +289     
+ Misses      40987    40933    -54

CriesofCarrots

Generally looks fine to me. One nit, plus a request for deduplication.

CriesofCarrots · 2024-01-30T19:49:37Z

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

+        if value == ASK_KEYWORD {
+            let skip_validation = matches.try_contains_id(SKIP_SEED_PHRASE_VALIDATION_ARG.name)?;
+            keypair_from_seed_phrase(name, skip_validation, true, None, true).map(Some)
+        } else {
+            read_keypair_file(value).map(Some)
+        }


It would be nice to de-deupe this with the logic in try_keypairs_of -- I know the duplication predates you.

Yes good point. If try_keypairs_of returns a keypair vector of length greater than 1, it is actually unclear what the behavior of this function should be. Whether multiple keypairs are allowed should be caught by Args, so technically this choice will not matter if the function is used correctly... I pushed a change that simply pops the keypair out of the returned vector. Please take a look.

Missing a push? I don't see any changes.

Oh, sorry I don't quite know where those commit went... But I pushed the changes!

Ohhh, I was confused about your comment until reading the code just now. By deduping, I actually meant put the following code in a helper fn that gets called from both methods:

if value == ASK_KEYWORD { let skip_validation = matches.try_contains_id(SKIP_SEED_PHRASE_VALIDATION_ARG.name)?; keypair_from_seed_phrase(name, skip_validation, true, None, true).map(Some) } else { read_keypair_file(value).map(Some) }

(And elsewhere, if we use it other places)

Probably what you have written is fine, but I don't know if there might be costs to calling try_get_many when we only expect one?

Oh oops! Sorry for the misinterpretation. I added a helper function extract_keypair to include the keypair extraction logic.

I think technically, the helper function should be named keypair_from_path, but we already have a public function with that name. The try_keypair_of and try_keypairs_of should probably use the existing keypair_from_path (unless we strictly want to restrict the source to just ASK or file), but that will be changing the functionality so we probably don't want to change it now.

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

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

t-nelson · 2024-02-02T00:50:03Z

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

+    Ok(matches.try_get_many::<String>(name)?.map(|values| {
+        values
+            .map(|value| {
+                value.parse::<Pubkey>().unwrap_or_else(|_| {
+                    read_keypair_file(value)
+                        .expect("read_keypair_file failed")


i think this can be refactored to avoid the expect()

Error handling was a bit tricky to do inside maps and closures, so I ended up using a for-loop to propagate the error. If you see a more elegant solution, let me know!

t-nelson · 2024-02-02T00:51:41Z

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

+    Ok(matches.try_get_many::<String>(name)?.map(|values| {
+        values
+            .map(|pubkey_signer_string| {
+                let mut signer = pubkey_signer_string.split('=');


prefer splitn() to avoid succeeding on invalid inputs

Yep, valid point. I ended up using split_once, which also returns an option.

t-nelson · 2024-02-02T00:53:34Z

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

+                let key = Pubkey::from_str(signer.next().unwrap()).unwrap();
+                let sig = Signature::from_str(signer.next().unwrap()).unwrap();


we should avoid unwrap()ing on user inputs

Again, I ended up using a for-loop here, so let me know if you see a more elegant solution.

…ctions

…functions

…, `try_pubkey_of`, and `try_pubkeys_of`

…airs_of`

samkim-crypto · 2024-03-01T22:16:16Z

@CriesofCarrots @t-nelson, it would be great to get this PR merged before the migration 🙏 . Can you take one last look at the PR to check if the changes look sound?

willhickey · 2024-03-03T04:56:05Z

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

samkim-crypto added the work in progress This isn't quite right yet label Jan 28, 2024

samkim-crypto removed the work in progress This isn't quite right yet label Jan 30, 2024

samkim-crypto requested review from t-nelson and CriesofCarrots January 30, 2024 06:44

samkim-crypto mentioned this pull request Jan 30, 2024

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

Closed

CriesofCarrots reviewed Jan 30, 2024

View reviewed changes

samkim-crypto requested a review from CriesofCarrots January 31, 2024 10:44

t-nelson reviewed Feb 2, 2024

View reviewed changes

samkim-crypto requested a review from t-nelson February 5, 2024 01:11

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 19, 2024

samkim-crypto removed the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 20, 2024

samkim-crypto added 15 commits March 2, 2024 07:14

add deprecated feature to produce warnings on use of deprecated fun…

a46c4ca

…ctions

replace multiple_occurrences with arg actions

bb4d4a1

replace possible_values with PossibleValueParser

2aecd8c

deprecate value_of and values_of

0e62014

deprecate unix_timestamp_from_rfc3339_datetime

455bb7f

deprecate cluster_type_of

98e20f0

deprecate commitment_of

e835b10

deprecate keypair_of, keypairs_of, pubkey_of, and pubkeys_of …

ce71544

…functions

replace deprecated functions from try_keypair_of, try_keypairs_of…

cd397fb

…, `try_pubkey_of`, and `try_pubkeys_of`

deprecate pubkeys_sigs_of

be6a749

allow deprecated on tests

4edef84

remove deprecation feature from clap-v3-utils

c404967

re-export pubkeys_sigs_of

37b1328

add helper extract_keypair to dedupe try_keypair_of and `try_keyp…

775333f

…airs_of`

remove unwraps and expects

838fd65

willhickey closed this Mar 3, 2024

samkim-crypto mentioned this pull request Mar 20, 2024

[clap-v3-utils] Remove deprecated functions anza-xyz/agave#313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clap-v3-utils] Remove deprecated functions #34989

[clap-v3-utils] Remove deprecated functions #34989

samkim-crypto commented Jan 28, 2024 •

edited

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading

CriesofCarrots left a comment

CriesofCarrots Jan 30, 2024

samkim-crypto Jan 31, 2024

CriesofCarrots Jan 31, 2024

samkim-crypto Feb 1, 2024

CriesofCarrots Feb 1, 2024 •

edited

Loading

samkim-crypto Feb 2, 2024

t-nelson Feb 2, 2024

samkim-crypto Feb 2, 2024

t-nelson Feb 2, 2024

samkim-crypto Feb 2, 2024

t-nelson Feb 2, 2024

samkim-crypto Feb 2, 2024

samkim-crypto commented Mar 1, 2024

willhickey commented Mar 3, 2024

		let key = Pubkey::from_str(signer.next().unwrap()).unwrap();
		let sig = Signature::from_str(signer.next().unwrap()).unwrap();

[clap-v3-utils] Remove deprecated functions #34989

[clap-v3-utils] Remove deprecated functions #34989

Conversation

samkim-crypto commented Jan 28, 2024 • edited Loading

Problem

Summary of Changes

codecov bot commented Jan 29, 2024 • edited Loading

Codecov Report

CriesofCarrots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CriesofCarrots Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samkim-crypto commented Mar 1, 2024

willhickey commented Mar 3, 2024

samkim-crypto commented Jan 28, 2024 •

edited

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading

CriesofCarrots Feb 1, 2024 •

edited

Loading