fix(x/auth/tx): add missing timeoutTimestamp in newBuilderFromDecodedTx #23492
Conversation
📝 WalkthroughWalkthroughThe pull request addresses a bug in the Cosmos SDK's transaction builder where the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
CHANGELOG.md (1)
Line range hint
1-1000: Add missing section for security fixesThe changelog should include a dedicated section for security-related fixes to help users identify critical updates.
# Changelog ## [v0.47.16](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.16) - 2024-12-16 +### Security + +* Fix [ABS-0043/ABS-0044](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-8wcc-m6j2-qxvm) Limit recursion depth for unknown field detection and unpack any +* (x/auth/vesting) [GHSA-4j93-fm92-rp4m] Add BlockedAddr check in CreatePeriodicVestingAccount + ### Features
🧹 Nitpick comments (3)
x/auth/tx/builder_test.go (1)
21-37: Strengthen the assertions in this test.
The test only checks thattimeoutTimestampis not zero but does not confirm it matches a specific expected time. Consider explicitly setting a known timestamp and verifying that the builder retains this exact value post-encoding/decoding.CHANGELOG.md (2)
57-58: Fix inconsistent formatting in bug fix entryThe bug fix entry should follow the same format as other entries with a proper description.
-* (x/auth/tx) [#23492](https://github.com/cosmos/cosmos-sdk/pull/23492) Add missing timeoutTimestamp in newBuilderFromDecodedTx. +* (x/auth/tx) [#23492](https://github.com/cosmos/cosmos-sdk/pull/23492) Added missing timeoutTimestamp field in newBuilderFromDecodedTx function to properly handle transaction timeouts.
Line range hint
1-1000: Improve changelog organizationThe changelog would be more readable with consistent formatting and grouping of related changes.
- Group related changes together within sections
- Use consistent punctuation at the end of entries
- Standardize capitalization in descriptions
- Add subsection headers for major components when there are many related changes
Example structure:
### API Breaking Changes #### Baseapp * Change 1 * Change 2 #### Modules * Change 1 * Change 2
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)x/auth/tx/builder.go(1 hunks)x/auth/tx/builder_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
x/auth/tx/builder_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/auth/tx/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (5)
x/auth/tx/builder_test.go (2)
9-10: Imports look fine.
No apparent issues. They appear consistent with Go conventions and do not raise any style or performance concerns.
13-13: New import is appropriate.
The newly added import forauthclientis relevant to reading transactions from file and is necessary for the test.x/auth/tx/builder.go (1)
66-66: Validate or document the timestamp usage.
Storing the timeout timestamp from the decoded transaction is correct. However, consider clarifying or validating edge-case timestamps (e.g., in the distant past or future) to prevent potential misuse.CHANGELOG.md (2)
Line range hint
1-3: LGTM: Standard changelog header formatThe header follows the standard format with version number and release date.
Line range hint
13-20: Verify completeness of breaking changes sectionThe breaking changes section should be verified against the actual code changes to ensure no breaking changes are missed.
✅ Verification successful
Breaking changes section is complete and accurate
The breaking changes section in CHANGELOG.md correctly documents all breaking changes. The recent code modifications only include a bug fix (adding missing timeoutTimestamp) and do not introduce any new breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any additional breaking changes in the codebase # Look for interface changes, removed functions, etc. rg -g '*.go' -l 'interface|func.*\bremove\b|func.*\bdelete\b|func.*\bdeprecate\b' | while read -r file; do echo "Checking $file for potential breaking changes..." rg 'interface|func.*\bremove\b|func.*\bdelete\b|func.*\bdeprecate\b' "$file" doneLength of output: 131070
Script:
#!/bin/bash # Search for breaking change markers in recent commits echo "=== Checking recent commits for breaking change markers ===" git log --oneline -n 20 | grep -i "breaking" # Check CHANGELOG.md for breaking changes section echo -e "\n=== Checking CHANGELOG.md for breaking changes ===" if [ -f CHANGELOG.md ]; then echo "Breaking changes section from CHANGELOG.md:" awk '/^## \[Unreleased\]/,/^## \[[0-9]/' CHANGELOG.md | grep -A 5 "BREAKING CHANGES" fi # Get list of modified files echo -e "\n=== Checking modified files ===" git diff --name-only HEAD~1Length of output: 624
Script:
#!/bin/bash # Check the actual changes in builder.go echo "=== Changes in x/auth/tx/builder.go ===" git diff HEAD~1 x/auth/tx/builder.go # Check CHANGELOG.md changes echo -e "\n=== Changes in CHANGELOG.md ===" git diff HEAD~1 CHANGELOG.mdLength of output: 1414
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
CHANGELOG.md (1)
Changelog Format Issues Found
The changelog has several formatting inconsistencies that should be addressed:
- The guiding principles and usage instructions at the top of the file should be moved to a separate documentation file or properly formatted as comments using HTML comment tags
- All changelog entries should start with either
*or-bullet points- Metadata and section headers should be clearly separated from changelog entries
Recommended fixes:
- Move the "Guiding Principles" and "Usage" sections to a CONTRIBUTING.md file
- Ensure all changelog entries start with bullet points
- Use proper HTML comment syntax for any non-changelog content that must remain in the file
The core changelog structure (sections and version headers) is correct, but these formatting improvements will make the changelog more consistent and easier to read.
🔗 Analysis chain
Line range hint
1-15000: Verify changelog format consistencyLet's verify that all changelog entries follow the Keep a Changelog format conventions.
|>|[|<!--)" CHANGELOG.md |
grep -v "^[A-Z]" |
grep -v "^[[:space:]]*$" |
head -n 10--- |>|<!--|\[|ATTENTION:|EOL|Security|This|Users|\s*\}|\s*\{))" CHANGELOG.md | \ while IFS=: read -r line_num content; do echo "Line $line_num: $content" done echo "Checking for proper section headers..." grep -n "^###" CHANGELOG.md🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent changelog entry formatting echo "Checking changelog format consistency..." # Check for proper section headers if ! grep -q "^### Features" CHANGELOG.md || \ ! grep -q "^### Bug Fixes" CHANGELOG.md || \ ! grep -q "^### Improvements" CHANGELOG.md; then echo "Error: Missing standard changelog sections" exit 1 fi # Check entry format (should start with * or -) if grep -Pv "^(\s*[\*\-]|#|$)" CHANGELOG.md; then echo "Error: Found entries not starting with * or -" exit 1 fi echo "Changelog format appears consistent"Length of output: 17303
Script:
#!/bin/bash # Find lines that don't match expected changelog format but should grep -Pv "^(\s*[\*\-]|###|##|#|$|\s*Length of output: 116
Script:
#!/bin/bash echo "Checking for improperly formatted changelog entries..." # Find lines that don't match expected format but excluding valid elements grep -n -P "^(?!(\s*[\*\-]|###|##|#|$|\s*Length of output: 235
Script:
#!/bin/bash echo "=== Checking CHANGELOG.md format ===" echo "1. Checking for required sections..." for section in "### Features" "### Bug Fixes" "### Improvements" "### API Breaking Changes" "### State Machine Breaking"; do if grep -q "^$section" CHANGELOG.md; then echo "✓ Found section: $section" else echo "✗ Missing section: $section" fi done echo -e "\n2. Checking entry formatting..." # Count lines that should be entries (non-empty, non-header, non-code block lines) grep -v "^$\|^#\|^[[:space:]]*$\|^[[:space:]]*\`\`\`" CHANGELOG.md | \ grep -v "^[[:space:]]*[-*]" | \ head -n 5 echo -e "\n3. Checking version headers..." grep "^## \[v" CHANGELOG.md | head -n 3Length of output: 2158
🧹 Nitpick comments (3)
x/auth/tx/builder_test.go (1)
21-37: Expand the test to validate an explicit timestamp value.While the test checks that
timeoutTimestampis set (not zero), consider setting and verifying a specific timestamp value to ensure the builder preserves the exact value.CHANGELOG.md (2)
Line range hint
1-1: Add missing title and version badgeThe changelog should start with a clear title and version badge for better visibility and reference.
+ # Changelog + [](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.0)
Line range hint
1-15000: Improve version organizationThe changelog mixes multiple versions together which makes it harder to track changes for specific releases. Consider splitting into separate files per major version.
Consider reorganizing the changelog into separate files:
- CHANGELOG_v0.47.md
- CHANGELOG_v0.46.md
- CHANGELOG_v0.45.md
etc.This would make it easier to track changes for specific major versions while keeping the main CHANGELOG.md focused on the latest release.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)x/auth/tx/builder.go(1 hunks)x/auth/tx/builder_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
x/auth/tx/builder_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/auth/tx/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: stf
- GitHub Check: Check go mod tidy
- GitHub Check: server-v2
- GitHub Check: build (arm64)
- GitHub Check: runtime-v2
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: dependency-review
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (5)
x/auth/tx/builder_test.go (2)
9-13: Good addition of required imports.These import statements align with the usage in the forthcoming test. Everything looks consistent with typical Go import style.
20-20: Empty line clarified.The blank line here is purely stylistic and doesn't pose any issue. Proceeding without concerns.
x/auth/tx/builder.go (1)
66-66: Ensure decoded timeout timestamp aligns with actual blockchain expectations.Using
decoded.GetTimeoutTimeStamp()is correct for persisting the field, but confirm it reflects the intended semantic of “timeout” (e.g., block time or OS time). If the chain uses block time differently, convert or clarify in documentation.CHANGELOG.md (2)
57-58: LGTM: Clear bug fix entryThe bug fix entry for adding missing timeoutTimestamp is well documented with a clear reference to the PR number.
Line range hint
1-15000: Verify PR referencesLet's verify that all PR references are valid and point to existing pull requests.
julienrbrt
added
the
backport/v0.52.x
…Tx (backport #23492) (#23497) Co-authored-by: mmsqe <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #23491
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Bug Fixes
Tests
The changes enhance transaction management by ensuring proper timeout configuration and validation.