fix(client/v2): resolve keyring flags properly #19060
Conversation
julienrbrt
added
the
backport/v0.50.x
WalkthroughThe changes across various files in the client module seem to focus on enhancing the command-line interface (CLI) by streamlining context management and error reporting. The modifications include the integration of context into CLI commands, simplification of flag parsing, and removal of redundant code. Additionally, the changes address a bug related to sending transactions via the CLI by improving how commands are set up and executed, ensuring that the context is consistently passed and utilized. Changes
Assessment against linked issues
The changes seem aligned with the objectives of improving CLI functionality and code quality, but without additional context on the CLI's argument processing, it is not entirely clear if the specific bug reported in issue #18122 is resolved. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
cc @luchenqun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -278,7 +279,7 @@ func readTxCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Context, err | |||
| from, _ := flagSet.GetString(flags.FlagFrom) | |||
| fromAddr, fromName, keyType, err := GetFromFields(clientCtx, clientCtx.Keyring, from) | |||
| if err != nil { | |||
| return clientCtx, err | |||
| return clientCtx, fmt.Errorf("failed to convert address field to address: %w", err) | |||
There was a problem hiding this comment.
The error message has been improved to provide more context, but the error wrapping with %w is only useful if the error is going to be unwrapped later. If not, %v should be used instead.
- return clientCtx, fmt.Errorf("failed to convert address field to address: %w", err)
+ return clientCtx, fmt.Errorf("failed to convert address field to address: %v", err)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return clientCtx, fmt.Errorf("failed to convert address field to address: %w", err) | |
| return clientCtx, fmt.Errorf("failed to convert address field to address: %v", err) |
|
Still needs changes from #18994 before the client config is being correctly used. I'll go ahead and open up #18994 for review. Let me know if it needs any changes. Steps to reproduce client config not being loaded on this PR:
simd --home $CONFIG tx --keyring-backend test slashing unjail --from `$TEST_KEY` --offline -a 1 -s 1Result: Despite chain ID being specified in This also applies to all values from the client config. Both this PR and #18994 together seem to fix the issue. |
|
Yea, definitely, your PR is still needed. This only fixes the keyring flags and pass the root context to AutoCLI commands. |
(cherry picked from commit 8f39bfb) # Conflicts: # client/v2/CHANGELOG.md # client/v2/autocli/builder.go # client/v2/autocli/common_test.go # client/v2/autocli/msg.go
) Co-authored-by: Julien Robert <[email protected]>
cosmos#19105) Co-authored-by: Julien Robert <[email protected]>
Solution: - update dependency to include the fix, see cosmos/cosmos-sdk#19060
Solution: - update dependency to include the fix, see cosmos/cosmos-sdk#19060 changelog fix test
Solution: - update dependency to include the fix, see cosmos/cosmos-sdk#19060 changelog fix test
Description
Closes: #18122
ref: #18994 (comment)
I dropped the refactoring away of the keyring as defined in the issue, as we want to be minimally breaking for v0.50 client/v2. This is a fix meant to be backported and then I'll do it the "better way".
I'll update Hubl in a follow-up.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...