fix(simapp): textual wiring #18242
fix(simapp): textual wiring #18242
Conversation
WalkthroughThis pull request introduces significant changes to improve code simplicity, maintainability, and performance. It includes updates to the client context, transaction configuration, and sign modes. It also refactors the code to remove unnecessary variables and functions, and corrects typos. The changes are spread across several files, including Changes
TipsChat with CodeRabbit Bot (
|
julienrbrt
added
the
backport/v0.50.x
|
@julienrbrt your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- client/cmd.go (3 hunks)
- client/tx/factory.go (1 hunks)
- client/v2/autocli/msg.go (1 hunks)
- simapp/simd/cmd/root_v2.go (5 hunks)
- x/auth/tx/config/config.go (2 hunks)
Files skipped from review due to trivial changes (2)
- client/tx/factory.go
- x/auth/tx/config/config.go
Additional comments: 9
client/v2/autocli/msg.go (2)
118-119: The client context is now updated with the command context and output. This is a good practice as it ensures that the client context is always in sync with the command context and output.
121-135: The creation of
txConfigand its assignment toclientCtxis now inside the condition that checks if the application is not offline and if the sign mode textual is not enabled. This change is logical as it prevents unnecessary operations when the application is offline or the sign mode textual is already enabled. However, ensure that this change does not affect other parts of the code that rely onclientCtxhaving atxConfigeven when the application is offline or the sign mode textual is already enabled.client/cmd.go (2)
11-11: The import
golang.org/x/exp/slicesis added to use theslices.Containsfunction. Ensure that this package is maintained and compatible with the current codebase.287-287: The loop checking for the presence of
SignMode_SIGN_MODE_TEXTUALinSupportedModes()has been replaced with a call toslices.Contains. This is a good optimization as it simplifies the code and potentially improves performance.simapp/simd/cmd/root_v2.go (5)
27-33: The import of
authtxconfighas been added and the variabletxConfigOptshas been removed from theNewRootCmdfunction. This change seems to be part of a larger refactoring to move the creation oftxConfigfromNewRootCmdtoProvideClientContext. Ensure that this change does not affect the functionality of theNewRootCmdfunction.47-52: The
txConfigOptsvariable has been removed from theNewRootCmdfunction. This change is part of the refactoring to move the creation oftxConfigfromNewRootCmdtoProvideClientContext. Ensure that this change does not affect the functionality of theNewRootCmdfunction.101-101: The
ProvideClientContextfunction now takestxConfigOptsas an argument instead oftxConfig. This change is part of the refactoring to move the creation oftxConfigfromNewRootCmdtoProvideClientContext. Ensure that all calls toProvideClientContexthave been updated to match the new signature.109-114: The
WithTxConfigmethod call has been removed from theclientCtxinitialization. This change is part of the refactoring to move the creation oftxConfigfromNewRootCmdtoProvideClientContext. Ensure that this change does not affect the functionality of theclientCtx.128-134: The
txConfigis now created in theProvideClientContextfunction usingtxConfigOptsand theNewTxConfigWithOptionsfunction. ThetxConfigis then set in theclientCtxusing theWithTxConfigmethod. This change is part of the refactoring to move the creation oftxConfigfromNewRootCmdtoProvideClientContext. Ensure that this change does not affect the functionality of theclientCtxandtxConfig.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- client/v2/go.mod
Files selected for processing (2)
- client/tx/tx.go (2 hunks)
- client/v2/autocli/msg.go (2 hunks)
Files skipped from review due to trivial changes (1)
- client/tx/tx.go
Additional comments: 3
client/v2/autocli/msg.go (3)
6-11: The import
"golang.org/x/exp/slices"has been removed. Ensure that this package is not used elsewhere in the code. If it is, you may need to replace its usage with an equivalent function or method.117-118: The client context is now updated with the command context and output. This is a good practice as it ensures that the client context is always in sync with the command context and output.
121-136: The check for
!clientCtx.Offlinehas been moved to encompass more code, including the creation oftxConfig. This change could potentially improve performance by avoiding unnecessary operations whenclientCtx.Offlineis true. However, ensure that this change does not affect the expected behavior of the code.- if !clientCtx.Offline && !slices.Contains(b.TxConfigOpts.EnabledSignModes, signing.SignMode_SIGN_MODE_TEXTUAL) { + if !clientCtx.Offline {Committable suggestion (Beta)
if !clientCtx.Offline { clientCtx = clientCtx.WithCmdContext(cmd.Context()) clientCtx = clientCtx.WithOutput(cmd.OutOrStdout()) b.TxConfigOpts.EnabledSignModes = append(b.TxConfigOpts.EnabledSignModes, signing.SignMode_SIGN_MODE_TEXTUAL) b.TxConfigOpts.TextualCoinMetadataQueryFn = authtxconfig.NewGRPCCoinMetadataQueryFn(clientCtx) txConfig, err := authtx.NewTxConfigWithOptions( codec.NewProtoCodec(clientCtx.InterfaceRegistry), b.TxConfigOpts, ) if err != nil { return err } clientCtx = clientCtx.WithTxConfig(txConfig) }
(cherry picked from commit fd93ee7) # Conflicts: # UPGRADING.md # client/tx/factory.go # client/tx/tx.go # client/v2/go.mod
Co-authored-by: Julien Robert <[email protected]>
| @@ -115,21 +114,26 @@ func (b *Builder) BuildMsgMethodCommand(descriptor protoreflect.MethodDescriptor | |||
| return err | |||
| } | |||
|
|
|||
| // enable sign mode textual and config tx options | |||
| if !clientCtx.Offline && !slices.Contains(b.TxConfigOpts.EnabledSignModes, signing.SignMode_SIGN_MODE_TEXTUAL) { | |||
There was a problem hiding this comment.
We removed this check here that !slices.Contains(b.TxConfigOpts.EnabledSignModes, signing.SignMode_SIGN_MODE_TEXTUAL), was that intentional?
There was a problem hiding this comment.
Yes, it was intentional.
In the client (autocli or root.go), we always need to overwrite the TxConfigOpts.TextualCoinMetadataQueryFn to use NewGRPCCoinMetadataQueryFn. If an app is using depinject, that value will using the bank keeper, which is only correct for the server (app.go)
Description
Closes: #17822
Verifies that textual works and --node flags works as well.
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
x/protocolpoolmodule in the UPGRADING.md file, helping developers to understand the changes and adapt their code accordingly.