fix: remove denom from query path #123
Conversation
WalkthroughThis pull request introduces several modifications to the Changes
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
=======================================
Coverage 49.37% 49.37%
=======================================
Files 57 57
Lines 4249 4249
=======================================
Hits 2098 2098
Misses 1721 1721
Partials 430 430 |
![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 and nitpick comments (4)
go.mod (2)
Line range hint
3-5: Fix Go version inconsistency.There's an inconsistency between the
godirective (1.22.2) and thetoolchaindirective (1.23.0). This could lead to build issues or unexpected behavior.Apply this diff to align the versions:
-go 1.22.2 +go 1.23.0 toolchain go1.23.0
Line range hint
246-248: Document temporary dependency override.The custom fork of cometbft-client is used as a temporary solution. Consider adding a TODO comment to track when this can be reverted.
Add a TODO comment to track the temporary nature of this override:
// use custom version until this PR is merged // - https://github.com/strangelove-ventures/cometbft-client/pull/10 +// TODO: Remove this override once PR #10 is merged replace github.com/strangelove-ventures/cometbft-client => github.com/initia-labs/cometbft-client v0.0.0-20240924071428-ef115cefa07eapi/opinit/opchild/v1/query.pulsar.go (2)
Line range hint
4-4: Documentation Update NeededThe TODO comment about adding tests should be addressed, especially since this is a query interface that clients depend on.
Would you like me to help create a test template for these query methods?
Line range hint
12-24: Consider Adding Response ValidationThe query response messages don't have any validation logic. Consider adding validation for critical fields, especially for responses that include addresses or numeric values.
Example validation could include:
- Non-negative sequence numbers
- Well-formed addresses
- Non-empty required fields
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/opchild/types/query.pb.gois excluded by!**/*.pb.gox/opchild/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (4)
api/opinit/opchild/v1/query.pulsar.go(2 hunks)api/opinit/opchild/v1/types.pulsar.go(16 hunks)go.mod(1 hunks)proto/opinit/opchild/v1/query.proto(1 hunks)
🔇 Additional comments (9)
proto/opinit/opchild/v1/query.proto (2)
57-57: Implementation aligns with PR objectives
The change successfully removes "denom" from the query path as intended in the PR title "fix: remove denom from query path". The implementation moves the parameter to what appears to be a query parameter, which is a cleaner approach for optional parameters.
57-57:
Breaking Change: API endpoint modification
The modification of the BaseDenom endpoint from /opinit/opchild/v1/base_denom/{denom} to /opinit/opchild/v1/base_denom is a breaking change that will affect existing API clients. While moving from path parameter to query parameter is valid, this change requires careful consideration of backward compatibility.
Let's verify the impact on existing API clients:
Consider the following recommendations:
- Document this breaking change in the changelog
- Consider implementing an API version upgrade strategy
- Update API documentation to reflect the new endpoint structure
Would you like me to help draft the changelog entry or documentation updates?
go.mod (1)
27-27: Verify API version bump impact.
The API version bump from v0.5.1 to v0.5.8 is significant and may introduce breaking changes. Please ensure all dependent code has been updated accordingly.
Let's verify the impact:
✅ Verification successful
Let me gather more information about the API usage to better understand the impact.
API version bump appears safe to proceed
The version bump from v0.5.1 to v0.5.8 appears to be safe based on the following findings:
- The API is used in a limited scope, primarily in autocli configurations and type definitions
- The repository has a local replacement directive (
replace github.com/initia-labs/OPinit/api => ./api), meaning it's using a local copy of the API - The upgrade was done gradually (v0.5.1 -> v0.5.7 -> v0.5.8) suggesting controlled updates
- No references to the old version remain in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old API version and verify imports
# Test: Search for old version references. Expect: No matches for old version.
rg "v0\.5\.1" --type go
# Test: Check all Go files importing the API package to ensure they're compatible
rg --type go "github.com/initia-labs/OPinit/api"
Length of output: 338
Script:
#!/bin/bash
# Check the actual usage of the API types and methods in the codebase
rg --type go -A 5 "github.com/initia-labs/OPinit/api"
# Look for any potential version-specific API usage
ast-grep --pattern 'import $_ "github.com/initia-labs/OPinit/api/$$$"'
# Check git history for the version bump to see what changed
git log -p go.mod | grep -A 5 -B 5 "github.com/initia-labs/OPinit/api"
Length of output: 3063
api/opinit/opchild/v1/types.pulsar.go (4)
3814-3815: LGTM: New field declaration and getter are well-structured.
The new HookMaxGas field is properly documented and uses an appropriate type (uint64) for storing gas limits. The generated getter method follows standard protobuf patterns.
Also applies to: 3880-3885
173-173: LGTM: Field descriptor initialization follows standard patterns.
The field descriptor for HookMaxGas is properly declared and initialized following the same pattern as other fields in the file.
Also applies to: 185-185
289-294: LGTM: Reflection method implementations are complete and correct.
The field handling is properly implemented across all required reflection methods:
- Range, Has, Clear, Get, Set, Mutable, and NewField methods
- Size calculation for wire format
- Marshal/Unmarshal operations with proper error handling
- Consistent with protobuf patterns and other fields in the file
Also applies to: 322-323, 352-353, 397-399, 438-439, 484-485, 514-515, 613-615, 645-649, 918-936
Line range hint 1-4284: Verify completeness of field integration.
Let's verify that all necessary references to the new field are present in the codebase.
✅ Verification successful
The search results show that the HookMaxGas field is properly integrated across the codebase:
- Proto/Go generated files:
- Properly defined in types.pb.go and types.pulsar.go with correct protobuf tags
- All necessary getter/setter methods generated
- Proper serialization/deserialization handling
- Business logic integration:
- Used in msg_server.go for handling bridge hooks with gas limits
- Default value defined in params.go
- Test coverage in msg_server_test.go
- Parameter handling:
- Default value set to 1,000,000
- Included in parameter initialization
- Proper YAML tags for configuration
Let me verify the proto files specifically since the previous command had an issue with proto file type:
Based on the comprehensive verification results, I can now provide a final response:
Field integration is complete and properly implemented
The HookMaxGas field is fully integrated across all necessary layers:
- Properly defined in proto file with correct tags and documentation
- Generated code includes all necessary serialization/deserialization handling
- Business logic integration present in msg_server.go
- Default values and parameters properly configured
- Test coverage exists
The implementation follows all the necessary patterns for protobuf field additions and is consistent with the codebase standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any missed references to HookMaxGas field
# This helps ensure the field is properly integrated everywhere it should be
# Search for HookMaxGas references
echo "Searching for HookMaxGas references..."
rg --type go "HookMaxGas"
# Search for hook_max_gas references (proto style)
echo "Searching for hook_max_gas references..."
rg --type proto "hook_max_gas"
Length of output: 2715
Script:
#!/bin/bash
# Search specifically in .proto files
echo "Searching for hook_max_gas in .proto files..."
find . -name "*.proto" -exec grep -l "hook_max_gas" {} \;
# Show the context of hook_max_gas in proto files
echo -e "\nShowing context in proto files:"
find . -name "*.proto" -exec grep -A 2 -B 2 "hook_max_gas" {} \;
Length of output: 491
api/opinit/opchild/v1/query.pulsar.go (2)
Line range hint 1-6485: LGTM for Generated Code
The overall structure of the generated protobuf code looks correct and follows standard patterns. The message definitions and service interfaces are well-organized.
6469-6471:
API Endpoint Change Verification Required
The endpoint path for BaseDenom query has been modified to remove the denom parameter from the path. This change needs verification to ensure all clients are updated accordingly.
Run the following script to check for any remaining old endpoint usages:
✅ Verification successful
Based on the search results, I can now provide a final response:
API Endpoint Change is Safe and Consistent
The endpoint change in query.pulsar.go is consistent with the protobuf definition in proto/opinit/opchild/v1/query.proto which defines the route as /opinit/opchild/v1/base_denom. The changes are part of the generated code and correctly reflect the API design in the proto files.
- The endpoint is properly defined in the proto file and consistently implemented across generated files
- No old endpoint pattern
/base_denom/{denom}was found in the codebase - The query interface and handler implementations are consistent with the new path
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old endpoint pattern
rg -A 2 "/base_denom/\{denom\}"
# Search for client code that might need updates
ast-grep --pattern 'client.$$$base_denom$$$'
Length of output: 80
Script:
#!/bin/bash
# Let's try a broader search for base_denom related patterns
rg -i "base.?denom" -A 2
# Also search for any gRPC route definitions
rg "/opinit/opchild/v1/base_denom" -A 2
# Search for protobuf route definitions
fd "\.proto$" --exec grep -l "base_denom" {} \;
# Look for any client implementations
ast-grep --pattern 'func $$$BaseDenom$$$'
Length of output: 68225
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 changeReviewers 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...
Summary by CodeRabbit
New Features
HookMaxGasto specify the maximum gas limit for hook execution.BaseDenomendpoint to simplify requests.Bug Fixes
Chores