chore: fix proto / typo #308
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several changes across multiple files, primarily focusing on updates to governance and channel state management. Key modifications include the removal of specific import statements, the addition of a new Changes
Possibly related PRs
Suggested reviewers
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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
![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)
proto/initia/gov/v1/gov.proto (1)
Line range hint
145-153: Enhance documentation for the TallyResult message.While the structure is well-defined, consider adding more detailed documentation to clarify:
- The relationship between
total_staking_power,total_vesting_power, and the values inv1_tally_result- The significance of
tally_heightand when it gets updatedAdd documentation comments like this:
// TallyResult defines the result of a tally. +// It extends the cosmos-sdk TallyResult by separately tracking staking and vesting power, +// while maintaining compatibility with the original format. message TallyResult { + // tally_height represents the block height at which the tally was computed uint64 tally_height = 1; + // total_staking_power represents the total voting power from staking at tally_height string total_staking_power = 2 [(cosmos_proto.scalar) = "cosmos.Int"]; + // total_vesting_power represents the total voting power from vesting at tally_height string total_vesting_power = 3 [(cosmos_proto.scalar) = "cosmos.Int"];x/ibc/nft-transfer/keeper/relay.go (1)
Line range hint
48-63: Consider using optional memo parameterThe function now passes an empty string for memo by default. Consider making this parameter optional or using a pointer/Option type to better represent the absence of a memo.
-func (k Keeper) SendNftTransfer( +func (k Keeper) SendNftTransfer( ctx sdk.Context, sourcePort, sourceChannel string, classId string, tokenIds []string, sender sdk.AccAddress, receiver string, timeoutHeight clienttypes.Height, timeoutTimestamp uint64, + memo *string, ) error { + memoStr := "" + if memo != nil { + memoStr = *memo + } _, err := k.sendNftTransfer( ctx, sourcePort, sourceChannel, classId, tokenIds, sender, receiver, timeoutHeight, timeoutTimestamp, - "", + memoStr, ) return err }api/ibc/applications/perm/v1/types.pulsar.go (1)
Migration from gogoproto is incomplete
The verification reveals an inconsistency in the protobuf implementation:
- While
types.pulsar.gohas removed the gogoproto import, the correspondingproto/ibc/applications/perm/v1/types.protoand other .proto files still contain gogoproto imports- This suggests an incomplete migration where generated
.pulsar.gofiles have moved away from gogoproto, but the source.protofiles haven't been updated accordingly🔗 Analysis chain
Line range hint
1-12: Verify the impact of removing gogoproto dependencyThe removal of gogoproto import suggests a migration from gogo protobuf. This change should be consistent across all generated protobuf files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining gogoproto dependencies # Test: Search for gogoproto imports. Expect: No remaining imports. rg -l 'github.com/cosmos/gogoproto/gogoproto' # Test: Search for gogo.proto imports in .proto files. Expect: No remaining imports. rg -l 'gogoproto/gogo.proto' --type protoLength of output: 2547
Script:
#!/bin/bash # Let's check for .proto files first to ensure we're searching the right file type fd '\.proto$' # Then search for gogoproto imports in .proto files fd '\.proto$' --exec grep -l "gogoproto/gogo.proto" {} # Let's also check the specific file we're reviewing for any remaining gogoproto annotations cat api/ibc/applications/perm/v1/types.pulsar.go | grep -A 5 -B 5 "gogo" # Check other .pulsar.go files in the same directory for consistency ls api/ibc/applications/perm/v1/*.pulsar.goLength of output: 3918
api/initia/gov/v1/gov.pulsar.go (1)
Line range hint
5165-5174: Clarify the necessity of both custom fields andV1TallyResultThe
TallyResultstruct includes custom fields (TallyHeight,TotalStakingPower,TotalVestingPower) as well as an embeddedV1TallyResultfrom thecosmos-sdk, which may contain overlapping data. Consider whether including both sets of fields is necessary or if it introduces redundancy. Simplifying the struct could improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
client/docs/swagger-ui/swagger.yamlis excluded by!**/*.yamlx/gov/types/gov.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/ibc/perm/types/types.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/mstaking/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (7)
api/ibc/applications/perm/v1/types.pulsar.go(1 hunks)api/initia/gov/v1/gov.pulsar.go(1 hunks)api/initia/mstaking/v1/query.pulsar.go(3 hunks)proto/ibc/applications/perm/v1/types.proto(0 hunks)proto/initia/gov/v1/gov.proto(1 hunks)proto/initia/mstaking/v1/query.proto(3 hunks)x/ibc/nft-transfer/keeper/relay.go(1 hunks)
🔥 Files not summarized due to errors (1)
- api/initia/mstaking/v1/query.pulsar.go: Error: Server error: no LLM provider could handle the message
💤 Files with no reviewable changes (1)
- proto/ibc/applications/perm/v1/types.proto
✅ Files skipped from review due to trivial changes (1)
- proto/initia/mstaking/v1/query.proto
🔇 Additional comments (9)
proto/initia/gov/v1/gov.proto (2)
Line range hint 154-218: LGTM! Clean integration of TallyResult.
The integration of the new TallyResult message into the Proposal message is well-implemented with appropriate protobuf options and clear documentation.
Line range hint 1-218: Verify PR objective completion.
The PR description mentions fixing a typo related to "mstaking", but no such typo fix is visible in this file. Please verify if:
- The typo fix is in another file not provided for review
- The PR description needs to be updated to match the actual changes
x/ibc/nft-transfer/keeper/relay.go (2)
Line range hint 423-435: LGTM!
The acknowledgement packet handling is well-implemented with proper error handling and clear documentation.
Line range hint 48-63: Verify memo field handling across codebase
Let's ensure all callers of SendNftTransfer are updated to handle the memo field and packet handling is consistent.
api/ibc/applications/perm/v1/types.pulsar.go (3)
768-774: LGTM: Well-structured message definition
The ChannelState message is well-defined with clear field names and helpful documentation for the admin field.
Line range hint 13-767: Generated code follows standard patterns
This is auto-generated protobuf reflection code that follows the expected patterns.
775-791: Auto-generated descriptor data
These changes to the raw descriptor data are automatically generated based on the proto file changes.
api/initia/mstaking/v1/query.pulsar.go (1)
12992-12992:
Fix "mstaking" typo in proto source files
The file path contains "mstaking" which appears to be a typo of "staking". Since this is a generated file, the fix should be applied to the source proto files rather than this generated output.
Let's verify the proto source files:
Also applies to: 13729-13731, 13823-13823
api/initia/gov/v1/gov.pulsar.go (1)
Line range hint 5165-5174: Verify the use of string types for numerical fields
The fields TotalStakingPower and TotalVestingPower are declared as string types. If these fields represent numerical values, consider using numeric types like sdk.Int or math/big.Int for precise arithmetic operations and to avoid potential issues with string parsing and conversion.
Run the following script to check for consistent usage of these fields across the codebase:
✅ Verification successful
Let me gather more information about how these fields are used in the codebase.
Let me gather one final piece of information to understand how these values are handled in math operations.
Using string type for power values is correct in this context
The string type for TotalStakingPower and TotalVestingPower is appropriate here because:
- These fields are part of a protobuf message structure used for serialization/deserialization
- The values are properly handled in the codebase:
- Values are converted from
sdk.Intto string using.String()when setting:TotalStakingPower: totalStakingPower.String() - The actual arithmetic operations are performed using proper numeric types (
math.Int,sdk.Int) before conversion to string - The total power calculation is done correctly:
totalPower := totalVestingPower.Add(totalStakingPower)
- Values are converted from
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `TotalStakingPower` and `TotalVestingPower` are consistently declared and used.
# Search for declarations of TotalStakingPower and TotalVestingPower
ast-grep --pattern $'
type $_ struct {
$$$
TotalStakingPower $_
TotalVestingPower $_
$$$
}'
Length of output: 2245
Script:
#!/bin/bash
# Search for usage of TotalStakingPower and TotalVestingPower in function implementations
rg -A 5 "TotalStakingPower|TotalVestingPower" --type go
Length of output: 12834
Script:
#!/bin/bash
# Search for math operations or conversions involving these fields
rg -B 2 -A 2 "\.String\(\)|\.Add\(|\.IsZero\(|\.Parse" x/gov/keeper/
Length of output: 24827
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #308 +/- ##
==========================================
+ Coverage 41.04% 41.11% +0.07%
==========================================
Files 268 268
Lines 25728 25682 -46
==========================================
Hits 10559 10559
+ Misses 13532 13486 -46
Partials 1637 1637
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
Makefile (1)
Line range hint
159-174: Consider documenting the protobuf generation workflow.The protobuf generation system is well-structured with clear separation between swagger and pulsar generation. Consider adding documentation in the repository (e.g., in
docs/orCONTRIBUTING.md) to explain:
- The purpose and output of each generator
- The relationship between proto files and generated artifacts
- When developers need to run these generators
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
client/docs/swagger-ui/swagger.yamlis excluded by!**/*.yaml
📒 Files selected for processing (1)
Makefile(1 hunks)
🔇 Additional comments (1)
Makefile (1)
159-159: LGTM! The proto-all target update aligns with PR objectives.
The modification to include proto-swagger-gen and proto-pulsar-gen in the proto-all target correctly implements the PR objective of making proto-all run these generators.
| @@ -156,7 +156,7 @@ protoVer=0.14.0 | |||
| protoImageName=ghcr.io/cosmos/proto-builder:$(protoVer) | |||
| protoImage=$(DOCKER) run --rm -v $(CURDIR):/workspace --workdir /workspace $(protoImageName) | |||
|
|
|||
| proto-all: proto-format proto-lint proto-gen | |||
| proto-all: proto-format proto-lint proto-gen proto-swagger-gen proto-pulsar-gen | |||
There was a problem hiding this comment.
💡 Codebase verification
Fix script permissions and verify swagger integration
The verification revealed several issues that need to be addressed:
protocgen-pulsar.shexists but lacks executable permissions- Both scripts are properly integrated into the Makefile targets
protoc-swagger-gen.shcorrectly integrates withupdate-swagger-docstarget- The scripts contain proper error handling with
set -eo pipefail
Required actions:
- Add executable permissions to
protocgen-pulsar.sh:chmod +x ./scripts/protocgen-pulsar.sh
🔗 Analysis chain
Verify the existence and permissions of protoc generation scripts.
The new targets reference shell scripts that need to be verified.
Also applies to: 166-174
Let me verify the content of these scripts and their dependencies to provide a more comprehensive response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and permissions of the protoc generation scripts
# Check if the scripts exist and are executable
for script in ./scripts/protoc-swagger-gen.sh ./scripts/protocgen-pulsar.sh; do
if [[ ! -f "$script" ]]; then
echo "Error: $script does not exist"
exit 1
fi
if [[ ! -x "$script" ]]; then
echo "Warning: $script is not executable"
fi
done
# Verify the integration with update-swagger-docs
if ! grep -q "statik" "client/docs/Makefile" 2>/dev/null; then
echo "Warning: statik integration might be missing in client/docs/Makefile"
fi
Length of output: 639
Script:
#!/bin/bash
# Check the content of the scripts and their dependencies
echo "=== Content of protoc-swagger-gen.sh ==="
cat ./scripts/protoc-swagger-gen.sh
echo -e "\n=== Content of protocgen-pulsar.sh ==="
cat ./scripts/protocgen-pulsar.sh
echo -e "\n=== Checking Makefile targets ==="
grep -A 5 "proto-swagger-gen:" Makefile
grep -A 5 "proto-pulsar-gen:" Makefile
Length of output: 3805
Description
proto/ibc/applications/perm/v1/types.protoproto-allto runproto-swagger-genandproto-pulsar-genAuthor 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...