chore: Bring Mantra-sdk up to date with upstream #243
Conversation
… migrate` (backport cosmos#22057) (cosmos#22070) Co-authored-by: tutufen <[email protected]>
…mos#22090) (cosmos#22097) Co-authored-by: tutufen <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
…21782) (cosmos#22228) Co-authored-by: Luis Carvalho <[email protected]> Co-authored-by: Julien Robert <[email protected]>
…s#22231) Co-authored-by: Julien Robert <[email protected]>
WalkthroughThe pull request introduces several changes across multiple files. It adds two new jobs to the GitHub Actions workflow for testing, updates the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
mantrachain-support
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
docs/docs/build/build.md (2)
5-6: Consider adding a brief introduction.The title is clear, but adding a short description would help users understand the purpose and scope of this section better.
# Build + +This section provides comprehensive documentation for building applications using the Cosmos SDK framework. Here you'll find guides, references, and tools to help you develop your blockchain application.
7-13: Address formatting and consistency issues.The navigation links section has several minor inconsistencies:
- Link format inconsistency: mixing
.mdand non-.md URLs- Inconsistent punctuation: some descriptions end with periods, others don't
- Incorrect apostrophe usage in "ADR's"
Apply these improvements:
-* [Building Apps](./building-apps/00-app-go.md) - The documentation in this section will guide you through the process of developing your dApp using the Cosmos SDK framework. -* [Modules](./modules/README.md) - Information about the various modules available in the Cosmos SDK: Auth, Authz, Bank, Crisis, Distribution, Evidence, Feegrant, Governance, Mint, Params, Slashing, Staking, Upgrade, NFT, Consensus, Circuit, Genutil. -* [Migrations](./migrations/01-intro.md) - See what has been updated in each release the process of the transition between versions. -* [Packages](./packages/README.md) - Explore a curated collection of pre-built modules and functionalities, streamlining the development process. -* [Tooling](./tooling/README.md) - A suite of utilities designed to enhance the development workflow, optimizing the efficiency of Cosmos SDK-based projects. -* [ADR's](./architecture/README.md) - Provides a structured repository of key decisions made during the development process, which have been documented and offers rationale behind key decisions being made. -* [REST API](https://docs.cosmos.network/api) - A comprehensive reference for the application programming interfaces (APIs) provided by the SDK. +* [Building Apps](./building-apps/00-app-go.md) - The documentation in this section will guide you through the process of developing your dApp using the Cosmos SDK framework +* [Modules](./modules/) - Information about the various modules available in the Cosmos SDK: Auth, Authz, Bank, Crisis, Distribution, Evidence, Feegrant, Governance, Mint, Params, Slashing, Staking, Upgrade, NFT, Consensus, Circuit, Genutil +* [Migrations](./migrations/01-intro.md) - See what has been updated in each release and the process of transitioning between versions +* [Packages](./packages/) - Explore a curated collection of pre-built modules and functionalities, streamlining the development process +* [Tooling](./tooling/) - A suite of utilities designed to enhance the development workflow, optimizing the efficiency of Cosmos SDK-based projects +* [ADRs](./architecture/) - Provides a structured repository of key architectural decisions made during the development process +* [REST API](./api/) - A comprehensive reference for the application programming interfaces (APIs) provided by the SDKx/genutil/client/cli/migrate.go (1)
Line range hint
16-22: Consider documenting version compatibility matrix.Given that this is part of syncing with upstream changes, it would be valuable to maintain explicit documentation about version compatibility between Mantra-sdk and upstream Cosmos SDK versions in the MigrationMap. This helps users understand which migrations are supported when moving between different versions.
x/tx/signing/aminojson/json_marshal.go (2)
306-318: Enhance error message for oneof field processingWhile the error handling is proper, consider making the error message more descriptive by including the field name and oneof descriptor name.
- if err != nil { - return err - } + if err != nil { + return fmt.Errorf("failed to get oneof names for field %q in oneof %q: %w", + f.Name(), entry.oneof.Name(), err) + }
326-336: Optimize sorting logic performanceThe current implementation creates temporary variables in each comparison. Consider pre-computing the sort names to improve performance, especially for large message types.
if shouldSortFields := !enc.doNotSortFields; shouldSortFields { + // Pre-compute sort names + sortNames := make([]string, len(indices)) + for i, ni := range indices { + if ni.oneof != nil { + sortNames[i] = ni.oneofFieldName + } else { + sortNames[i] = ni.name + } + } sort.Slice(indices, func(i, j int) bool { - ni, nj := indices[i], indices[j] - niName, njName := ni.name, nj.name - - if indices[i].oneof != nil { - niName = indices[i].oneofFieldName - } - - if indices[j].oneof != nil { - njName = indices[j].oneofFieldName - } - - return niName < njName + return sortNames[i] < sortNames[j] }) }CHANGELOG.md (4)
Line range hint
1-24: Documentation structure needs improvementThe guiding principles section at the top is helpful, but could be better structured for readability.
Consider:
- Adding a table of contents
- Grouping the principles into categories (e.g., Format, Content, Process)
- Adding examples for each principle
<!-- Guiding Principles: +Table of Contents: +- Format Guidelines +- Content Requirements +- Process Rules + +Format Guidelines: Changelogs are for humans, not machines. There should be an entry for every single version. The same types of changes should be grouped. + +Content Requirements: Versions and sections should be linkable. The latest version comes first. The release date of each version is displayed. + +Process Rules: Mention whether you follow Semantic Versioning.
53-53: Inconsistent usage section formattingThe "Usage" section format differs from the guiding principles above it, and the entry format example is not properly highlighted.
Consider using consistent formatting and adding syntax highlighting:
Usage: -Change log entries are to be added to the Unreleased section under the -appropriate stanza (see below). Each entry is required to include a tag and -the Github issue reference in the following format: +Change log entries should be added to the "Unreleased" section under the +appropriate stanza (see below). Each entry must include: +1. A tag indicating the subsystem +2. The Github issue reference +3. A descriptive message + +Format: +``` +* (<tag>) \#<issue-number> message +```Also applies to: 14-20
Line range hint
32-45: Improve stanza types documentationThe types of changes (stanzas) section could be more informative with examples and clearer descriptions.
Consider adding:
- Examples for each type
- When to use each type
- Impact level indicators
Types of changes (Stanzas): -"Features" for new features. -"Improvements" for changes in existing functionality. -"Deprecated" for soon-to-be removed features. -"Bug Fixes" for any bug fixes. +"Features" - New functionality added to the system + Example: * (x/bank) #1234 Add new transfer method + +"Improvements" - Enhancements to existing features + Example: * (x/auth) #5678 Optimize signature verification + +"Deprecated" - Features planned for removal + Impact: High - Requires user migration + Example: * (x/staking) #9012 Deprecate legacy staking methods
Line range hint
53-1024: Enhance changelog entry organizationThe changelog entries could be better organized for improved readability and searchability.
Consider:
- Strictly ordering entries by PR number within each section
- Grouping related changes together under subsection headers
- Adding a migration impact level (High/Medium/Low) to breaking changes
- Ensuring consistent formatting of PR links and descriptions
This would make it easier for users to:
- Find all related changes for a specific module
- Understand the impact of breaking changes
- Plan migrations effectively
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)client/keys/migrate.go(0 hunks)docs/docs/build/_category_.json(1 hunks)docs/docs/build/build.md(1 hunks)simapp/app.go(1 hunks)x/genutil/client/cli/migrate.go(1 hunks)x/group/client/cli/tx.go(1 hunks)x/group/client/cli/util.go(1 hunks)x/tx/CHANGELOG.md(1 hunks)x/tx/signing/aminojson/json_marshal.go(4 hunks)
💤 Files with no reviewable changes (1)
- client/keys/migrate.go
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/test.yml
- docs/docs/build/category.json
🔇 Additional comments (8)
docs/docs/build/build.md (1)
1-3: LGTM! Frontmatter is correctly structured.
The sidebar position of 0 is appropriate for this main navigation page.
x/group/client/cli/util.go (1)
56-56: LGTM! Consider improving readability with a constant.
The change correctly implements the additional "1" option for execution flags, aligning with the upstream changes mentioned in PR objectives (backport cosmos#22229).
Consider defining a constant for the numeric value to improve code readability and maintainability:
const (
+ ExecTryNumeric = "1"
)
func execFromString(execStr string) group.Exec {
exec := group.Exec_EXEC_UNSPECIFIED
- if execStr == ExecTry || execStr == "1" {
+ if execStr == ExecTry || execStr == ExecTryNumeric {
exec = group.Exec_EXEC_TRY
}
return exec
}Let's verify that the documentation has been updated consistently:
✅ Verification successful
Let me gather more information about the CLI documentation and flag definitions.
Let me check the actual command implementation and help text for the exec flag.
Based on the gathered information, I can now generate the final response:
Documentation is consistent with the code change
The change to accept both "1" and "try" for the exec flag is properly documented:
- CHANGELOG.md documents the change: "Accept
1andtryin CLI for group proposal exec" - CLI help text for the flag shows both options:
- "Set to 1 or 'try' to try to execute proposal immediately after creation"
- "Set to 1 to try to execute proposal immediately after voting"
The original suggestion to add a constant is still valid for code maintainability, but the change is well-documented and consistent throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the CLI documentation reflects both "1" and "try" options
# Expected: Documentation should mention both options for the exec flag
# Search for exec flag documentation in CLI files
rg -i "exec.*try.*1|1.*try.*exec" --type md
rg -i "exec.*flag" --type go
Length of output: 6756
Script:
#!/bin/bash
# Check CLI flag definitions and help text
rg -A 5 "FlagExec.*=.*" x/group/client/cli/
rg -A 10 'AddTxFlagsToCmd|AddQueryFlagsToCmd' x/group/client/cli/
Length of output: 9572
Script:
#!/bin/bash
# Check the implementation of exec command and its flag usage
rg -A 10 'MsgExecCmd\(\).*{' x/group/client/cli/
rg -B 2 -A 5 'cmd.Flags\(\).String\(FlagExec' x/group/client/cli/
Length of output: 973
x/tx/CHANGELOG.md (1)
36-38: LGTM! The changelog entry is well-formatted and accurate.
The new entry correctly documents the bug fix for JSON attribute sort order on messages with oneof fields, following the changelog format guidelines:
- Appropriate categorization under "Bug Fixes"
- Includes correct PR reference (fix(x/tx): sort with oneof field name in amino-json cosmos/cosmos-sdk#21782)
- Clear, concise description
- Properly placed in the "Unreleased" section
x/genutil/client/cli/migrate.go (1)
39-39: LGTM! Example is accurate and well-structured.
The example correctly demonstrates:
- Required arguments in the correct order (target-version and genesis-file)
- Optional flags with realistic values
- Usage of a supported migration version (v0.47)
x/tx/signing/aminojson/json_marshal.go (2)
271-275: LGTM: Well-structured additions to nameAndIndex struct
The new fields properly support protobuf oneof field handling with clear, descriptive names.
373-373: LGTM: Proper Amino encoding structure for oneof fields
The JSON structure correctly follows Amino encoding conventions with type and value fields.
x/group/client/cli/tx.go (1)
654-654: Verify implementation of execFromString function.
The help text update clarifies that both "1" and "try" are valid inputs for the --exec flag. Let's verify that the execFromString function correctly handles these values.
✅ Verification successful
Help text accurately reflects implementation
The help text update is correct and consistent with the implementation. The execFromString function in x/group/client/cli/util.go explicitly handles both "1" and "try" values:
func execFromString(execStr string) group.Exec {
exec := group.Exec_EXEC_UNSPECIFIED
if execStr == ExecTry || execStr == "1" {
exec = group.Exec_EXEC_TRY
}
return exec
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that execFromString function supports both "1" and "try" values.
# Expected: Find the execFromString function implementation to confirm it handles both values.
# Test: Search for execFromString function implementation
ast-grep --pattern 'func execFromString($param) $ret {
$$$
}'
# Test: Search for any tests validating the execFromString function
rg -l "execFromString.*test"
Length of output: 96
Script:
#!/bin/bash
# Let's try a different approach to find the execFromString function
# First, let's search for the function name using ripgrep
rg -A 10 "execFromString"
# Also search for any switch/if conditions handling "1" or "try" values
rg -A 5 'case "1"|case "try"|"1"|"try"' x/group/client/cli/
Length of output: 9362
simapp/app.go (1)
212-214: Verify the signing context validation change across the codebase.
The change from txConfig.SigningContext() to interfaceRegistry.SigningContext() for signing context validation is logically sound, as it moves the validation closer to the source of truth (the interface registry). However, we should verify this change is consistent across the codebase to prevent any potential signing validation issues.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to get a complete picture.
Based on the comprehensive results, I can now generate the final response:
The signing context validation change is correctly implemented and consistent
The change from txConfig.SigningContext() to interfaceRegistry.SigningContext() for validation is verified to be correct because:
- The validation is consistently implemented in both
simapp/app.goandruntime/module.go - The remaining uses of
txConfig.SigningContext()inx/genutil/client/cli/commands.goare for different purposes (address codec access) and don't involve validation - The change aligns with the SDK's architecture where
InterfaceRegistryis the source of truth for signing context, as evidenced by the CHANGELOG entry
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usage of txConfig.SigningContext() that might need updating
# Test: Look for any remaining instances of txConfig.SigningContext()
rg "txConfig\.SigningContext\(\)"
# Test: Look for other signing context validations to ensure consistency
rg "SigningContext\(\)\.Validate\(\)"
Length of output: 845
Script:
#!/bin/bash
# Let's check the usage context of SigningContext() in runtime/module.go to ensure it's consistent
rg -B 5 -A 5 "SigningContext\(\)\.Validate\(\)" runtime/module.go
# Check the usage of txConfig.SigningContext() to understand if they are different use cases
rg -B 5 -A 5 "txConfig\.SigningContext\(\)" x/genutil/client/cli/commands.go
# Look for any other SigningContext-related code to ensure complete coverage
rg "SigningContext"
Length of output: 5858
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
docs/docs/build/build.md (2)
1-3: Consider enhancing the frontmatter metadata.While the current frontmatter is functional, consider adding more metadata fields to improve documentation organization and searchability:
--- sidebar_position: 0 +title: "Build" +description: "Guide to building applications using Cosmos SDK" +keywords: ["cosmos-sdk", "build", "development"] ---
7-7: Consider using semantic file naming.The link to
./building-apps/00-app-go.mduses a numbered prefix. Consider adopting semantic naming for better maintainability:-* [Building Apps](./building-apps/00-app-go.md) - The documentation in this section will guide you through the process of developing your dApp using the Cosmos SDK framework. +* [Building Apps](./building-apps/getting-started.md) - The documentation in this section will guide you through the process of developing your dApp using the Cosmos SDK framework.x/group/client/cli/util.go (1)
56-56: LGTM! Consider improving magic number usage.The addition of "1" as an alternative input value maintains backward compatibility while expanding supported inputs.
Consider defining a constant for the numeric value to improve code clarity and maintainability:
const ( + ExecTryNumeric = "1" // Alternative input value for EXEC_TRY ) func execFromString(execStr string) group.Exec { exec := group.Exec_EXEC_UNSPECIFIED - if execStr == ExecTry || execStr == "1" { + if execStr == ExecTry || execStr == ExecTryNumeric { exec = group.Exec_EXEC_TRY } return exec }x/tx/CHANGELOG.md (1)
36-39: Remove extra blank line for consistent formatting.To maintain consistent formatting with other sections, remove the extra blank line after "Bug Fixes".
### Bug Fixes - * [#21782](https://github.com/cosmos/cosmos-sdk/pull/21782) Fix JSON attribute sort order on messages with oneof fields.simapp/app.go (2)
Line range hint
227-246: Consider implementing custom mempool handlers.The commented example shows how to implement custom mempool and ABCI handlers. If these customizations are needed for your application, consider implementing them rather than leaving them as examples.
Would you like me to help implement custom mempool handlers tailored to your application's needs?
Line range hint
516-520: Remove empty gov hooks registration if not needed.The governance hooks registration is currently empty. If no governance hooks are needed, consider removing this empty registration.
-app.GovKeeper = *govKeeper.SetHooks( - govtypes.NewMultiGovHooks( - // register the governance hooks - ), -) +app.GovKeeper = *govKeeper
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)client/keys/migrate.go(0 hunks)docs/docs/build/_category_.json(1 hunks)docs/docs/build/build.md(1 hunks)simapp/app.go(1 hunks)x/genutil/client/cli/migrate.go(1 hunks)x/group/client/cli/tx.go(1 hunks)x/group/client/cli/util.go(1 hunks)x/tx/CHANGELOG.md(1 hunks)x/tx/signing/aminojson/json_marshal.go(4 hunks)
💤 Files with no reviewable changes (1)
- client/keys/migrate.go
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/test.yml
- docs/docs/build/category.json
🔇 Additional comments (12)
x/tx/CHANGELOG.md (2)
36-37: LGTM: Changelog entry structure follows guidelines.
The new entry is correctly placed under the "Unreleased" section with the appropriate "Bug Fixes" stanza.
38-38: LGTM: Clear and well-formatted changelog entry.
The entry follows the recommended format [#<issue-number>] Changelog message and clearly describes the bug fix related to JSON attribute sorting for oneof fields.
x/genutil/client/cli/migrate.go (1)
39-39: LGTM! Example field update improves command clarity.
The change from migrate to genesis migrate in the example provides better context and aligns with the standard Cosmos SDK command structure, making it clearer that this is a subcommand of the genesis command group.
simapp/app.go (1)
212-214: LGTM! Verify signing context validation change.
Moving the signing context validation from txConfig to interfaceRegistry improves the separation of concerns. The validation is correctly placed early in the application initialization.
Let's verify that this change doesn't affect any existing transaction signing implementations:
✅ Verification successful
Let's gather more specific information about the signing context validation usage to ensure this change is safe.
Let's check one more aspect to understand the interface registry's signing context implementation and its relationship with the previous txConfig approach.
Based on the gathered evidence, I can now provide a final response:
✅ Signing context validation change is safe and properly implemented
The change from txConfig to interfaceRegistry.SigningContext() validation is architecturally sound and consistently implemented:
- Only two locations use this validation pattern:
simapp/app.goandruntime/module.go - The
InterfaceRegistryinterface properly defines theSigningContext()method - Previous
txConfigvalidation was used for different purposes (genesis validation) and doesn't conflict with this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom signing context implementations that might be affected
rg -l "SigningContext|SignModeHandler" --type go
Length of output: 2530
Script:
#!/bin/bash
# Check the implementation and usage of SigningContext validation
ast-grep --pattern 'func ($x *$_) SigningContext() $_'
# Check for any direct SigningContext validation calls
rg 'SigningContext\(\).*Validate\(\)' -A 2 --type go
Length of output: 389
Script:
#!/bin/bash
# Check the InterfaceRegistry implementation and its SigningContext method
ast-grep --pattern 'type InterfaceRegistry interface {
$$$
}'
# Look for previous txConfig validation patterns
rg "txConfig.*Validate|Validate.*txConfig" --type go
Length of output: 4260
CHANGELOG.md (1)
53-53: LGTM! Well-formatted changelog entry.
The bug fix entry follows the proper changelog format with:
- Clear description of the change
- Proper PR reference (chore(x/group): update supported flags cosmos/cosmos-sdk#22229)
- Placement in the correct section
x/tx/signing/aminojson/json_marshal.go (7)
271-275: Addition of oneof-related fields to nameAndIndex struct
The inclusion of oneof, oneofFieldName, and oneofTypeName enhances the struct to handle oneof fields more effectively during JSON serialization.
306-311: Initialization of nameAndIndex with oneof information
Correctly populating the nameAndIndex instance with oneof descriptors and field names ensures accurate processing of oneof fields in subsequent operations.
312-319: Proper retrieval and error handling of oneof field names
The logic effectively retrieves oneofFieldName and oneofTypeName using getOneOfNames(f), with appropriate error handling to capture any issues during the process.
345-345: Accurate determination of oneof field status
The assignment of isOneOf correctly identifies if a field belongs to a oneof, which is essential for processing in subsequent logic.
352-355: Correct handling of unset oneof fields
The logic appropriately writes null for oneof fields that are unset, ensuring that the serialized JSON accurately represents the message state.
373-373: Ensuring proper JSON structure for set oneof fields
The formatting correctly wraps set oneof fields with "type" and "value" keys, adhering to the Amino JSON encoding conventions.
326-336: Verification of sorting logic for oneof fields
Ensure that the updated sorting mechanism accurately utilizes oneofFieldName for fields within oneof constructs to maintain consistent serialization order.
Run the following script to verify that fields are sorted correctly when they are part of a oneof:
✅ Verification successful
Let me verify the implementation further by checking the complete context around this sorting logic.
Let me try one more verification to understand the struct definition and oneof handling:
Based on the code snippets gathered, I can now provide a final response:
Sorting logic for oneof fields is correctly implemented
The implementation properly handles oneof fields by:
- Populating
oneofFieldNamethroughgetOneOfNamesduring field indexing - Using
oneofFieldNamein the sorting comparison when a field is part of a oneof - Maintaining consistent serialization by applying the same sorting logic across all fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the sorting logic correctly handles oneof fields.
# Test: Extract sorting function implementation and check ordering criteria.
grep -A 20 'sort\.Slice' x/tx/signing/aminojson/json_marshal.go
# Expected Result: The sorting comparison should use 'oneofFieldName' for oneof fields.
Length of output: 472
Script:
#!/bin/bash
# Get the complete context of the sorting implementation
rg -B 10 -A 30 'sort\.Slice.*indices' x/tx/signing/aminojson/json_marshal.go
# Check how oneofFieldName is populated
rg "oneofFieldName.*=" x/tx/signing/aminojson/json_marshal.go
# Look for struct definition to understand the fields
rg -B 5 "type.*struct.*{.*name.*}" x/tx/signing/aminojson/json_marshal.go
Length of output: 1484
Script:
#!/bin/bash
# Look for struct definition with fixed regex
rg -B 5 "type.*struct.*\{" x/tx/signing/aminojson/json_marshal.go
# Check the getOneOfNames function implementation
rg -A 10 "func getOneOfNames" x/tx/signing/aminojson/json_marshal.go
# Check test cases related to oneof field sorting
rg -l "TestSort.*Oneof" x/tx/signing/aminojson/
Length of output: 780
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
docs/docs/build/build.md (1)
7-13: Consider minor formatting improvements for consistency.The documentation is well-structured, but could benefit from minor enhancements:
-* [Building Apps](./building-apps/00-app-go.md) - The documentation in this section will guide you through the process of developing your dApp using the Cosmos SDK framework. -* [Modules](./modules/README.md) - Information about the various modules available in the Cosmos SDK: Auth, Authz, Bank, Crisis, Distribution, Evidence, Feegrant, Governance, Mint, Params, Slashing, Staking, Upgrade, NFT, Consensus, Circuit, Genutil. -* [Migrations](./migrations/01-intro.md) - See what has been updated in each release the process of the transition between versions. -* [Packages](./packages/README.md) - Explore a curated collection of pre-built modules and functionalities, streamlining the development process. -* [Tooling](./tooling/README.md) - A suite of utilities designed to enhance the development workflow, optimizing the efficiency of Cosmos SDK-based projects. -* [ADR's](./architecture/README.md) - Provides a structured repository of key decisions made during the development process, which have been documented and offers rationale behind key decisions being made. -* [REST API](https://docs.cosmos.network/api) - A comprehensive reference for the application programming interfaces (APIs) provided by the SDK. +* **[Building Apps](./building-apps/00-app-go.md)** - The documentation in this section will guide you through the process of developing your dApp using the Cosmos SDK framework. +* **[Modules](./modules/README.md)** - Information about the various modules available in the Cosmos SDK: Auth, Authz, Bank, Crisis, Distribution, Evidence, Feegrant, Governance, Mint, Params, Slashing, Staking, Upgrade, NFT, Consensus, Circuit, Genutil. +* **[Migrations](./migrations/01-intro.md)** - See what has been updated in each release and the process of transitioning between versions. +* **[Packages](./packages/README.md)** - Explore a curated collection of pre-built modules and functionalities, streamlining the development process. +* **[Tooling](./tooling/README.md)** - A suite of utilities designed to enhance the development workflow, optimizing the efficiency of Cosmos SDK-based projects. +* **[ADRs](./architecture/README.md)** - Provides a structured repository of key decisions made during the development process, documenting the rationale behind key decisions. +* **[REST API](https://docs.cosmos.network/api)** - A comprehensive reference for the application programming interfaces (APIs) provided by the SDK.Changes suggested:
- Bold the section titles for better visual hierarchy
- Fix typo: "ADR's" → "ADRs" (no apostrophe needed for plurals)
- Minor grammar improvements in descriptions
x/group/client/cli/util.go (1)
56-56: Consider using a constant for the numeric value.While the addition of "1" as an alternative to ExecTry improves CLI usability, consider defining it as a constant (e.g.,
ExecTryNumeric) for better maintainability and to avoid magic numbers.const ( ExecTry = "try" + ExecTryNumeric = "1" ) func execFromString(execStr string) group.Exec { exec := group.Exec_EXEC_UNSPECIFIED - if execStr == ExecTry || execStr == "1" { + if execStr == ExecTry || execStr == ExecTryNumeric { exec = group.Exec_EXEC_TRY } return exec }x/tx/CHANGELOG.md (1)
36-39: Consider maintaining consistent spacing between sections.The new entry has extra blank lines that differ from the spacing pattern used in other sections of the changelog.
Apply this diff to maintain consistent spacing:
### Bug Fixes - * [#21782](https://github.com/cosmos/cosmos-sdk/pull/21782) Fix JSON attribute sort order on messages with oneof fields. -x/group/client/cli/tx.go (1)
654-654: LGTM! Consider adding input validation.The updated help text clearly documents that both "1" and "try" are valid inputs for the
--execflag. This improves user guidance.Consider adding validation for the flag value to ensure only valid inputs ("1" or "try") are accepted. Here's a suggested implementation:
cmd.Flags().String(FlagExec, "", "Set to 1 or 'try' to try to execute proposal immediately after creation (proposers signatures are considered as Yes votes)") +execStr, _ := cmd.Flags().GetString(FlagExec) +if execStr != "" && execStr != "1" && execStr != "try" { + return fmt.Errorf("invalid value for --exec flag. Must be either '1' or 'try'") +}simapp/app.go (1)
212-214: LGTM! Consider adding a comment explaining the signing context validation.The change from
txConfig.SigningContext()tointerfaceRegistry.SigningContext()for validation is correct and aligns with upstream changes. However, since this is a critical initialization check, consider adding a comment explaining why this validation is necessary and what it ensures.Add a comment like:
+ // Validate the signing context to ensure proper transaction signing configuration if err := interfaceRegistry.SigningContext().Validate(); err != nil { panic(err) }CHANGELOG.md (3)
53-53: Fix bullet point indentation and PR link formatting.The bullet point is not properly aligned with other entries and the PR link is not properly formatted.
-* (x/group) [#22229](https://github.com/cosmos/cosmos-sdk/pull/22229) Accept `1` and `try` in CLI for group proposal exec. +* (x/group) [#22229](https://github.com/cosmos/cosmos-sdk/pull/22229) Accept `1` and `try` in CLI for group proposal exec.
Line range hint
15-24: Add missing periods for consistency.Several changelog entries are missing periods at the end of their descriptions for consistency with other entries.
-* (crypto/keyring) [#21653](https://github.com/cosmos/cosmos-sdk/pull/21653) New Linux-only backend that adds Linux kernel's `keyctl` support +* (crypto/keyring) [#21653](https://github.com/cosmos/cosmos-sdk/pull/21653) New Linux-only backend that adds Linux kernel's `keyctl` support. -* (server) [#21941](https://github.com/cosmos/cosmos-sdk/pull/21941) Regenerate addrbook.json for in place testnet +* (server) [#21941](https://github.com/cosmos/cosmos-sdk/pull/21941) Regenerate addrbook.json for in place testnet. -* (sims) [21906](https://github.com/cosmos/cosmos-sdk/pull/21906) Skip sims test when running dry on validators +* (sims) [21906](https://github.com/cosmos/cosmos-sdk/pull/21906) Skip sims test when running dry on validators. -* (cli) [#21919](https://github.com/cosmos/cosmos-sdk/pull/21919) Query address-by-acc-num by account_id instead of id +* (cli) [#21919](https://github.com/cosmos/cosmos-sdk/pull/21919) Query address-by-acc-num by account_id instead of id.
Line range hint
21-21: Fix PR link formatting.The PR link is missing the
#prefix in the link text.-* (sims) [21906](https://github.com/cosmos/cosmos-sdk/pull/21906) Skip sims test when running dry on validators +* (sims) [#21906](https://github.com/cosmos/cosmos-sdk/pull/21906) Skip sims test when running dry on validatorsx/tx/signing/aminojson/json_marshal.go (1)
271-275: Update unit tests to cover new fields innameAndIndexstructThe addition of
oneof,oneofFieldName, andoneofTypeNameto thenameAndIndexstruct enhances the handling of oneof fields during marshaling. Please ensure that unit tests are added or updated to cover scenarios involving these new fields to prevent regressions.Would you like assistance in generating unit tests for these changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)client/keys/migrate.go(0 hunks)docs/docs/build/_category_.json(1 hunks)docs/docs/build/build.md(1 hunks)simapp/app.go(1 hunks)x/genutil/client/cli/migrate.go(1 hunks)x/group/client/cli/tx.go(1 hunks)x/group/client/cli/util.go(1 hunks)x/tx/CHANGELOG.md(1 hunks)x/tx/signing/aminojson/json_marshal.go(4 hunks)
💤 Files with no reviewable changes (1)
- client/keys/migrate.go
✅ Files skipped from review due to trivial changes (3)
- .github/workflows/test.yml
- docs/docs/build/category.json
- x/genutil/client/cli/migrate.go
🔇 Additional comments (8)
docs/docs/build/build.md (1)
1-3: LGTM! Frontmatter is properly structured.
The frontmatter correctly sets the sidebar position to 0, making this the main entry point for the Build section.
x/group/client/cli/util.go (1)
56-56: Verify consistent support for numeric value across related commands.
Let's ensure this enhancement is consistently implemented across all related group module commands.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check the implementation details more thoroughly.
Based on the comprehensive results, I can now provide the final response:
Support for numeric value "1" is consistently implemented across the group module
The verification shows that:
- The CLI layer correctly handles both "try" and "1" values through
execFromStringinutil.go - The keeper layer properly processes these values using
group.Exec_EXEC_TRY - The flag documentation in
tx.goclearly documents this functionality - Test files demonstrate the feature using "try" value
- No inconsistencies found in the implementation across different commands
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in handling of exec flag across group module commands
# Expected: All group proposal execution related code should support both "try" and "1"
# Search for group proposal execution related code
echo "Searching for group proposal execution related files..."
rg -l "group.Exec_EXEC_TRY"
# Search for ExecTry usage in CLI commands
echo "Checking CLI command implementations..."
rg "ExecTry" "x/group/client/cli/"
# Search for flag documentation
echo "Checking flag documentation..."
rg -A 2 "FlagExec" "x/group/client/cli/"
Length of output: 2702
Script:
#!/bin/bash
# Check the actual implementation of exec flag handling in msg_server.go
echo "Checking msg_server implementation..."
rg -A 5 "Exec_EXEC_TRY" x/group/keeper/msg_server.go
# Check if there are any other numeric values being used for exec
echo "Checking for other numeric exec values..."
rg -i "exec.*[\"'](1|2|0)" x/group/
# Check the actual implementation in util.go
echo "Checking util.go implementation..."
cat x/group/client/cli/util.go
Length of output: 5641
x/tx/CHANGELOG.md (2)
36-38: LGTM! Changelog entry follows the correct structure.
The new entry is properly placed under the "Unreleased" section with the correct "Bug Fixes" stanza.
38-38: LGTM! Clear and accurate description of the fix.
The changelog entry clearly describes the bug fix and correctly references PR cosmos#21782, matching the PR objectives regarding the transaction module fix for sorting oneof fields in amino-json.
x/tx/signing/aminojson/json_marshal.go (4)
352-355: Prevent redundant null entries for empty oneof fields
The logic writes null once per empty oneof field, which is appropriate. However, confirm that this does not introduce unintended null entries in the JSON output that could affect downstream processing.
373-373: Verify JSON structure for serialized oneof fields is compliant
The serialization of oneof fields outputs a nested JSON object with type and value keys. Confirm that this structure aligns with the expected Amino JSON format and that it is compatible with existing clients and parsers.
Would you like me to provide example serialized outputs for oneof fields to validate the structure?
312-319:
Ensure robust error handling in getOneOfNames usage
When calling getOneOfNames(f), it's crucial to handle potential errors to prevent runtime exceptions. Verify that getOneOfNames correctly handles all edge cases for field descriptors.
Run the following script to check how getOneOfNames is used and ensure errors are appropriately handled:
#!/bin/bash
# Description: Search for all usages of `getOneOfNames` and examine error handling.
# Expected Result: All calls should handle errors returned by `getOneOfNames`.
rg -A 3 'getOneOfNames\('326-336: Confirm field sorting logic with oneof fields is accurate
The sorting logic now considers oneofFieldName when sorting fields. Ensure that this change maintains the correct field order in the JSON output according to the Amino JSON encoding specifications.
Run the following script to review messages with oneof fields and verify field ordering:
✅ Verification successful
Let me gather more information about the field sorting implementation and oneof handling.
The above script will help us understand:
- The complete context around oneof field name usage in sorting
- Any existing tests that verify this behavior
This will help us verify if the sorting logic is implemented correctly according to Amino JSON specifications.
Let me check the test file and any related test cases to verify the oneof field sorting behavior.
This will help us verify if there are test cases that validate the correct sorting behavior for oneof fields and confirm the implementation follows the Amino JSON specification.
Let me check the actual proto definitions to understand the oneof field structure and test cases.
This will help us understand the oneof field definitions and their corresponding test cases to verify the sorting behavior.
Let me check the implementation of getOneOfNames function and its usage in tests.
This will help us understand how oneof fields are handled and if there are specific test cases validating this behavior.
Based on the implementation review, I can now provide a final response:
Oneof field sorting implementation is correct and follows Amino JSON specification
The sorting logic for oneof fields is properly implemented:
- It correctly uses the oneof field name (
oneofFieldName) from the containing oneof descriptor - The
getOneOfNamesfunction properly handles the Amino JSON specification by:- Using camelCase for the oneof field name
- Requiring the
amino.oneof_type_nameoption for type information - Validating that fields are actually within a oneof
The sorting implementation maintains consistency by:
- Using the oneof field name for comparison when fields are part of a oneof
- Falling back to the regular field name for non-oneof fields
- Applying the same lexicographical ordering (
niName < njName) regardless of field type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all messages with oneof fields to verify field sorting in marshaling.
# Expected Result: Fields are ordered correctly, considering `oneofFieldName`.
ast-grep --pattern $'message $_ {
oneof $_ {
$$$
}
$$$
}'
Length of output: 589
Script:
#!/bin/bash
# Check the implementation of field sorting and oneof handling
rg -B 5 -A 10 'oneofFieldName' x/tx/signing/aminojson/json_marshal.go
# Also check for any tests that verify oneof field sorting
rg -l 'oneof.*test' x/tx/signing/aminojson/
Length of output: 2270
Script:
#!/bin/bash
# Check test implementations for oneof field sorting
rg -B 5 -A 10 'oneof' x/tx/signing/aminojson/internal/testpb/test.pulsar.go
# Also check for any test cases in the main test file
rg -B 5 -A 10 'oneof' x/tx/signing/aminojson/json_marshal_test.go
Length of output: 18367
Script:
#!/bin/bash
# Check proto definitions for oneof fields
rg -B 5 -A 10 'oneof' x/tx/signing/aminojson/internal/testpb/test.proto
# Check test cases specifically for JSON marshaling
rg -B 5 -A 10 'TestMarshal.*oneof|oneof.*Marshal' x/tx/signing/aminojson/json_marshal_test.go
Length of output: 167
Script:
#!/bin/bash
# Check the implementation of getOneOfNames
rg -B 5 -A 15 'func getOneOfNames' x/tx/signing/aminojson/options.go
# Look for any test files containing 'oneof' in the filename
fd -t f oneof
# Check for any test cases with oneof in the description or test name
rg -B 2 -A 2 'func.*Test.*oneof|oneof.*json' x/tx/signing/aminojson/
Length of output: 822
--dry-runflag description forkeys migrate(backport docs(keys): remove unsupported--dry-runflag description forkeys migratecosmos/cosmos-sdk#22057) (docs(keys): remove unsupported--dry-runflag description forkeys migrate(backport #22057) cosmos/cosmos-sdk#22070)genesis migratecommand examples (backport docs(x/genutil): fixgenesis migratecommand examples cosmos/cosmos-sdk#22090) (docs(x/genutil): fixgenesis migratecommand examples (backport #22090) cosmos/cosmos-sdk#22097)Description
Closes: #XXXX
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
Release Notes
New Features
1andtryfor group proposal execution.Improvements
Bug Fixes
Documentation
build.mdfile detailing the application build process.