fix(x/auth/tx): add missing CacheWithValue for ExtensionOptions #23144
Conversation
📝 WalkthroughWalkthroughThis pull request introduces changes to the Cosmos SDK, focusing on updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
|
mmsqe
requested review from
facundomedica,
testinginprod,
aaronc and
a team
as code owners
![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
🧹 Nitpick comments (4)
x/auth/tx/gogotx.go (3)
10-10: Check import alias consistency and clarity.
The import aliasgogoprotomight be confusing given it's also referencing thegithub.com/cosmos/gogoproto/types/anypackage. Consider whether a more descriptive alias (e.g.,gogotypes) would increase clarity.
277-279: Check the error handling ofdecodeFromAny.
value, _ = gogoproto.NewAnyWithCacheWithValue(msg)ignores the second return parameter. Consider proper error handling to catch potential issues (e.g., invalid message).if msg, err := decodeFromAny(cdc, v2); err == nil { - value, _ = gogoproto.NewAnyWithCacheWithValue(msg) + value, createErr := gogoproto.NewAnyWithCacheWithValue(msg) + if createErr != nil { + // handle error or log it + } }
281-287: Fallback logic is correct; consider additional logging.
Falling back to acodectypes.Anyhelps ensure robust decoding. Adding debug logs here may assist troubleshooting.CHANGELOG.md (1)
Line range hint
12-15: Consider adding migration guide linksWhile breaking changes are well documented, consider adding direct links to migration guides at the top of major versions to help users upgrade smoothly.
📜 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/gogotx.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
x/auth/tx/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/tx/gogotx.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"
🔇 Additional comments (6)
x/auth/tx/gogotx.go (3)
240-240: ExtensionOptions retrieval looks solid.
Returning intoAnyV1(w.cdc, w.Tx.Body.ExtensionOptions) is aligned with the new function signature. Ensure that code invoking GetExtensionOptions expects the updated type.
244-244: NonCriticalExtensionOptions retrieval aligned.
Similar to GetExtensionOptions, returning intoAnyV1(w.cdc, w.Tx.Body.NonCriticalExtensionOptions) is consistent with the new approach.
274-274: Refactored function signature for codec-based conversion.
Introducing cdc codec.BinaryCodec improves flexibility. Just ensure that all callers now provide the correct codec.
x/auth/tx/builder.go (1)
49-49: Ensure correct handling of single-element slice return.
PublicKey: intoAnyV1(codec, []*anypb.Any{sigInfo.PublicKey})[0] presumes that intoAnyV1 returns a non-empty slice. It might be safer to check the slice length before indexing to avoid unintended panics.
CHANGELOG.md (2)
Line range hint 1-1: LGTM: Well-structured changelog following standard conventions
The changelog follows proper semantic versioning and standard sections (Features, Improvements, Bug Fixes, Breaking Changes). Each change is well-documented with PR links and clear descriptions.
Line range hint 1-2000: Verify all PR/issue links are valid
The changelog contains numerous links to PRs and issues. It would be good to verify these are all valid and accessible.
✅ Verification successful
Based on the output from checking all PR/issue links in the changelog, I can see that most links are valid (returning HTTP 200), with only a few redirects (HTTP 301/302). The redirects are typically for:
- Issue links that have been converted to PRs
- Some very old PRs (e.g. Improper use of bcrypt API #3129)
- A few empty PR URLs (ending with just
/pull/)
Since all links are either valid or properly redirecting, and no links are broken (no 404s or 5xx errors), I can provide the final response:
All PR and issue links in the changelog are valid and accessible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Extract and verify all PR/issue links
grep -o 'https://github.com/cosmos/cosmos-sdk/pull/[0-9]*\|https://github.com/cosmos/cosmos-sdk/issues/[0-9]*' CHANGELOG.md | while read -r url; do
curl -s -o /dev/null -w "%{http_code}" "$url"
done
Length of output: 70737
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 6f2bab5)
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...
!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
CacheWithValueforExtensionOptionsin thex/auth/txmodule.API Breaking Changes
x/paramsmodule; users should migrate to the new parameters system introduced in v0.47.testutil/networkpackage; recommended to use integration or system tests framework instead.Improvements