refactor(serevr/v2/cometbft): update RegisterQueryHandlers and GRPC queries
#22403
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
hieuvubk
requested review from
facundomedica,
sontrinh16,
a team and
julienrbrt
as code owners
|
@hieuvubk your pull request is missing a changelog! |
hieuvubk
changed the title
RegisterQueryHandlers and GRPC queriesRegisterQueryHandlers and GRPC queries
| @@ -282,7 +285,7 @@ func (c *Consensus[T]) maybeRunGRPCQuery(ctx context.Context, req *abci.QueryReq | |||
| } | |||
|
|
|||
| resp, err = queryResponse(res, req.Height) | |||
| return resp, isGRPC, err | |||
| return resp, true, err | |||
There was a problem hiding this comment.
why r we assume isGRPC here is true ?
There was a problem hiding this comment.
is it possible to change the return type to (resp *abciproto.QueryResponse, err error)
There was a problem hiding this comment.
I think we couldn't, its using as flag to run GRPC request or app/p2p/store. Can try by wrapping err with a special message.
julienrbrt
changed the title
RegisterQueryHandlers and GRPC queriesRegisterQueryHandlers and GRPC queries
julienrbrt
added
the
backport/v0.52.x
![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/abci.go (1)
Line range hint
276-280: Avoid variable shadowing ofrespin error handlingIn the
if err != nilblock, declaringrespshadows the outerrespreturn variable. This can lead to confusion and potential bugs. It's better to assign directly to the named return variable or use a different variable name.Apply this diff to fix the issue:
res, err := c.app.Query(ctx, uint64(req.Height), protoRequest) if err != nil { - resp := QueryResult(err, c.cfg.AppTomlConfig.Trace) + resp = QueryResult(err, c.cfg.AppTomlConfig.Trace) resp.Height = req.Height return resp, true, err }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
runtime/v2/manager.go(4 hunks)server/v2/cometbft/abci.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
runtime/v2/manager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
runtime/v2/manager.go (3)
845-850: LGTM: Well-structured constructor function.
The new NewStfRouterWrapper function properly initializes all fields and improves code maintainability by centralizing the initialization logic.
Line range hint 645-649: LGTM: Improved service registration with proper error handling.
The changes to use NewStfRouterWrapper in both message and query handler registration improve code consistency and error handling.
Also applies to: 654-659
864-867:
Verify the impact of resetting handlers on error.
The change to reset the entire handlers map when an error occurs (s.handlers = map[string]appmodulev2.Handler{}) is a significant behavioral change. This could potentially clear valid handlers that were registered before the error occurred.
Consider preserving previously registered handlers and only skipping the current registration instead of clearing all handlers.
-if s.error != nil {
- s.handlers = map[string]appmodulev2.Handler{}
-}
+if s.error == nil {
+ s.handlers[requestName] = handler
+}server/v2/cometbft/abci.go (2)
262-267: LGTM: Correct assignment of handlerFullName
The logic for determining handlerFullName based on whether the query is gRPC is correctly implemented.
Line range hint 270-288: LGTM: Proper handling of gRPC queries in maybeRunGRPCQuery
The updated code uses handlerFullName to retrieve the appropriate handler from queryHandlersMap and processes the gRPC query correctly.
runtime/v2/manager.go
Outdated
| @@ -842,6 +842,13 @@ type stfRouterWrapper struct { | |||
| handlers map[string]appmodulev2.Handler | |||
| } | |||
|
|
|||
| func NewStfRouterWrapper(stfRouterBuilder *stf.MsgRouterBuilder) stfRouterWrapper { | |||
There was a problem hiding this comment.
why is this public? it should be made private imho
runtime/v2/manager.go
Outdated
| @@ -854,7 +861,7 @@ func (s *stfRouterWrapper) RegisterHandler(handler appmodulev2.Handler) { | |||
| s.error = errors.Join(s.error, err) | |||
|
|
|||
| // also make the decoder | |||
| if s.error == nil { | |||
| if s.error != nil { | |||
There was a problem hiding this comment.
shall we just check if s.handlers is nil? Checking the error doesn't seem related tbh
… queries (backport #22403) (#22429) Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: Julien Robert <[email protected]>
* main: (24 commits) build(deps): upgrade to [email protected] (#22436) docs: Update tendermint validators query pagination documentation (#22412) refactor(client/v2): offchain uses client/v2/factory (#22344) feat: wire new handlers to grpc (#22333) fix(x/group): proper address rendering in error (#22425) refactor(serevr/v2/cometbft): update `RegisterQueryHandlers` and GRPC queries (#22403) docs: update ADR 59 (#22423) build(deps): Bump github.com/fsnotify/fsnotify from 1.7.0 to 1.8.0 in /tools/cosmovisor (#22407) docs: Module account address documentation (#22289) feat(baseapp): add per message telemetry (#22175) docs: Update Twitter Links to X in Documentation (#22408) docs: redirect the remote generation page (#22404) refactor(serverv2): remove unused interface methods, honuor context (#22394) fix(server/v2): return ErrHelp (#22399) feat(indexer): implement `schema.HasModuleCodec` interface in the `bank` module (#22349) refactor(math): refactor ApproxRoot for readality (#22263) docs: fix KWallet Handbook (#22395) feat(client/v2): broadcast logic (#22282) refactor(server/v2): eager config loading (#22267) test(system): check feePayers signature (#22389) ...
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 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
stfRouterWrapper.CheckTxmethod for better transaction validation.Improvements
Bug Fixes