Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(serevr/v2/cometbft): update RegisterQueryHandlers and GRPC queries #22403

Merged
merged 3 commits into from
Nov 5, 2024
Merged

refactor(serevr/v2/cometbft): update RegisterQueryHandlers and GRPC queries #22403

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc99806
make bankv2 query work
hieuvubk Oct 31, 2024
ec0fb44
feedback
hieuvubk Nov 5, 2024
5852a7f
fix
hieuvubk Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions runtime/v2/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ func registerServices[T transaction.Tx](s appmodulev2.AppModule, app *App[T], re

// if module implements register msg handlers
if module, ok := s.(appmodulev2.HasMsgHandlers); ok {
wrapper := stfRouterWrapper{stfRouter: app.msgRouterBuilder}
wrapper := NewStfRouterWrapper(app.msgRouterBuilder)
module.RegisterMsgHandlers(&wrapper)
if wrapper.error != nil {
return fmt.Errorf("unable to register handlers: %w", wrapper.error)
Expand All @@ -651,7 +651,7 @@ func registerServices[T transaction.Tx](s appmodulev2.AppModule, app *App[T], re

// if module implements register query handlers
if module, ok := s.(appmodulev2.HasQueryHandlers); ok {
wrapper := stfRouterWrapper{stfRouter: app.queryRouterBuilder}
wrapper := NewStfRouterWrapper(app.queryRouterBuilder)
module.RegisterQueryHandlers(&wrapper)

for path, handler := range wrapper.handlers {
Expand Down Expand Up @@ -842,6 +842,13 @@ type stfRouterWrapper struct {
handlers map[string]appmodulev2.Handler
}

func NewStfRouterWrapper(stfRouterBuilder *stf.MsgRouterBuilder) stfRouterWrapper {
Copy link
Member

@julienrbrt julienrbrt Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this public? it should be made private imho

wrapper := stfRouterWrapper{stfRouter: stfRouterBuilder}
wrapper.error = nil
wrapper.handlers = map[string]appmodulev2.Handler{}
return wrapper
}

func (s *stfRouterWrapper) RegisterHandler(handler appmodulev2.Handler) {
req := handler.MakeMsg()
requestName := gogoproto.MessageName(req)
Expand All @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we just check if s.handlers is nil? Checking the error doesn't seem related tbh

s.handlers = map[string]appmodulev2.Handler{}
}
s.handlers[requestName] = handler
Expand Down
9 changes: 6 additions & 3 deletions server/v2/cometbft/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,12 +259,15 @@ func (c *Consensus[T]) maybeRunGRPCQuery(ctx context.Context, req *abci.QueryReq
return nil, false, err
}

var handlerFullName string
md, isGRPC := desc.(protoreflect.MethodDescriptor)
if !isGRPC {
return nil, false, nil
handlerFullName = string(desc.FullName())
} else {
handlerFullName = string(md.Input().FullName())
}

handler, found := c.queryHandlersMap[string(md.Input().FullName())]
handler, found := c.queryHandlersMap[handlerFullName]
if !found {
return nil, true, fmt.Errorf("no query handler found for %s", req.Path)
}
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why r we assume isGRPC here is true ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to change the return type to (resp *abciproto.QueryResponse, err error)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// InitChain implements types.Application.
Expand Down
Loading