fix unsupported sign-mode issue #8728
Conversation
WalkthroughThe changes in this pull request modify the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
<details>
Files selected for processing (1)
- cmd/osmosisd/cmd/root.go (4 hunks)
Additional comments not posted (1)
cmd/osmosisd/cmd/root.go (1)
23-23: Imported packages are appropriate and necessaryThe added import statements for
github.com/cosmos/cosmos-sdk/x/auth/tx,github.com/cosmos/cosmos-sdk/types/tx/signing, andauthtxconfig "github.com/cosmos/cosmos-sdk/x/auth/tx/config"are essential for configuring transaction signing modes and handling SIGN_MODE_TEXTUAL.Also applies to: 65-65, 67-67
| if !initClientCtx.Offline { | ||
| txConfigOpts := tx.ConfigOptions{ | ||
| EnabledSignModes: append(tx.DefaultSignModes, signing.SignMode_SIGN_MODE_TEXTUAL), | ||
| TextualCoinMetadataQueryFn: authtxconfig.NewGRPCCoinMetadataQueryFn(initClientCtx), | ||
| } | ||
| txConfigWithTextual, err := tx.NewTxConfigWithOptions( | ||
| initClientCtx.Codec, | ||
| txConfigOpts, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| initClientCtx = initClientCtx.WithTxConfig(txConfigWithTextual) | ||
| } |
There was a problem hiding this comment.
Ensure consistent sign mode configuration across the application
You have correctly enabled SIGN_MODE_TEXTUAL in the NewRootCmd function when the client is online. However, in the autoCliOpts function (lines 1324-1327), EnabledSignModes is set to tx.DefaultSignModes without including SIGN_MODE_TEXTUAL. This inconsistency might lead to issues with transaction signing in the auto-generated CLI commands.
Apply this diff to include SIGN_MODE_TEXTUAL in autoCliOpts:
func autoCliOpts(initClientCtx client.Context, tempApp *osmosis.OsmosisApp) autocli.AppOptions {
...
TxConfigOpts: tx.ConfigOptions{
- EnabledSignModes: tx.DefaultSignModes,
+ EnabledSignModes: append(tx.DefaultSignModes, signing.SignMode_SIGN_MODE_TEXTUAL),
TextualCoinMetadataQueryFn: authtxconfig.NewGRPCCoinMetadataQueryFn(initClientCtx),
},
...
}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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !initClientCtx.Offline { | |
| txConfigOpts := tx.ConfigOptions{ | |
| EnabledSignModes: append(tx.DefaultSignModes, signing.SignMode_SIGN_MODE_TEXTUAL), | |
| TextualCoinMetadataQueryFn: authtxconfig.NewGRPCCoinMetadataQueryFn(initClientCtx), | |
| } | |
| txConfigWithTextual, err := tx.NewTxConfigWithOptions( | |
| initClientCtx.Codec, | |
| txConfigOpts, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| initClientCtx = initClientCtx.WithTxConfig(txConfigWithTextual) | |
| } | |
| func autoCliOpts(initClientCtx client.Context, tempApp *osmosis.OsmosisApp) autocli.AppOptions { | |
| ... | |
| TxConfigOpts: tx.ConfigOptions{ | |
| EnabledSignModes: append(tx.DefaultSignModes, signing.SignMode_SIGN_MODE_TEXTUAL), | |
| TextualCoinMetadataQueryFn: authtxconfig.NewGRPCCoinMetadataQueryFn(initClientCtx), | |
| }, | |
| ... | |
| } |
|
Thanks for both of your PRs, would you be able to add a changelog here as well? |
PaddyMc
added
V:state/compatible/backport
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
Discrepancy Found in CHANGELOG.md Updates
The CHANGELOG.md suggests that certain code segments have been removed or replaced in lines 52-58. However, the executed shell scripts reveal multiple references to
v18within the codebase, indicating that some changes may not have been fully implemented.Notable Findings:
- app/app.go
- Imports and upgrade references still include
v18.- .github/mergify.yml
- Contains backport configurations for
v18.xbranches.- Networks and Scripts
- Multiple files in
networks/osmosis-1/andscripts/release/directories referencev18.0.0.- CHANGELOG.md
- Entries under
## v18.0.0indicate ongoing references despite intended removals.These lingering references suggest that the removal or replacement of
v18may be incomplete, potentially leading to inconsistencies or unexpected behaviors in the system.Analysis chain
Line range hint
131-242: Major update with state-breaking changes and new features.This v19.0.0 release introduces significant changes:
New modules:
- ProtoRev module for MEV capture
- Validator Set Preference module
- Pool Manager module
State-breaking changes:
- Updates to GAMM API
- Changes to superfluid staking
- Modifications to tokenfactory and protorev modules
Major features:
- Taker fee implementation
- IBC composability improvements
- Cosmwasm upgrades
Configuration changes:
- Updates to app.toml and config.toml
These changes will require careful consideration during the upgrade process. Node operators and developers should pay close attention to the upgrade instructions and potential impacts on their systems.
To ensure a smooth upgrade process, it's recommended to run the following commands to check for any potential issues:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any deprecated API usage in custom modules grep -R "github.com/osmosis-labs/osmosis/v18" . # Verify that all necessary modules are imported in app.go grep "import" app/app.go # Check for any hardcoded version numbers that might need updating grep -R "v18" .Length of output: 5680
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments not posted (4)
CHANGELOG.md (4)
Line range hint
55-91: LGTM! Minor improvements and API refinements.This section introduces several improvements and minor API changes:
- Fixes for state export and CW pool gauges
- Performance improvements for IAVL
- New helper functions in osmoutils
- Refinements to Concentrated Liquidity queries and functions
- Internal API changes to improve code quality and performance
These changes appear to be beneficial for the project without introducing major breaking changes.
Line range hint
93-129: LGTM! Performance improvements and bug fixes.This section introduces several performance improvements and bug fixes:
- New helper functions in osmoutils for efficient calculations
- Performance improvements in Concentrated Liquidity math operations
- Bug fixes for CLI commands and error handling
- Improvements to TWAP calculations and pool creation fees
These changes should enhance the overall performance and reliability of the system.
Line range hint
244-2172: Well-structured changelog providing comprehensive project history.The CHANGELOG.md file offers a detailed history of the Osmosis project's development:
- Consistent structure: Each version is clearly marked with features, bug fixes, and improvements.
- Detailed descriptions: Changes are explained with references to pull requests and commits.
- Version coverage: The changelog covers multiple major and minor versions, allowing users to track changes over time.
- Upgrade instructions: For major versions, links to upgrade guides are provided.
- Breaking changes: Clearly marked sections for state-breaking changes and API breaks.
This changelog serves as an excellent resource for developers, node operators, and users to understand the project's evolution and prepare for upgrades.
Line range hint
1-2172: Excellent changelog following best practices.The CHANGELOG.md file is well-maintained and follows best practices:
- Clear version demarcation
- Consistent structure across versions
- Detailed descriptions of changes with PR references
- Proper categorization of changes (features, bug fixes, breaking changes)
- Upgrade instructions for major versions
- Comprehensive coverage of the project's history
This changelog serves as a valuable resource for all stakeholders in the Osmosis project, facilitating smooth upgrades and providing a clear picture of the project's evolution.
Hi @mattverse we can test it by executing unjail command with any address with patch |
|
Thank you for the PR ❤️ |
* fix unsupported sign-mode issue * update changelog --------- Co-authored-by: PaddyMc <[email protected]> (cherry picked from commit 502f559) # Conflicts: # CHANGELOG.md
* fix unsupported sign-mode issue (#8728) * fix unsupported sign-mode issue * update changelog --------- Co-authored-by: PaddyMc <[email protected]> (cherry picked from commit 502f559) # Conflicts: # CHANGELOG.md * Update CHANGELOG.md --------- Co-authored-by: Marri Harish <[email protected]> Co-authored-by: PaddyMc <[email protected]>
Closes: #8727
What is the purpose of the change
This PR fixes issue with signing unjail transaction with v26.0.1 binary
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)