fix(x/auth): update proto address conversion query to prevent duplicate HTTP binding #23357
Conversation
📝 WalkthroughWalkthroughThis pull request addresses HTTP binding issues for address conversion methods in the Cosmos SDK. The changes modify the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant AddressConverter
Client->>Server: Request AddressBytesToString
Server->>AddressConverter: Convert bytes to string
AddressConverter-->>Server: Return string representation
Server-->>Client: Respond with converted address
Client->>Server: Request AddressStringToBytes
Server->>AddressConverter: Convert string to bytes
AddressConverter-->>Server: Return byte representation
Server-->>Client: Respond with converted address
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
|
julienrbrt
added
the
backport/v0.52.x
…uplicate-annotation
…uplicate-annotation
technicallyty
requested review from
a team,
julienrbrt and
JulianToledano
as code owners
This comment has been minimized.
This comment has been minimized.
|
@julienrbrt i imagine this should be backported to v0.52 and thats where i should put the changelog entry? i imagine we may want to tag a minor bump for the |
Correct it should be backported. x/auth isn't an extracted module from the SDK, so the changelog should only be in CHANGELOG.md. Once merged you can do the following: git checkout main
git pull
git tag api/v0.8.2 -m 'Release api v0.8.2' -s
git pushAfterwards instead of using the pseudo version in the backport PR from mergify, just use v0.8.2 |
![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
🧹 Nitpick comments (3)
api/cosmos/auth/v1beta1/query.pulsar.go (3)
Line range hint
52-53: Consider documenting the expected account types.The
accountsfield usesgoogle.protobuf.Anyfor flexibility, but it would be helpful to document which concrete account types can be expected here for better type safety and easier client implementation.
9530-9537: Document error handling for address conversion endpoints.The HTTP bindings for address conversion should document possible error cases and their corresponding HTTP status codes, especially for invalid addresses or encoding/decoding failures.
9551-9562: Consider adding pagination support for AccountInfo queries.For consistency with other query endpoints and to handle large result sets efficiently, consider adding pagination support to the AccountInfo query endpoint.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
client/grpc/reflection/reflection.pb.gois excluded by!**/*.pb.goserver/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumx/auth/types/query.pb.gois excluded by!**/*.pb.gox/auth/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (6)
api/cosmos/auth/v1beta1/query.pulsar.go(2 hunks)proto/cosmos/auth/v1beta1/query.proto(1 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
api/cosmos/auth/v1beta1/query.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (6)
proto/cosmos/auth/v1beta1/query.proto (3)
66-71: LGTM! Good approach to maintain backward compatibility.The additional binding for the decode endpoint provides better clarity while maintaining the original endpoint for backward compatibility.
73-77: Endpoint path now better reflects the operation.The change from
/bech32/{address_string}to/bech32/encode/{address_string}improves API clarity by explicitly indicating the operation type.
79-84: LGTM! No functional changes.Only formatting changes were made to improve code readability.
server/v2/cometbft/go.mod (1)
17-17: LGTM! Consistent version update.The version update is consistent with other modules.
tests/go.mod (1)
6-6: LGTM! Consistent version update.The version update is consistent with other modules.
simapp/v2/go.mod (1)
6-6: LGTM! Consistent version update.The version update is consistent with other modules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
CHANGELOG.md (4)
Line range hint
1-1: Add missing title to the CHANGELOG fileThe file should start with a # title like "# Changelog" for better documentation.
+ # Changelog
60-60: Fix inconsistent bullet point styleThe bullet point style is inconsistent with the rest of the file. Other entries use
-while this one uses*.- * (x/auth) [#23357](https://github.com/cosmos/cosmos-sdk/pull/23357) Fixes accessibility of the AddressStringToBytes HTTP binding and adds another binding to AddressBytesToString. + * (x/auth) [#23357](https://github.com/cosmos/cosmos-sdk/pull/23357) Fixes accessibility of the AddressStringToBytes HTTP binding and adds another binding to AddressBytesToString.
Line range hint
785-790: Fix broken link formattingThe link to previous versions contains raw markdown that should be properly formatted.
- [CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/c17c3caab86a1426a1eef4541e8203f5f54a1a54/CHANGELOG.md#v0391---2020-08-11) (pre Stargate). + See [CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/c17c3caab86a1426a1eef4541e8203f5f54a1a54/CHANGELOG.md#v0391---2020-08-11) for changes before Stargate.
Line range hint
791-793: Remove unnecessary code blockThere's an empty code block at the end of the file that should be removed.
- ```
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
client/grpc/reflection/reflection.pb.gois excluded by!**/*.pb.goserver/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumx/auth/types/query.pb.gois excluded by!**/*.pb.gox/auth/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)api/cosmos/auth/v1beta1/query.pulsar.go(2 hunks)proto/cosmos/auth/v1beta1/query.proto(1 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
api/cosmos/auth/v1beta1/query.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze
- GitHub Check: test-system-v2
- GitHub Check: Summary
🔇 Additional comments (12)
api/cosmos/auth/v1beta1/query.pulsar.go (4)
9530-9537: HTTP bindings for address conversion have been updated correctlyThe HTTP bindings for address conversion methods have been properly updated with distinct endpoints:
/cosmos/auth/v1beta1/bech32/decode/{address_bytes}for bytes to string conversion/cosmos/auth/v1beta1/bech32/{address_bytes}as an alternative endpoint
9538-9550: HTTP binding for address string to bytes conversion is properly definedThe endpoint
/cosmos/auth/v1beta1/bech32/encode/{address_string}is correctly defined for string to bytes conversion, maintaining API consistency.
Line range hint
16-24: Message definitions follow protobuf best practicesThe message definitions properly use the Any type for account interfaces and include appropriate pagination support. The code follows protobuf best practices for message definitions.
9562-9575: Package metadata is properly configuredThe generated code includes complete package metadata with proper Go package path and protobuf options. The metadata ensures correct integration with the Cosmos SDK.
proto/cosmos/auth/v1beta1/query.proto (4)
66-71: Well-structured HTTP bindings for address decoding!The addition of a more descriptive
/decode/endpoint while maintaining the original endpoint for backward compatibility is a good approach.
73-77: Consistent HTTP binding pattern for address encoding!The endpoint now follows a consistent pattern with its decode counterpart, using
/encode/to clearly indicate its purpose.
79-84: LGTM - Well-documented RPC method!The method includes all necessary annotations for versioning, safety, and HTTP binding.
66-84: Verify no remaining duplicate HTTP bindings.Let's ensure there are no other duplicate HTTP bindings in the codebase.
✅ Verification successful
HTTP bindings verification successful ✅
The changes have successfully eliminated duplicate HTTP bindings, and no other duplicates were found in the codebase. The new encode/decode path segments are properly organized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for duplicate HTTP bindings in proto files # Expected: No duplicate GET paths should be found # Find all proto files and extract HTTP GET paths rg -t proto 'option \(google\.api\.http\).*get:.*"([^"]+)"' -o | sort | uniq -d # Also check additional_bindings rg -t proto 'additional_bindings.*get:.*"([^"]+)"' -o | sort | uniq -dLength of output: 214
Script:
#!/bin/bash # Find all proto files first, then search within them fd '\.proto$' | xargs rg 'get: "([^"]+)"' -o | sort | uniq -c | awk '$1 > 1 {print $0}'Length of output: 89
server/v2/go.mod (1)
12-12: Verify the accessibility of the pseudo-version.The dependency update to
cosmossdk.io/api v0.8.2-0.20250114014050-e279d7cec391uses a pseudo-version with a timestamp from 2025. This future date might cause issues with dependency resolution.server/v2/cometbft/go.mod (1)
17-17: Consistent dependency update.The update to
cosmossdk.io/api v0.8.2-0.20250114014050-e279d7cec391aligns with the version used in other modules, maintaining consistency across the codebase.tests/go.mod (1)
6-6: Consistent dependency update.The update to
cosmossdk.io/api v0.8.2-0.20250114014050-e279d7cec391aligns with the version used in other modules, maintaining consistency across the codebase.simapp/v2/go.mod (1)
6-6: Consistent dependency update.The update to
cosmossdk.io/api v0.8.2-0.20250114014050-e279d7cec391aligns with the version used in other modules, maintaining consistency across the codebase.
imho that isn't necessary, we have an unit test for it. system tests are expensive in CI time, and this isn't a critical flow and that would basically mean testing the grpc gateway implementation. |
OTOH, this request did lead me to finding a bug, which prompted #23390. however after that is merged, i think that type of surface for bugs is incredibly minimized since custom HTTP handling code is 0 at this point. |
I disagree. We should be aggressive with our testing. System tests are already taking up to 20min in CI, so running more comprehensive tests that act as end to end acceptance tests seems perfectly fine to me. |
More test is indeed better, but we do not need to spin up a testnet with 4 nodes, just to query one endpoint. Just my two cents, but I remember @alpe warned us about using system tests wisely. |
…uplicate-annotation
this makes sense to me. i think for testing that an endpoint works, we should using something lighter than systemtest. @julienrbrt is there an example of creating an application with a server and everything in a test that's not in systemtest? |
There was a problem hiding this comment.
Shouldn't you remove those error log and return error from the todos from https://github.com/cosmos/cosmos-sdk/pull/23262/files#diff-8747e2b8c2dfb0745ab69db88d9d112a05d7b826862319fb00c6af7f0e4ccc2aR239 here too?
There was a problem hiding this comment.
removed the panic handler that ignores duplicate mux registration here 3e52f9d
To test that an endpoint work you do not need even need an app: https://github.com/cosmos/cosmos-sdk/blob/main/x/bank/keeper/grpc_query_test.go#L199-L254. You definitely need some system tests to be sure that the gRPC wiring is correct, but we have those. An integration test doesn't have consensus nor server but is a real app, so you could start a small one (however this needs to be fixed first #22825 😅) However if you really want to add this as a system tests, then i'd just add it in an existing test instead of creating a new one (like this one: https://github.com/cosmos/cosmos-sdk/blob/main/tests/systemtests/auth_test.go#L477-L496) The impact would then be minimal. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/systemtests/auth_test.go (1)
498-540: Document the test address values.The test cases comprehensively cover the address conversion endpoints. Consider adding comments to explain the hardcoded test values:
- What these addresses represent
- Why these specific values were chosen
- How the base64 encoded values were derived
addr := "cosmos1q6cc9u0x5r3fkjcex0rgxee5qlu86w8rh2ypaj" +// Test address representing a valid cosmos address addrBytesURLEncoded := "BrGC8eag4ptLGTPGg2c0B%2Fh9OOM%3D" +// URL-encoded base64 representation of the address bytes addrBytes := "BrGC8eag4ptLGTPGg2c0B/h9OOM=" +// Base64 representation of the address bytes
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
server/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
server/v2/api/grpcgateway/handler.go(1 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)tests/systemtests/auth_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/go.mod
- server/v2/cometbft/go.mod
- simapp/v2/go.mod
- server/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/api/grpcgateway/handler.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/systemtests/auth_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (3)
server/v2/api/grpcgateway/handler.go (1)
73-78: Verify the removal of panic handling for duplicate HTTP annotations.The removal of panic handling could potentially expose the application to unhandled panics from duplicate HTTP annotations. While this aligns with the PR's objective to fix HTTP binding issues, we should verify this change doesn't impact error reporting.
✅ Verification successful
Removal of panic handling is safe and well-structured
The verification shows that duplicate HTTP annotations are prevented by design:
- URIs are managed through a map structure that inherently prevents duplicates
- Registration process is deterministic with sorted URI processing
- Handler initialization is type-safe with proper struct field assignment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any duplicate HTTP annotations that might cause panics ast-grep --pattern 'google.api.http = { $$$ get: "$_" $$$ }'Length of output: 68
Script:
#!/bin/bash # Find proto files and their HTTP annotations fd -e proto -x rg -l "google\.api\.http" # Check the context around handler registration for panic handling rg -B 5 -A 5 "mux\.Handle.*protoHandler" server/Length of output: 1961
tests/systemtests/auth_test.go (2)
Line range hint
478-497: LGTM! Test coverage improved.The test now properly verifies memo preservation during transaction encoding and decoding.
Line range hint
542-600: LGTM! Comprehensive test coverage for fee payer functionality.The test thoroughly covers:
- Failure case when fee payer's signature is missing
- Success case with proper signatures
- Balance verification after fee deduction
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/systemtests/auth_test.go (1)
500-540: Consider enhancing test coverage with additional edge cases.The test cases are well-structured and cover the main scenarios. Consider adding:
- Tests for addresses with different prefixes (e.g.,
osmo1...,juno1...)- Tests for addresses of different lengths
- Tests for malformed base64 encodings
testCases := []systest.RestTestCase{ + { + Name: "should handle different address prefix", + Url: fmt.Sprintf(stringToBytesPath, "osmo1q6cc9u0x5r3fkjcex0rgxee5qlu86w8rjd5225"), + ExpCode: http.StatusOK, + ExpOut: `{"address_bytes":"..."}`, // Add expected bytes + }, + { + Name: "should fail with malformed base64", + Url: fmt.Sprintf(bytesToStringPath, "!@#$"), + ExpCode: http.StatusBadRequest, + ExpOut: `{"code":3,"message":"illegal base64 data","details":[]}`, + },
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
server/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
server/v2/api/grpcgateway/handler.go(1 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)tests/systemtests/auth_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/go.mod
- server/v2/cometbft/go.mod
- server/v2/go.mod
- simapp/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/api/grpcgateway/handler.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/systemtests/auth_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
server/v2/api/grpcgateway/handler.go (1)
73-78: LGTM! Verify route registration behavior.The simplified route registration aligns with the PR's objective to support multiple HTTP bindings. However, let's verify there are no unintended side effects.
✅ Verification successful
Route registration change is safe and verified
The simplified route registration aligns with the codebase's HTTP routing patterns. All routes are properly namespaced by module and version, with no conflicting patterns that could cause routing issues. The removal of panic handling is safe in this context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential duplicate HTTP routes that might conflict # Look for routes with similar patterns but different HTTP methods # Search for HTTP rule annotations in proto files rg -A 5 "option \(google\.api\.http\)"Length of output: 68298
tests/systemtests/auth_test.go (2)
7-7: LGTM!The addition of net/http import is appropriate for the new REST API test cases.
Line range hint
478-499: LGTM! Function name accurately reflects expanded scope.The function rename better describes its expanded functionality, and the chain start is correctly positioned before the REST queries.
…te HTTP binding (#23357) (cherry picked from commit 9a27317) # Conflicts: # CHANGELOG.md # api/cosmos/auth/v1beta1/query.pulsar.go # server/v2/api/grpcgateway/handler.go # server/v2/cometbft/go.mod # server/v2/cometbft/go.sum # server/v2/go.mod # simapp/v2/go.mod # simapp/v2/go.sum # tests/go.mod # tests/go.sum # x/params/go.sum
Description
Closes: #23281
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
Dependency Updates
cosmossdk.io/apidependency to versionv0.8.2-0.20250114014050-e279d7cec391across multiple modulesAPI Modifications
Bug Fixes