fix(simapp): home flag is not respected

[PoisonPhang]

Description

In attempting to get AutoCLI to respect the home flag, I found several opportunities to mitigate bugs or resolve potential issues. I've collected those changes here.

• client/cmd.go & server/util.go
  • Use cmd.SetContext(context.WithValue()) to update context of cmd
• client/config/config.go
  • Use WithKeyringDir(ctx.KeyringDir) instead of forcing it to be ctx.HomeDir
• client/config/toml.go
  • Use new viper instance to avoid reading tempDir() again

---

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...

• included the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• reviewed "Files changed" and left comments if necessary
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• updated the relevant documentation or specification, including comments for documenting Go code
• confirmed all CI checks have passed

Reviewers 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...

• confirmed the correct type prefix in the PR title
• confirmed all author checklist items have been addressed
• reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

[PoisonPhang]

After more testing, this does not fix the keyring issue when signing transactions, however - it does enable them to be created with a key from --home

[julienrbrt]

After more testing, this does not fix the keyring issue when signing transactions, however - it does enable them to be created with a key from --home

Yes, that was to be expected, as described here: #18868 (comment), this is because of the way to initiate the flag builder.

[PoisonPhang]

During the signing process in client/tx/tx.go:Sign, the key ring is listing the correct keys. This leads me to believe that it should be able to create a valid signature. Could you help me pinpoint where the incorrect keyring is being supplied/used? @julienrbrt

For context, I'm currently seeing errors in x/auth/ante/sigverify.go:AnteHandle after signing.

[julienrbrt]

Yes, the problem isn't there, signing is fine.
The issue is about resolving the key name to an address in the command argument. This happens here: https://github.com/cosmos/cosmos-sdk/blob/main/client/v2/autocli/flag/address.go
Basically the instantiated keyring (form root.go), has no way to change its directory (or just any keyring argument). And we want to keep the client/v2 keyring interface.
That's basically this issue: #18122

[PoisonPhang]

Given the scope of changes required to make AutoCLI functional, I'll first switch back to manual CLI for tx commands in our private cosmos-sdk fork.
I might suggest doing this for v0.50.X as well.

Initializing the AutoCLI keyring only after flags have been parsed doesn't seem possible without significant redesign.

After I update our private fork, I'll adjust this PR to only include genuine fixes that will be needed regardless of how the project moves forward with AutoCLI.

[julienrbrt]

Given the scope of changes required to make AutoCLI functional, I'll first switch back to manual CLI for tx commands in our private cosmos-sdk fork. I might suggest doing this for v0.50.X as well.

AutoCLI isn't used for tx commands in v0.50 (only main, see #18122). We'll have for sure a fix before v0.51 anyway.
The easy fix is killing key name resolution in flag handler, accept anything and do the resolution later on when we have the context. That does not require much of a redesign, and what I have been doing for solving
#18122, just hasn't big the biggest priority to get it to the finish line. Give me 1 week to reprioritize it.

[PoisonPhang]

I believe three commands use AutoCLI in v0.50.2.
@julienrbrt

• slashing
  • unjail
• crisis
  • invariant-broken
• distribution
  • withdraw-validator-commission

Using any of those commands with a --home flag other than the default home results in the accounts not being found.

We specify using 0.50.X in #18868

[julienrbrt]

Ah okay, forgot about those, then that changes everything. I'll have a look Monday 👍

[coderabbitai]

Walkthrough

The recent updates primarily focus on refining context management across both client and server components, enhancing configuration handling, and minor script adjustments for better environment compatibility. The modifications streamline context setting procedures and configuration management by employing more standardized methods and removing redundant parameters. These changes aim to improve code maintainability, configuration flexibility, and the overall structure of the application's context handling and configuration setup.

Changes

File(s)	Change Summary
client/cmd.go, server/util.go	Modified to use cmd.SetContext for setting context values, streamlining context management. Also, added necessary imports.
client/config/config.go	Updated to use KeyringDir for context object update, enhancing keyring management logic.
client/config/toml.go	Removed the v *viper.Viper parameter, now creates a viper.Viper instance internally, changing how the instance is obtained and used.
simapp/simd/cmd/root_v2.go	Added a call to WithViper on clientCtx, introducing new behavior related to Viper configuration.
scripts/init-simapp.sh	Changed shebang for better environment compatibility and added a newline at the end.
server/util_test.go	Added server.GetServerContextFromCmd(cmd) in test functions for setting up the server context, aimed at enhancing test setup.

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Note: Auto-reply has been disabled for this repository by the repository owner. The CodeRabbit bot will not respond to your comments unless it is explicitly tagged.

• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[julienrbrt]

utACK.

[PoisonPhang]

Hey @julienrbrt, I was looking at some of the failing tests and was hoping you could provide some insight.

In server/util_test.go, instead of checking the server context stored in the the command context - we're checking the value of the serverCtx to see if the value was set.

My changes to the SetCmdServerContext function ensure that the context of the command is always set, however - it doesn't make any assurances that the ref the object we wanted set is updated.

Should this ref also be set (if so, why?), or should theses tests be updated to ensure the context of the command is what is being updated.

[PoisonPhang]

@julienrbrt bumping this for feedback

[julienrbrt]

Should this ref also be set (if so, why?), or should theses tests be updated to ensure the context of the command is what is being updated.

I actually don't know the historical reason for having the reference updated. Maybe @alexanderbez knows.
Imho, your PR looks good as it. We should, however, document the slight behavior change of SetCmdServerContext in a changelog.

[PoisonPhang]

@julienrbrt change log updated. Thanks for the feedback.

[alexanderbez]

LGTM!