refactor(server/v2/cometbft): use only protov1 and backport #21084 #21681
Conversation
julienrbrt
added
the
backport/v0.52.x
julienrbrt
requested review from
a team,
facundomedica and
sontrinh16
as code owners
WalkthroughWalkthroughThe changes involve updates to the Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional context usedPath-based instructions (1)
Additional comments not posted (2)
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
|
github-actions
bot
added
C:server/v2
julienrbrt
changed the title
![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 (1)
server/v2/cometbft/utils.go (1)
166-173: Add a TODO comment to track the removal of the hack.The code segment correctly creates an array of
gogoany.Anymessages from the transaction result responses usinggogoany.NewAnyWithCacheWithValue. While the code itself is fine, the comment about using this approach as a hack to maintain the protov2 API suggests that it may be temporary.Consider adding a TODO comment to track the removal of this hack and the transition to a more permanent solution in the future. This will help ensure that the code is properly updated and the API is cleaned up when appropriate.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- server/v2/cometbft/client/rpc/block.go (4 hunks)
- server/v2/cometbft/client/rpc/utils.go (4 hunks)
- server/v2/cometbft/commands.go (2 hunks)
- server/v2/cometbft/go.mod (1 hunks)
- server/v2/cometbft/utils.go (2 hunks)
- simapp/gomod2nix.toml (0 hunks)
Files not reviewed due to no reviewable changes (1)
- simapp/gomod2nix.toml
Files skipped from review due to trivial changes (1)
- server/v2/cometbft/go.mod
Additional context used
Path-based instructions (4)
server/v2/cometbft/client/rpc/utils.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/client/rpc/block.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/utils.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (11)
server/v2/cometbft/client/rpc/utils.go (3)
14-17: LGTM!The function signature update to use
cmttypes.Blockis consistent with the overall refactor and the function logic remains intact.
32-34: LGTM!The function signature updates to use
cmttypes.Blockandsdk.SearchBlocksResultare consistent with the overall refactor and the function logic remains intact.
Line range hint
45-56: LGTM!The function has been appropriately updated to use the new
cmttypespackage andgogoprotoserialization method, which aligns with the overall refactor goals. The changes are sound and maintain the core functionality.server/v2/cometbft/client/rpc/block.go (3)
Line range hint
42-55: LGTM!The function signature update aligns with the PR objective of using only protov1. The change is straightforward and the function logic remains intact.
Line range hint
59-76: LGTM!The function signature update aligns with the PR objective of using only protov1. The change is straightforward and the function logic remains intact.
Line range hint
80-102: LGTM!The function signature update aligns with the PR objective of using only protov1. The change is straightforward and the function logic remains intact.
server/v2/cometbft/utils.go (3)
24-24: LGTM!The import statement for the
github.com/cosmos/cosmos-sdk/typespackage is valid and follows the correct syntax. Thesdkalias is a conventional shorthand for the package.
149-163: Looks good!The code segment correctly converts the transaction result events to ABCI events and maps the event attributes to ABCI event attributes. The
indexAllflag is used appropriately to control the indexing behavior. The code follows the expected logic and does not introduce any issues.
176-189: LGTM!The code segment correctly creates an
sdk.SimulationResponseobject with the relevant fields populated from the transaction result. TheGasInfofield is properly set, and theResultfield contains ansdk.Resultobject with the events, message responses, and error log. The use ofgogoproto.Marshalto marshal the response is appropriate. The code follows the expected logic and does not introduce any issues.server/v2/cometbft/commands.go (2)
10-12: LGTM!The imported packages are relevant and necessary.
204-204: Verify compatibility of serialization change.The change to use
gogoproto.Marshalfor serialization instead ofprotojson.Marshallooks good as it aligns with the goal of enhancing performance and compatibility with CometBFT specific data structures.However, please verify that this serialization change doesn't break any existing functionality or cause issues in other parts of the codebase that may be relying on the previous serialization format.
Description
Simplify cometbft server by using only protov1, instead of going between protov2 <-> protov1
Additionally backports #21084 to cometbft server commands.
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
New Features
Bug Fixes
Documentation