fix: remove grpc replace directive #11089

fdymylja · 2022-02-01T23:04:06Z

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...

Reviewers 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...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

fdymylja · 2022-02-01T23:05:29Z

@aaronc, @AmauryM as a note for app wiring WG, we should attempt to remove temporal application configurations, such as GRPCRouter.SetInterfaceRegistry, they're generally bad and hard to debug and keep track of.

fdymylja · 2022-02-01T23:18:59Z

NOTE: also #10997 will not work as expected on certain cases without this fix due to conflicts between protobuf types and gogoproto types and interface unpacking...

fdymylja · 2022-02-02T00:49:51Z

BREAKING changes:

client.Context always expects a Codec (should we relax this?)
grpc clients will have to use a ForceCodec option otherwise it will default to the protov2 codec which will cause panics.

fdymylja · 2022-02-02T01:21:50Z

All tests seem to pass...

This removes the grpc replace from go.mod and unblocks the Tendermint module upgrade.

Unsure about where to exactly place the GRPCCodec part, and also I didn't want to break the codec.Codec interface...

tac0turtle

Generally fine with this approach since its temporary until we fully migrate to orm. which will m most likely be 1 or 2 major releases.

Does this allow us to bump grpc-gateway and swagger as well?

fdymylja · 2022-02-02T11:11:46Z

Generally fine with this approach since its temporary until we fully migrate to orm. which will m most likely be 1 or 2 major releases.

Does this allow us to bump grpc-gateway and swagger as well?

if they were blocked on gRPC then I'd assume yes

amaury1093

thanks a lot @fdymylja for digging into this, really appreciate it

go.mod

amaury1093 · 2022-02-02T15:38:12Z

client/config/config_test.go

@@ -27,7 +29,8 @@ func initClientContext(t *testing.T, envVar string) (client.Context, func()) {
 	home := t.TempDir()
 	clientCtx := client.Context{}.
 		WithHomeDir(home).
-		WithViper("")
+		WithViper("").
+		WithCodec(codec.NewProtoCodec(codectypes.NewInterfaceRegistry()))


client.Context always expects a Codec (should we relax this?)

How would you relax it? Probably only WithInterfaceRegistry is required?

Yes given the interface registry we have our codec too.

As far as relaxing goes, we could default to the gogoproto marshaler (if no codec is specified) but IDK how it would behave with Interface unpacking (let's assume WithInterfaceRegistry was not called).

If it worked before with the default gRPC codec then I would assume it also would work this way.

with orm landing I don't think we can default to gogoproto. I think the force is fine for a single release and but then most teams will have migrated anyways

server/grpc/server_test.go

tac0turtle · 2022-02-02T15:51:38Z

how does this effect js and other lang clients?

fdymylja · 2022-02-02T15:57:26Z

how does this effect js and other lang clients?

totally unaffected, for instance if you were to use dynamic-cosmos with the pulsar generated types those changes do not touch you at all.

they touch every codebase relying on client.Context this is why I was trying to find a solution to relax the panic if Codec is not set in client.Context,

and they also touch every codebase relying on grpc.Dial because they'll need to pass in the application codec. IDK how many people queried the chain with gRPC only without client.Context.

tac0turtle · 2022-02-02T16:40:55Z

can we add this to 0.46?

fdymylja · 2022-02-02T17:01:36Z

can we add this to 0.46?

depends on the timeline for 0.46, the change is quite breaking for [cosmos-sdk go] clients and i'd like to give them time to adapt and digest the change.

amaury1093 · 2022-02-03T16:54:27Z

can we add this to 0.46?

depends on the timeline for 0.46, the change is quite breaking for [cosmos-sdk go] clients and i'd like to give them time to adapt and digest the change.

What's your feeling on this @marbar3778?

I think this can go in for 0.46. For users using grpc.Dial, we can add some docs. And anyways the majority still should use client.Context, which we can make it non-breaking (i.e. relax).

tac0turtle · 2022-02-03T17:04:15Z

its fine by me and fixes many issues users are running into

amaury1093

LGTM, pending some docs/changelogs

server/grpc/server_test.go

fdymylja · 2022-02-03T17:24:18Z

LGTM, pending some docs/changelogs

Thank you. Will wrap up by EOD with relaxation over client.Context, docs updates and changelog.

…/fix-grpc

fdymylja · 2022-02-03T19:26:21Z

Almost ready, going to test grpc json endpoints to check if the marshaller/unmarshaller works correctly.

fdymylja · 2022-02-03T19:43:28Z

@AmauryM, @marbar3778 applied the relaxation over client context, updated docs and tested grpc web, the json transcoding endpoints (api exposed at 1317), all seems to work.

Wanted to get a final ACK, then it's good to merge.

fix: remove grpc replace directive

93f06bd

fix: grpc server uses the sdk codec

7c57e60

fdymylja added 2 commits February 2, 2022 00:52

fix: grpc proto codec switches

d6e2c7a

fix: client context

edc88fe

github-actions bot added the C:CLI label Feb 2, 2022

fix: config_test.go

6f19e6d

fix: baseapp_test.go

2635966

fdymylja marked this pull request as ready for review February 2, 2022 01:18

fdymylja requested review from aaronc and alexanderbez as code owners February 2, 2022 01:18

fdymylja requested review from tac0turtle, robert-zaremba, aaronc and amaury1093 and removed request for aaronc and alexanderbez February 2, 2022 01:19

tac0turtle reviewed Feb 2, 2022

View reviewed changes

fdymylja added 2 commits February 2, 2022 12:32

Merge branch 'master' into fdymylja/fix-grpc

2a57f4e

Merge branch 'master' into fdymylja/fix-grpc

531ae62

amaury1093 approved these changes Feb 2, 2022

View reviewed changes

Merge branch 'master' into fdymylja/fix-grpc

429eca3

Merge branch 'master' into fdymylja/fix-grpc

959abeb

tac0turtle approved these changes Feb 3, 2022

View reviewed changes

fdymylja and others added 2 commits February 3, 2022 14:09

Merge branch 'master' into fdymylja/fix-grpc

5f95158

chore: remove go.mod replace comment

3542565

amaury1093 approved these changes Feb 3, 2022

View reviewed changes

server/grpc/server_test.go Show resolved Hide resolved

fdymylja and others added 3 commits February 3, 2022 19:25

Merge branch 'master' into fdymylja/fix-grpc

788798e

change: relax panic on client.Context

d7a0a1e

Merge remote-tracking branch 'origin/fdymylja/fix-grpc' into fdymylja…

f21d60f

…/fix-grpc

fdymylja added 3 commits February 3, 2022 20:34

chore: update run node docs

176ca8e

chore: fix doc

e027b5f

chore: add doc on codec.go

93bf06f

fdymylja and others added 2 commits February 3, 2022 20:52

chore: update CHANGELOG.md

1fdcc20

Merge branch 'master' into fdymylja/fix-grpc

0223ae7

fdymylja added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 4, 2022

tac0turtle approved these changes Feb 4, 2022

View reviewed changes

mergify bot merged commit 3f7e3e8 into master Feb 4, 2022

mergify bot deleted the fdymylja/fix-grpc branch February 4, 2022 10:54

RollkitBot mentioned this pull request Aug 24, 2023

[Snyk] Upgrade tailwindcss from 3.2.2 to 3.3.3 rollkit/cosmos-sdk-old#44

Closed

0Tech mentioned this pull request Oct 20, 2023

fix: remove grpc replace directive Finschia/finschia-sdk#1153

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove grpc replace directive #11089

fix: remove grpc replace directive #11089

fdymylja commented Feb 1, 2022

fdymylja commented Feb 1, 2022

fdymylja commented Feb 1, 2022 •

edited

Loading

fdymylja commented Feb 2, 2022 •

edited

Loading

fdymylja commented Feb 2, 2022

tac0turtle left a comment •

edited

Loading

fdymylja commented Feb 2, 2022

amaury1093 left a comment

amaury1093 Feb 2, 2022

fdymylja Feb 2, 2022

fdymylja Feb 2, 2022

tac0turtle Feb 3, 2022

tac0turtle commented Feb 2, 2022

fdymylja commented Feb 2, 2022

tac0turtle commented Feb 2, 2022

fdymylja commented Feb 2, 2022

amaury1093 commented Feb 3, 2022 •

edited

Loading

tac0turtle commented Feb 3, 2022 •

edited

Loading

amaury1093 left a comment

fdymylja commented Feb 3, 2022

fdymylja commented Feb 3, 2022

fdymylja commented Feb 3, 2022

fix: remove grpc replace directive #11089

fix: remove grpc replace directive #11089

Conversation

fdymylja commented Feb 1, 2022

Description

Author Checklist

Reviewers Checklist

fdymylja commented Feb 1, 2022

fdymylja commented Feb 1, 2022 • edited Loading

fdymylja commented Feb 2, 2022 • edited Loading

fdymylja commented Feb 2, 2022

tac0turtle left a comment • edited Loading

Choose a reason for hiding this comment

fdymylja commented Feb 2, 2022

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Feb 2, 2022

Choose a reason for hiding this comment

fdymylja Feb 2, 2022

Choose a reason for hiding this comment

fdymylja Feb 2, 2022

Choose a reason for hiding this comment

tac0turtle Feb 3, 2022

Choose a reason for hiding this comment

tac0turtle commented Feb 2, 2022

fdymylja commented Feb 2, 2022

tac0turtle commented Feb 2, 2022

fdymylja commented Feb 2, 2022

amaury1093 commented Feb 3, 2022 • edited Loading

tac0turtle commented Feb 3, 2022 • edited Loading

amaury1093 left a comment

Choose a reason for hiding this comment

fdymylja commented Feb 3, 2022

fdymylja commented Feb 3, 2022

fdymylja commented Feb 3, 2022

fdymylja commented Feb 1, 2022 •

edited

Loading

fdymylja commented Feb 2, 2022 •

edited

Loading

tac0turtle left a comment •

edited

Loading

amaury1093 commented Feb 3, 2022 •

edited

Loading

tac0turtle commented Feb 3, 2022 •

edited

Loading