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.

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

change(serverv2/comet): use codec to parse gRPC requests #21105

Closed
testinginprod opened this issue Jul 29, 2024 · 3 comments · Fixed by #22566
Closed

change(serverv2/comet): use codec to parse gRPC requests #21105

testinginprod opened this issue Jul 29, 2024 · 3 comments · Fixed by #22566
Assignees
Labels
C:server/v2 Issues related to server/v2

Comments

@testinginprod
Copy link
Contributor

We need to pass codec to comet to properly decode gRPC requests.

Originally posted by @tac0turtle in #21038 (comment)

@github-actions github-actions bot added the needs-triage Issue that needs to be triaged label Jul 29, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Cosmos-SDK Jul 29, 2024
@testinginprod testinginprod changed the title change(serverv2/comet): use change(serverv2/comet): use codec to parse gRPC requests Jul 29, 2024
@tac0turtle tac0turtle added C:server/v2 Issues related to server/v2 C:server/v2 and removed needs-triage Issue that needs to be triaged C:server/v2 labels Jul 30, 2024
@psiphi5
Copy link
Contributor

psiphi5 commented Aug 14, 2024

I had a look at the PR that had the v2/server change (#21038) and wanted to ask if this would require re-adding the GRPCQueryDecoders field the App struct? Also why is it better to decode via codec, since both ways return a gogoproto.Message?

@psiphi5
Copy link
Contributor

psiphi5 commented Aug 20, 2024

Hi @tac0turtle I had a look at gogoproto and wanted to check if using a codec would mean a change like:

diff --git a/server/v2/cometbft/abci.go b/server/v2/cometbft/abci.go
index b7a300b05f..c105a4f57b 100644
--- a/server/v2/cometbft/abci.go
+++ b/server/v2/cometbft/abci.go
@@ -10,6 +10,7 @@ import (
        abci "github.com/cometbft/cometbft/abci/types"
        abciproto "github.com/cometbft/cometbft/api/cometbft/abci/v1"
        gogoproto "github.com/cosmos/gogoproto/proto"
+       gogocodec "github.com/cosmos/gogoproto/codec"

        coreappmgr "cosmossdk.io/core/app"
        "cosmossdk.io/core/comet"
@@ -192,7 +193,7 @@ func (c *Consensus[T]) Query(ctx context.Context, req *abciproto.QueryRequest) (
        makeGRPCRequest, isGRPC := c.grpcMethodsMap[req.Path]
        if isGRPC {
                protoRequest := makeGRPCRequest()
-               err = gogoproto.Unmarshal(req.Data, protoRequest) // TODO: use codec
+               err = gogocodec.New(len(req.Data)).Unmarshal(req.Data, protoRequest)
                if err != nil {
                        return nil, fmt.Errorf("unable to decode gRPC request with path %s from ABCI.Query: %w", req.Path, err)
                }

@tac0turtle
Copy link
Member

@testinginprod is this still needed?

@julienrbrt julienrbrt self-assigned this Nov 20, 2024
@julienrbrt julienrbrt moved this from 📋 Backlog to 🤸‍♂️ In Progress in Cosmos-SDK Nov 20, 2024
@github-project-automation github-project-automation bot moved this from 👀 Waiting / In review to 🥳 Done in Cosmos-SDK Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:server/v2 Issues related to server/v2
Projects
Status: 🥳 Done
Development

Successfully merging a pull request may close this issue.

4 participants