use bech32 addresses for mapped address queries; resolves #1719 #1766
Conversation
WalkthroughThe pull request introduces extensive modifications to the API specifications and service definitions for the Quicksilver Chain. Key changes include the addition of several new endpoints, updates to existing endpoint paths and response schemas, and the introduction of new gRPC methods for querying mapped and inverse mapped accounts. The changes enhance the functionality and clarity of the API, aligning it with new requirements for address encoding and data handling. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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
🧹 Outside diff range and nitpick comments (4)
x/interchainstaking/keeper/grpc_query_test.go (1)
1182-1315: Update test case names to reflect the function under testIn the
TestKeeper_InverseMappedAccountsfunction, the test case names start with "MappedAccounts_", which may cause confusion. Consider renaming them to start with "InverseMappedAccounts_" to accurately represent the function being tested.proto/quicksilver/interchainstaking/v1/query.proto (2)
103-104: Be cautious with changing API endpoint paths to avoid breaking clientsChanging the HTTP GET path for the
MappedAccountsRPC method may affect clients that rely on the existing endpoint. Consider maintaining backward compatibility by supporting the old path or incrementing the API version.
298-299: Changing field type in response message may break client compatibilityUpdating
remote_address_mapfrommap<string, bytes>tomap<string, string>changes the wire format and can break clients that depend on the previous message structure. Ensure that clients are ready to handle this change or consider versioning the API messages to maintain compatibility.docs/swagger.yml (1)
2899-2938: Add summary and descriptions for the new API endpointThe newly added endpoint
/quicksilver/interchainstaking/v1/mapped_addresses/remote/{chain_id}/{remote_address}lacks asummaryfield and parameter descriptions. Including a summary, detailed description, and parameter descriptions will enhance the API documentation and improve clarity for users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/interchainstaking/types/query.pb.gois excluded by!**/*.pb.gox/interchainstaking/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (4)
docs/swagger.yml(4 hunks)proto/quicksilver/interchainstaking/v1/query.proto(2 hunks)x/interchainstaking/keeper/grpc_query.go(1 hunks)x/interchainstaking/keeper/grpc_query_test.go(2 hunks)
🔇 Additional comments (3)
x/interchainstaking/keeper/grpc_query.go (1)
360-383: The new InverseMappedAccounts method is well implemented
The InverseMappedAccounts method correctly handles input validation, retrieves the necessary zone information, and properly returns the local address corresponding to the given remote address and chain ID.
x/interchainstaking/keeper/grpc_query_test.go (1)
1433-1438: Good addition of validation for Bech32 address decoding
The added validation in TestKeeper_MappedAccounts ensures that the returned addresses are correctly encoded in Bech32 format and can be decoded using the appropriate account prefix.
proto/quicksilver/interchainstaking/v1/query.proto (1)
301-307: New RPC method and messages are appropriately defined
The InverseMappedAccounts RPC method and its request and response messages are well-defined and align with existing conventions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1766 +/- ##
==========================================
- Coverage 61.46% 61.41% -0.05%
==========================================
Files 196 196
Lines 17079 17105 +26
==========================================
+ Hits 10497 10505 +8
- Misses 5735 5747 +12
- Partials 847 853 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/interchainstaking/keeper/grpc_query.go (2)
353-356: Consider logging errors during address encodingWhen an error occurs while encoding the address with
EncodeAddressToBech32, the error is currently ignored, and the iteration continues. For better observability and debugging, consider logging the error to help identify potential issues with specific addresses.
383-383: Enhance error message for clarityThe error message when no local address is found could be more informative. Currently, it only mentions the chain ID:
return nil, status.Error(codes.NotFound, fmt.Sprintf("no local address found matching %s", req.ChainId))Consider including the remote address to provide clearer context:
return nil, status.Error(codes.NotFound, fmt.Sprintf("no local address found for remote address %s on chain %s", req.RemoteAddress, req.ChainId))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/interchainstaking/keeper/grpc_query.go(1 hunks)
🔇 Additional comments (2)
x/interchainstaking/keeper/grpc_query.go (2)
342-357: LGTM
The updates to the MappedAccounts method, including changing remoteAddressMap to map[string]string and handling errors from EncodeAddressToBech32, are implemented correctly.
364-387: LGTM
The new InverseMappedAccounts method is well-implemented with proper input validation and error handling. It effectively retrieves the local address mapped to a given remote address and chain ID.
1. Summary
Fixes #1719
2.Type of change
Summary by CodeRabbit
New Features
Modifications
Bug Fixes
Tests