Bug fix: Graceful handling of absent clap arguments

[grod220]

Problem

While working on CLI multsig support for solana-program/token-wrap, I noticed that signer_from_source_with_config was throwing during a parse step intended to return an option. The source of the issue was the internal use of:

matches.try_get_many::<String>(name)?

Which throws MatchesError::UnknownArgument if a match is not found. Given the helpers return an Option, I believe it is more in the spirit of these to return a None instead of raising an error and will fix the outer signer_from_source_with_config parse bug. This is the strategy employed in token-2022 as well.

Summary of Changes

• Modified the internal implementation of affected functions to use matches.try_get_many::<String>(name).ok().flatten().
• Added unit tests to explicitly verify graceful handling, confirming they return Ok(None) without erroring or panicking.

[alexpyattaev]

If at all possible, suggest migrating to clap4 rather than fixing clap3 stuff (unless there is no other way). Also the PR title is worng =)

[grod220]

suggest migrating to clap4

Can you point to that crate where that work is? Didn't realize there was an updated version out.

Also the PR title is worng =)

Is it because it includes the word "Bug Fix"? That is there because signer_from_source_with_config() is broken for the Pubkey enum branch.

[alexpyattaev]

Can you point to that crate where that work is? Didn't realize there was an updated version out.

Yes clap has been at v4 for a while and it provides substantially better ergonomics with derive macros, removes around 50% of the code needed to do parsing and is far more maintainable. Vortexor component uses v4 (it might be the first one that was merged). Switching to v4 is only really justified if you are using the derive macros though, basic API might not have improved much.

Is it because it includes the word "Bug Fix"?
No it literally says "Big fix:"

[grod220]

clap has been at v4 for a while

oh! Will go check out the new library and see if it has the same issue. Thanks for pointing that out.

No it literally says "Big fix:"

Omg. Random misspellings like this happen to me a lot 😆

[grod220]

Talked offline. I was a bit confused on the feedback. The solana-program/token-wrap has a tight dependency on this crate for parsing signer sources in its CLI. So this bug fix is still relevant and there isn't another clap-v4-utils library out yet. Just the general library at that higher version.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.3%. Comparing base (f0de5c3) to head (53cc0f2).
Report is 43 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #5534    +/-   ##
========================================
  Coverage    83.3%    83.3%            
========================================
  Files         829      829            
  Lines      373645   373800   +155     
========================================
+ Hits       311549   311691   +142     
- Misses      62096    62109    +13     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[alexpyattaev]

Looks good, suggest you also add some "positive" test cases, as now you are only testing for graceful failure. Probably another PR would be apprpriate for that.

Another commit necessary. Found a number of other instances in the crate.

[grod220]

@samkim-crypto would be great to get your eyes on this as it looks like you authored a good number of these. This PR attempts to change the opinion on how absent CLI arguments should be handled. I am under the belief if the argument is not present and is required, that should be enforced by the application defining those commands. The utilities are more in line with the spirit of returning Option's for absent arguments. However, it requires using .ok().flatten() and .unwrap_or(false) to override Clap's default behavior of raising an MatchError:UnknownArgument.

Tried my best to add unit tests where possible. However, think we have a bit of technical debt with test coverage generally in this crate.

[samkim-crypto]

Yeah this was a source of a lot of headaches for myself too and we had the same discussion about this last year as well (solana-labs#34801 and more precisely solana-labs#34801 (comment)). Can you point me to a downstream location (token-wrap-cli?) where this early termination with MatchesError::UnknownArgument actually becomes a problem?

Ideally, we want to stick with the clap-v3 style where the downstream apps explicitly handle the MatchesError::UnknownArgument. I know this is not always simple or even possible. In this case, maybe just reverting solana-labs#34801 (comment) is simpler.

[samkim-crypto]

In terms of upgrading to clap-v4, we are planning to skip it and make an upgrade to clap-v5 once it comes out. This is because upgrade from clap-v2 to clap-v3 was a lot of headache.

The reason why this part of the solana-clap-v3-utils parsing logic is complicated is that most existing Solana CLI tools were (and many still are) originally written with clap-v2. With v2, the standard way to parse arguments is to use value_of(...), which gives None if the argument is unknown or not present and Some(...).

With clap-v3, the value_of(...) was deprecated in favor of try_get_one::<TYPE>(...), which gives MatchesError::UnknownArgument on an unknown argument, Ok(None) if argument is not present, and Ok(Some(...)) on success.

This change made it really difficult to upgrade the CLI tools that use clap-v2 to clap-v3. For something like token-cli, the arguments and logic are intertwined in very complicated/subtle ways, so it was difficult to properly handle for MatchesError::UnknownArgument without major rewriting of the logic, so we ended up replacing value_of with try_get_one::<TYPE>(...).ok().flatten() in a lot of places.

[alexpyattaev]

In terms of upgrading to clap-v4, we are planning to skip it and make an upgrade to clap-v5 once it comes out. This is because upgrade from clap-v2 to clap-v3 was a lot of headache.

Clap v5 timeline is probably "very far away":
https://github.com/clap-rs/clap/milestone/81
is it 100% a good idea to wait? Also it does not appear like they are planning a ton of breaking features.

[samkim-crypto]

In terms of upgrading to clap-v4, we are planning to skip it and make an upgrade to clap-v5 once it comes out. This is because upgrade from clap-v2 to clap-v3 was a lot of headache.

Clap v5 timeline is probably "very far away": https://github.com/clap-rs/clap/milestone/81 is it 100% a good idea to wait? Also it does not appear like they are planning a ton of breaking features.

Yeah I'd be very happy to see all the Solana cli tools using clap v4. If one is willing to take charge, then I'd be willing to help out as well.

I do want to note that the old interface from clap-v2 are just (opt-in) deprecated in v3, while they are entirely removed in v4. This makes it much easier to upgrade clap-v2 -> clap-v3 -> clap-v4 because we can break up PRs into smaller chunks (e.g. solana-labs/solana-program-library#6376, solana-labs/solana-program-library#7447, solana-labs/solana-program-library#7448).

Many of the existing solana cli tools still rely on clap-v2 including the main cli, so it might make sense to upgrade these to clap-v3 first before we create another clap-v4 utils and have different cli tools using 3 different versions of clap.

All in all, we should probably create a separate github issue for clap version related discussions and focus on just the changes in this PR here.

[alexpyattaev]

Yeah I'd be very happy to see all the Solana cli tools using clap v4. If one is willing to take charge, then I'd be willing to help out as well.

Get in touch with @yihau he is already doing some ground work for this in the validator main.

[alexpyattaev]

I would suggest we add also positive test cases as well. At the very least they would demonstrate how the things are supposed to work. Could be even within the same functions to avoid boilerplate.

[grod220]

Can you point me to a downstream location (token-wrap-cli?) where this early termination with MatchesError::UnknownArgument actually becomes a problem?

The primary motivation for this PR was that these utils are enforcing the presence of fields that should be considered optional. This behavior has caused consumers to completely side-step using them and create optional versions of the same thing (see token-2022 vs clap-v3-utils).

The signer_from_source_with_config function is extremely useful. However, in the Pubkey branch it enforces the presence of both --signer and then --sign-only before returning a NullSigner. I'd consider this a bug as it doesn't make sense to pass a separate --signer argument when you are trying to get a NullSigner of something else.

Ideally, we want to stick with the clap-v3 style where the downstream apps explicitly handle the MatchesError::UnknownArgument

Fixing the bug above would be sufficient in unblocking my work. However, in general, it feels like an API requiring optional flags to be present or it raises an error is a strictness mismatch. I think querying for the value of an optional flag and it not being present should default to None or False depending on the type.

If folks disagree with that or feel it's a separate issue to discuss, I can revert that in this PR and focus just on the bug.

[samkim-crypto]

Yeah if it is okay, I would vote for scoping the changes to the bug fix in the Pubkey branch for now to unblock. The general interface change would probably need further discussion and also involve @t-nelson (btw Trent, do you have opinion on this?).

The primary motivation for this PR was that these utils are enforcing the presence of fields that should be considered optional. This behavior has caused consumers to completely side-step using them and create optional versions of the same thing (see token-2022 vs clap-v3-utils).

The way things are handled in the token-cli is due to the fact that it was originally written with clap-v2 and value_of, which returned None on unknown arguments. When the token-cli was upgraded to use clap-v3, some of the value_of was just lazily replaced by try_get().ok().flatten() (btw, for the token22, this hack was added to cope with this "bug"...). It doesn't necessarily mean that newer cli tools should consume these functions as done in token-cli by not handling the error.

However, in general, it feels like an API requiring optional flags to be present or it raises an error is a strictness mismatch. I think querying for the value of an optional flag and it not being present should default to None or False depending on the type.

This is a totally fair view and I agree as well. What I am afraid of is that the clap-v3 parsing functions return Result<Option<T>, E> because it precisely makes a distinction between an optional flag and an unknown flag. The clap-v3 view is that all optional flags should be listed as part of the App. Parsing for an known flag is technically a logic error by the programmer. The solana-clap-v3-utils convenience functions adopts the Result<Option<T>, E> interface, so not erroring on Result<Option<T>, E> makes the outer Result pointless and also could be misleading to someone who is used to the clap-v3 style of parsing.

[grod220]

it precisely makes a distinction between an optional flag and an unknown flag

Ah yes, thank you for making that clarification. The issue is specifically that the flag is not defined in ArgMatches, not passed. That said, it's namely subcommand incompatibility that makes this an issue. For instance, in the token-wrap CLI, I define --fee-payer at the top level, but --signer at the subcommand level. This feels like a natural API as --signer should not be a generic global flag for every command. However, when I want to use signer_from_source_with_config to parse a NullSigner the function requires both of those flags to be defined within the same level within ArgMatches or an UnknownArgument err is raised. To support this, the function would need to prevent that error from propagating and default to None/False in that conditional.

I think that instance kinda makes the argument that a strict global schema doesn't work in all cases as consumers will define subcommands in ways that is hard to anticipate.

Yeah if it is okay, I would vote for scoping the changes to the bug fix in the Pubkey branch for now to unblock.

Totally, makes sense. Will revert specifically to the changes to signer_from_source_with_config to address the above case.

[samkim-crypto]

Yes, I think this is a completely reasonable change. I am glad that we don't need to do stuff like this after this!

I think that instance kinda makes the argument that a strict global schema doesn't work in all cases as consumers will define subcommands in ways that is hard to anticipate.

I definitely agree. I think generally, clap is nice when we have a standalone cli application, but when we have an upstream cli-specific utility crate that has multiple downstream consumers as we have here, it is often hard to work with. I'd be happy to discuss more about what an ideal API should be for these parsing functions, but let's get the token-wrap CLI unblocked first.

[grod220]

Great! Going to add tests for the rest of the conditions in the Pubkey branch of that function and merge. EDIT: Requires a re-review.