fix(textual): only enable when online and added upgrading docs #18166
Conversation
There was a problem hiding this comment.
Could you add this in AutoCLI: https://github.com/cosmos/cosmos-sdk/blob/3690d9f/client/v2/autocli/msg.go#L117-L128 ?
UPGRADING.md
Outdated
| } | ||
| ``` | ||
|
|
||
| When using `depinject` / `app v2`, it's enabled by default, and only needs to be added in the application client (`root.go`). |
There was a problem hiding this comment.
That is not true, unfortunately we still need to enable it for app_v2.
We can probably abstract it there with a provider in runtime.
There was a problem hiding this comment.
Isn't this doing that?
cosmos-sdk/x/auth/tx/config/config.go
Lines 83 to 87 in 3a8f114
There was a problem hiding this comment.
Wow, I didn't know. It can be removed from root_v2 then.
In client/v2 let's check if set before re-setting it then (so it supports app v2 apps and legacy wiring)
julienrbrt
added
the
backport/v0.50.x
There was a problem hiding this comment.
we should add some docs in a signing section to let teams and users know about this, the current docs are limited
https://docs.cosmos.network/main/learn/advanced/transactions#sign_mode_textual
Co-authored-by: samricotta <37125168+samricotta@users.noreply.github.com>
WalkthroughThis update introduces a new sign mode, Changes
TipsChat with CodeRabbit Bot (
|
![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
Files selected for processing (4)
- UPGRADING.md (1 hunks)
- simapp/app.go (5 hunks)
- simapp/simd/cmd/root.go (2 hunks)
- simapp/simd/cmd/root_v2.go (1 hunks)
Additional comments (Suppressed): 15
simapp/simd/cmd/root_v2.go (3)
81-95: The new sign mode
SIGN_MODE_TEXTUALis now only enabled when the client is online. This is a change from the previous behavior where it was always enabled. Ensure that this change does not affect any functionality that relies on this sign mode being available when the client is offline.81-95: The error handling for the creation of
txConfigWithTextualis done correctly. If an error occurs, it is returned immediately, preventing further execution of the function.97-99: The error handling for
SetCmdClientContextHandleris done correctly. If an error occurs, it is returned immediately, preventing further execution of the function.simapp/simd/cmd/root.go (4)
13-18: The import statement for
"github.com/cosmos/cosmos-sdk/codec"has been removed. Ensure that this does not affect any other parts of the code that might be using it.72-92: The new sign mode
SIGN_MODE_TEXTUALis now only enabled when the client is online. This is a change in behavior from the previous version whereSIGN_MODE_TEXTUALwas always enabled. Ensure that this change is intentional and that it does not break any existing functionality that relies onSIGN_MODE_TEXTUALbeing available even when the client is offline.74-92: The error handling for the
tx.NewTxConfigWithOptionsfunction has been improved. Previously, the error was not checked and could potentially cause a panic if the function failed. Now, the error is properly checked and returned if it occurs. This is a good improvement in terms of error handling.74-92: The codec used in the
tx.NewTxConfigWithOptionsfunction has been changed fromcodec.NewProtoCodec(encodingConfig.InterfaceRegistry)toinitClientCtx.Codec. Ensure that this change is intentional and thatinitClientCtx.Codecis equivalent tocodec.NewProtoCodec(encodingConfig.InterfaceRegistry).simapp/app.go (5)
61-61: The import of
sigtypesis new and is used to enable the new sign modeSIGN_MODE_TEXTUAL. This change is consistent with the PR summary.71-71: The import of
txmoduleis new and is used to configure theTextualCoinMetadataQueryFnfor the new sign mode. This change is consistent with the PR summary.151-151: The
BankKeepertype has been changed frombankkeeper.Keepertobankkeeper.BaseKeeper. Ensure that this change does not break any existing functionality that relies on methods not present inbankkeeper.BaseKeeper.292-305: The new sign mode
SIGN_MODE_TEXTUALis being enabled here by appending it to the default sign modes. A new transaction configuration is then created with the updated sign modes and options. TheTextualCoinMetadataQueryFnis configured using theNewBankKeeperCoinMetadataQueryFnfunction from thetxmodulepackage. The new transaction configuration is then set as thetxConfigfor theSimApp. This change is consistent with the PR summary.478-478: The error handling for
RegisterServiceshas been changed from a single line to multiple lines. This change does not affect the functionality of the code.UPGRADING.md (3)
35-42: The new sign mode,
SIGN_MODE_TEXTUAL, is introduced. It's important to note that this sign mode does not support offline signing. This could potentially impact users who rely on offline signing for their transactions. Ensure that this change is communicated to all relevant stakeholders.64-77: The code snippet shows how to enable the new sign mode in the
root.gofile when the client is not offline. The error returned bytx.NewTxConfigWithOptionsis handled correctly here, which is good practice.79-79: The new sign mode is enabled by default when using
depinject/app v2. This could potentially impact users who are not expecting this change. Ensure that this change is communicated to all relevant stakeholders.
UPGRADING.md
Outdated
| ```golang | ||
| enabledSignModes := append(tx.DefaultSignModes, sigtypes.SignMode_SIGN_MODE_TEXTUAL) | ||
| txConfigOpts := tx.ConfigOptions{ | ||
| EnabledSignModes: enabledSignModes, | ||
| TextualCoinMetadataQueryFn: txmodule.NewBankKeeperCoinMetadataQueryFn(app.BankKeeper), | ||
| } | ||
| txConfig, err := tx.NewTxConfigWithOptions( | ||
| appCodec, | ||
| txConfigOpts, | ||
| ) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| app.txConfig = txConfig | ||
| ``` |
There was a problem hiding this comment.
The code snippet shows how to enable the new sign mode in the app.go file. It's important to handle the error returned by tx.NewTxConfigWithOptions. The current code panics if there's an error. While this might be acceptable in some cases, it's generally better to handle errors gracefully. Consider logging the error and exiting the application with a non-zero status code.
- if err != nil {
- panic(err)
- }
+ if err != nil {
+ log.Fatalf("Failed to create new TxConfig with options: %v", err)
+ }
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- client/v2/autocli/msg.go (1 hunks)
Additional comments (Suppressed): 2
client/v2/autocli/msg.go (2)
118-121: The new sign mode
SIGN_MODE_TEXTUALis now only enabled when the client is online. This is a change from the previous behavior where it was always enabled. Ensure that this change does not affect any existing functionality that relies on the sign mode being enabled regardless of the client's online status.123-130: The creation of
txConfighas been moved inside the condition that checks if the client is online. This means thattxConfigwill not be created when the client is offline. If there are any parts of the code that rely ontxConfigbeing available even when the client is offline, this could cause issues. Please verify.
| @@ -32,6 +32,52 @@ clientCtx = clientCtx. | |||
|
|
|||
| Refer to SimApp `root_v2.go` and `root.go` for an example with an app v2 and a legacy app. | |||
|
|
|||
| #### Textual sign mode | |||
There was a problem hiding this comment.
Can we add this here #18166 (review) as well, as Marko said?
There was a problem hiding this comment.
added the link to the docs section and the ADR. I've also opened a PR on the docs repo
There was a problem hiding this comment.
the build and learn section is still tracked from the sdk, so the changes should be here.
the docs repo manages the building of the docs and probably later tutorials.
client/v2/autocli/msg.go
Outdated
| @@ -115,17 +115,19 @@ func (b *Builder) BuildMsgMethodCommand(descriptor protoreflect.MethodDescriptor | |||
| } | |||
|
|
|||
| // enable sign mode textual and config tx options | |||
| b.TxConfigOpts.EnabledSignModes = append(b.TxConfigOpts.EnabledSignModes, signing.SignMode_SIGN_MODE_TEXTUAL) | |||
| b.TxConfigOpts.TextualCoinMetadataQueryFn = authtxconfig.NewGRPCCoinMetadataQueryFn(clientCtx) | |||
| if !clientCtx.Offline { | |||
There was a problem hiding this comment.
Mmmh, that does make it re-add it and re-wire for depinject enabled app.
Imho, we want an extra check like if b.TxConfigOpts.EnabledSignModes not contains signing.SignMode_SIGN_MODE_TEXTUAL
There was a problem hiding this comment.
Replaced it with if !clientCtx.Offline && !slices.Contains(b.TxConfigOpts.EnabledSignModes, signing.SignMode_SIGN_MODE_TEXTUAL) {
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 (3)
- UPGRADING.md (1 hunks)
- client/v2/autocli/msg.go (2 hunks)
- docs/learn/advanced/01-transactions.md (1 hunks)
Files skipped from review due to trivial changes (1)
- UPGRADING.md
Additional comments (Suppressed): 4
docs/learn/advanced/01-transactions.md (1)
- 97-103: The updated documentation provides a more detailed explanation of the
SIGN_MODE_TEXTUALfeature, including its purpose, how it works, and how developers can customize it. It also mentions the addition of "expert" screens for more advanced users. This is a good improvement as it provides more context and understanding for the users of the SDK. However, ensure that the link to ADR-050 is working and leads to the correct resource.client/v2/autocli/msg.go (3)
9-9: The new import
"golang.org/x/exp/slices"is used for theslices.Containsfunction. Ensure that this package is maintained and reliable, as it's under the experimental section of the Go project.119-122: The new sign mode
SIGN_MODE_TEXTUALis now only enabled when the client is online and the sign mode is not already enabled. This is a change from the previous behavior where the sign mode was always enabled. Ensure that this change does not affect any existing functionality that relies on the sign mode being enabled.124-131: The creation of
txConfighas been moved inside the condition that checks if the client is online and the sign mode is not already enabled. This means thattxConfigwill not be created if the client is offline or the sign mode is already enabled. Ensure that this change does not affect any existing functionality that relies ontxConfigbeing created.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Co-authored-by: samricotta <37125168+samricotta@users.noreply.github.com> (cherry picked from commit 511db52) # Conflicts: # UPGRADING.md # client/v2/go.mod # simapp/simd/cmd/root.go
Description
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
SIGN_MODE_TEXTUAL, across the SDK for a more human-readable output, enhancing user experience especially on hardware wallets. This mode provides a string representation of transactions, formatted as screens for easy reading. Note: This mode does not support offline signing.UPGRADING.mdand01-transactions.mdfiles to provide instructions on enabling the new sign mode and to explain its benefits and limitations.