chore: use comet proto type instead of custom #20255
Conversation
WalkthroughWalkthroughThe recent updates aim to enhance the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 4
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
client/grpc/cmtservice/query.pb.gois excluded by!**/*.pb.go
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- api/cosmos/base/tendermint/v1beta1/query.pulsar.go (27 hunks)
- client/grpc/cmtservice/service.go (1 hunks)
- client/grpc/cmtservice/types.go (1 hunks)
- proto/cosmos/base/tendermint/v1beta1/query.proto (3 hunks)
Files not summarized due to errors (1)
- api/cosmos/base/tendermint/v1beta1/query.pulsar.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (2)
- client/grpc/cmtservice/service.go
- client/grpc/cmtservice/types.go
Additional Context Used
GitHub Check Runs (1)
break-check failure (3)
proto/cosmos/base/tendermint/v1beta1/query.proto: [failure] 175-175:
Previously present field "8" with name "proof_ops" on message "ABCIQueryResponse" was deleted.
Path-based Instructions (2)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"api/cosmos/base/tendermint/v1beta1/query.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (32)
proto/cosmos/base/tendermint/v1beta1/query.proto (3)
13-13: The addition of the import statement for "cometbft/crypto/v1/proof.proto" aligns with the PR's objective to standardize the use of Comet proto types. This change should help in integrating the crypto structures more seamlessly.
188-188: Updating theproof_opsfield to use a fully qualified proto path (.cometbft.crypto.v1.ProofOps) enhances clarity and helps prevent type conflicts. This change is a good practice in proto definitions to ensure that the correct types are used, especially when similar names might exist in different packages.
Line range hint
175-175: The static analysis tool flagged the deletion of the previously existing field "8" named "proof_ops" in theABCIQueryResponsemessage. This change is not reflected in the provided code, suggesting a possible oversight. Please verify this change to ensure that it aligns with the intended modifications and does not affect backward compatibility.CHANGELOG.md (3)
184-184: LGTM! The description is clear and concise.
184-184: LGTM! The description is clear and concise.
184-184: LGTM! The description is clear and concise.api/cosmos/base/tendermint/v1beta1/query.pulsar.go (26)
8449-8449: The addition of the field descriptor forproof_opsaligns well with the PR's objectives to integrate Comet proto types. Good job on maintaining consistency.
8463-8463: Correct assignment of the field descriptor forproof_ops. This is necessary for the integration of the new Comet proto types.
4-4: Ensure the surrounding logic for theHeightfield check integrates well with the rest of the function. The provided snippet is correct.
8579-8584: Proper handling of theProofOpsfield using proto reflection methods. This is consistent with the integration of the new Comet proto types.
8616-8617: Correct addition of theproof_opscase in the switch statement. This ensures proper handling of the newProofOpsfield.
8650-8651: Proper handling of the clearing of theProofOpsfield. This is necessary for correct data management.
8692-8694: Correct handling for retrieving theProofOpsfield value using proto reflection methods. This ensures consistency with the new field addition.
8731-8732: Correct handling for setting theProofOpsfield value. This ensures that the field is correctly populated with the expected type.
8755-8755: Proper initialization of theProofOpsfield usingnew. This is necessary to ensure that the field is ready for use.
8803-8805: Correct handling for creating a newProofOpsinstance for a field value. This ensures that the field is correctly populated.
8904-8907: Correct calculation of the size of theProofOpsfield. This is necessary for accurate serialization.
Line range hint
13-13: Proper marshaling of theProofOpsfield with appropriate error handling. This ensures data integrity during serialization.
8949-8950: Proper encoding of theProofOpsfield. This ensures that the field is correctly represented in the serialized data.
9220-9224: Proper unmarshaling of theHeightfield with appropriate error handling and wire type checking. This ensures data integrity during deserialization.
9240-9243: Proper unmarshaling of theCodespacefield with appropriate error handling and wire type checking. This ensures data integrity during deserialization.
9271-9273: Proper unmarshaling of theProofOpsfield with appropriate error handling and wire type checking. This ensures data integrity during deserialization.
9300-9305: Proper unmarshaling of theProofOpsfield with appropriate error handling. This ensures data integrity during deserialization.
11176-11176: Correct update of theABCIQueryResponsestruct to include the newProofOpsfield. This aligns with the PR's objectives to integrate Comet proto types.
Line range hint
20-20: Correct implementation of the getter method for theHeightfield. This ensures proper encapsulation and data access.
11255-11260: Correct implementation of the getter method for theProofOpsfield. This ensures proper encapsulation and data access.
11323-11324: Correct marking of theProofOpstype as deprecated. This aligns with the PR's objectives to move away from older structures.
11385-11387: Correct inclusion of necessary imports for proto definitions. This ensures that all required types are available in the file.
11524-11524: Correct definition of fields in the proto message. This ensures that the message structure is properly specified.
11533-11540: Correct continuation of field definitions in the proto message. This ensures that the message structure is properly specified.
11695-11695: Correct definition of type references in the proto file. This ensures that the types are correctly linked and accessible.
11714-11714: Correct definition of type references and service methods in the proto file. This ensures that the types and methods are correctly linked and defined.
| @@ -181,6 +181,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
| * (baseapp) [#19993](https://github.com/cosmos/cosmos-sdk/pull/19993) Indicate pruning with error code "not found" rather than "invalid request". | |||
| * (x/consensus) [#20010](https://github.com/cosmos/cosmos-sdk/pull/20010) Move consensus module to be its own go.mod | |||
| * (server) [#20140](https://github.com/cosmos/cosmos-sdk/pull/20140) Remove embedded grpc-web proxy in favor of standalone grpc-web proxy. [Envoy Proxy](https://www.envoyproxy.io/docs/envoy/latest/start/start) | |||
| * (client) [#20255](https://github.com/cosmos/cosmos-sdk/pull/20255) Use comet proofOp proto type instead of sdk version to avoid needing to translate to later be proven in the merkle proof runtime. | |||
There was a problem hiding this comment.
Remove trailing spaces at the end of the line.
- * (client) [#20255](https://github.com/cosmos/cosmos-sdk/pull/20255) Use comet proofOp proto type instead of sdk version to avoid needing to translate to later be proven in the merkle proof runtime.
+ * (client) [#20255](https://github.com/cosmos/cosmos-sdk/pull/20255) Use comet proofOp proto type instead of sdk version to avoid needing to translate to later be proven in the merkle proof runtime.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * (client) [#20255](https://github.com/cosmos/cosmos-sdk/pull/20255) Use comet proofOp proto type instead of sdk version to avoid needing to translate to later be proven in the merkle proof runtime. | |
| * (client) [#20255](https://github.com/cosmos/cosmos-sdk/pull/20255) Use comet proofOp proto type instead of sdk version to avoid needing to translate to later be proven in the merkle proof runtime. |
| option deprecated = true; | ||
| string type = 1; | ||
| bytes key = 2; | ||
| bytes data = 3; |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that the ProofOp and ProofOps messages are still present and referenced in the proto/cosmos/base/tendermint/v1beta1/query.proto file. This contradicts the expectation that all references to ProofOp should have been updated or a migration path should have been documented following its deprecation.
Action Required:
- Update all references to
ProofOpandProofOpsin the codebase to use the new standardized structures. - Alternatively, ensure that a clear migration path is documented for transitioning from
ProofOpandProofOpsto the new structures.
Locations needing attention:
proto/cosmos/base/tendermint/v1beta1/query.proto: References toProofOpandProofOpsare still present.
Analysis chain
Marking the ProofOp message as deprecated is consistent with the move towards using more standardized structures. Ensure that all references to ProofOp in the codebase are updated to use the new structures, or that a migration path is clearly documented.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all references to `ProofOp` have been updated or that a migration path is documented.
# Test: Search for the usage of `ProofOp`. Expect: No occurrences or documented migration path.
rg --type proto $'ProofOp'
Length of output: 55
Script:
#!/bin/bash
# Description: Re-verify that all references to `ProofOp` have been updated or that a migration path is documented.
# Test: Search for the usage of `ProofOp` in .proto files. Expect: No occurrences or documented migration path.
rg 'ProofOp' -g '*.proto'
Length of output: 907
| option deprecated = true; | ||
| repeated ProofOp ops = 1 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true]; |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed that the ProofOps message and its references are still present in the proto/cosmos/base/tendermint/v1beta1/query.proto file. This contradicts the expectation that all usages of ProofOps would have been updated or that a migration strategy would be in place following its deprecation.
proto/cosmos/base/tendermint/v1beta1/query.proto: Contains the definition ofProofOpsand references to it.
Analysis chain
Similarly, marking the ProofOps message as deprecated aligns with the PR's goals. It's important to verify that all usages of ProofOps have been appropriately updated or that a migration strategy is in place.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all references to `ProofOps` have been updated or that a migration path is documented.
# Test: Search for the usage of `ProofOps`. Expect: No occurrences or documented migration path.
rg --type proto $'ProofOps'
Length of output: 56
Script:
#!/bin/bash
# Description: Re-verify that all references to `ProofOps` have been updated or that a migration path is documented.
# Test: Search for the usage of `ProofOps` in .proto files. Expect: No occurrences or documented migration path.
rg 'ProofOps' --glob '*.proto'
Length of output: 437
| @@ -2,6 +2,7 @@ | |||
| package tendermintv1beta1 | |||
|
|
|||
| import ( | |||
| v12 "buf.build/gen/go/cometbft/cometbft/protocolbuffers/go/cometbft/crypto/v1" | |||
There was a problem hiding this comment.
Consider removing the unused import _ "cosmossdk.io/api/amino" to clean up the codebase and avoid including unnecessary dependencies.
Description
ref #20250
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.
I have...
Summary by CodeRabbit
New Features
ABCIQueryfunction to return more detailed responses includingProofOps,Height, andCodespace.Refactor
FromABCIResponseQueryfunction to streamline response handling.Documentation