Prefer sending tx_bytes to Simulate gRPC endpoint #8926

amaury1093 · 2021-03-18T15:03:08Z

Description

closes: #8425
closes: #7726

This PR does 2 things, both around tx simulation:

deprecate the tx field on SimulateRequest, prefer tx_bytes.
- it's more consistent with the Broadcast gRPC endpoint, which also accepts tx_bytes
- the old tx field has not been removed, as to not introduce any proto-breaking change
- but if the old tx field is used, then there might be sig verification errors, as described in adr-020
refactor CalculateGas to use the tx service gRPC query client (instead of hardcoding the path)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

orijbot · 2021-03-18T15:57:49Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=a716ec678810892f266ddf4b813597e54552f409

orijbot · 2021-03-18T15:57:51Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=c3d1f8488c564a40998331a59298d240

orijbot · 2021-03-18T15:58:19Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=8ee3a37892a78a17b43a285f89da219541f60bd4

orijbot · 2021-03-18T15:58:20Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=9d7dac8f4cd94ad0bbb81315316fa4c8

amaury1093 · 2021-03-18T16:08:22Z

x/auth/client/tx.go

-//
-// TODO:/XXX: Using a package-level global isn't ideal and we should consider
-// refactoring the module manager to allow passing in the correct module codec.
-var Codec codec.Marshaler


orijbot · 2021-03-18T16:17:00Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=76d16d4b0bc296871e09132da0ee377ae25352a9

orijbot · 2021-03-18T16:17:01Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=43e834b9481b47598a9891d7edd70832

orijbot · 2021-03-19T10:26:12Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=806c3b5f77b8a05026428d15d990cf789654153c

codecov · 2021-03-19T10:38:29Z

Codecov Report

Merging #8926 (6412b30) into master (8ee9da7) will increase coverage by 0.00%.
The diff coverage is 52.00%.

❗ Current head 6412b30 differs from pull request most recent head 375b439. Consider uploading reports for the commit 375b439 to get more accurate results

@@           Coverage Diff           @@
##           master    #8926   +/-   ##
=======================================
  Coverage   58.88%   58.88%           
=======================================
  Files         571      571           
  Lines       32120    32111    -9     
=======================================
- Hits        18914    18910    -4     
+ Misses      10984    10981    -3     
+ Partials     2222     2220    -2

Impacted Files	Coverage Δ
simapp/simd/cmd/root.go	`61.47% <ø> (-0.32%)`	⬇️
x/auth/client/tx.go	`39.28% <ø> (ø)`
x/auth/tx/builder.go	`74.24% <ø> (ø)`
client/tx/tx.go	`29.62% <43.75%> (+0.53%)`	⬆️
x/auth/tx/service.go	`73.61% <66.66%> (+0.75%)`	⬆️
x/bank/keeper/send.go	`83.62% <0.00%> (-0.87%)`	⬇️
types/int.go	`72.53% <0.00%> (-0.39%)`	⬇️
x/bank/keeper/keeper.go	`72.91% <0.00%> (-0.28%)`	⬇️
simapp/test_helpers.go	`0.49% <0.00%> (+<0.01%)`	⬆️

blushi

It seems like a test is failing (related to sig verification, in the test that still uses Tx).
Should this also close #7726?

x/auth/tx/service.go

x/auth/tx/service_test.go

…425-fix-simulation

Co-authored-by: Marie Gauthier <[email protected]>

…os-sdk into am/8425-fix-simulation

technicallyty

lgtm, just one snippet to remove from a merge conflict

CHANGELOG.md

fdymylja

LGTM!

alessio

Good one. ACK from me.

orijbot · 2021-03-24T16:06:53Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=519f6431cda33e56a216d890cc017f4d5a338123

orijbot · 2021-03-24T16:42:15Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=21239dafa30240f09a8b4cc751e6eeee

orijbot · 2021-03-24T16:42:23Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=40cebc3bfbbcfcbabd886e8bc7e3755827a4a4c4

orijbot · 2021-03-25T10:06:40Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=e812b923cb3146cf92d803380ed82b6d

orijbot · 2021-03-25T10:06:47Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=5c2e4a46b7254e0b32807e44458aae4e6992b9d7

orijbot · 2021-03-25T10:53:01Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=7a9f196407fb4eeea888ae2c3493fb12

orijbot · 2021-03-25T10:53:07Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=1fa2c22d8a1cfb63d0d0e49074a7720d1a81e660

robert-zaremba · 2021-03-25T14:24:52Z

client/tx/tx.go

 	"errors"
 	"fmt"
 	"net/http"
 	"os"

+	gogogrpc "github.com/gogo/protobuf/grpc"


typealias is not needed.

robert-zaremba · 2021-03-25T14:25:44Z

client/tx/tx.go

 func WriteGeneratedTxResponse(
-	ctx client.Context, w http.ResponseWriter, br rest.BaseReq, msgs ...sdk.Msg,
+	clientCtx client.Context, w http.ResponseWriter, br rest.BaseReq, msgs ...sdk.Msg,


usually we don't need to repeat a typename in the variable name.

* First run * Remove dead code * Make test pass * Proto gen * Fix lint * Add changelog * Fix tests * Fix test * Update x/auth/tx/service.go Co-authored-by: Marie Gauthier <[email protected]> * Remove protoTxProvider * Add grpc-gateway test * Add comment * move to api breaking * lesser diff * remove conflict * empty commit to rerun CI * empty commit to rerun CI Co-authored-by: Marie Gauthier <[email protected]> Co-authored-by: Alessio Treglia <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

amaury1093 added 3 commits March 18, 2021 10:58

First run

7149841

Remove dead code

0a6b14a

Make test pass

981f68d

amaury1093 added the C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. label Mar 18, 2021

amaury1093 added 2 commits March 18, 2021 16:54

Proto gen

2858900

Fix lint

a716ec6

Merge branch 'master' into am/8425-fix-simulation

8ee3a37

Add changelog

4165d78

amaury1093 commented Mar 18, 2021

View reviewed changes

Fix tests

76d16d4

amaury1093 marked this pull request as ready for review March 19, 2021 09:34

amaury1093 requested review from aaronc, alessio and alexanderbez as code owners March 19, 2021 09:34

Fix test

806c3b5

clevinson assigned blushi, jgimeno and fdymylja Mar 19, 2021

alessio mentioned this pull request Mar 19, 2021

SDK Core Team Sprint [2021.03.19] #8944

Closed

blushi reviewed Mar 22, 2021

View reviewed changes

x/auth/tx/service.go Outdated Show resolved Hide resolved

x/auth/tx/service_test.go Outdated Show resolved Hide resolved

amaury1093 and others added 2 commits March 22, 2021 12:56

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am/8…

67a4d77

…425-fix-simulation

Update x/auth/tx/service.go

7a9cd64

Co-authored-by: Marie Gauthier <[email protected]>

amaury1093 added 2 commits March 23, 2021 11:46

Merge master

8e6c7aa

Merge branch 'am/8425-fix-simulation' of ssh://github.com/cosmos/cosm…

a38d91c

…os-sdk into am/8425-fix-simulation

technicallyty suggested changes Mar 23, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

remove conflict

c557f76

fdymylja self-requested a review March 24, 2021 11:05

fdymylja approved these changes Mar 24, 2021

View reviewed changes

alessio approved these changes Mar 24, 2021

View reviewed changes

Merge branch 'master' into am/8425-fix-simulation

6412b30

alessio requested a review from technicallyty March 24, 2021 12:21

alessio approved these changes Mar 24, 2021

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 24, 2021

Merge branch 'master' into am/8425-fix-simulation

519f643

technicallyty approved these changes Mar 24, 2021

View reviewed changes

Merge branch 'master' into am/8425-fix-simulation

40cebc3

amaury1093 added 3 commits March 25, 2021 10:49

Merge branch 'master' into am/8425-fix-simulation

6d9e49d

empty commit to rerun CI

1dae35e

Merge branch 'master' into am/8425-fix-simulation

5c2e4a4

empty commit to rerun CI

375b439

mergify bot merged commit 1fa2c22 into master Mar 25, 2021

mergify bot deleted the am/8425-fix-simulation branch March 25, 2021 10:52

robert-zaremba reviewed Mar 25, 2021

View reviewed changes

client/tx/tx.go

"errors"

"fmt"

"net/http"

"os"

gogogrpc "github.com/gogo/protobuf/grpc"

Copy link

Collaborator

robert-zaremba Mar 25, 2021

Choose a reason for hiding this comment

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

typealias is not needed.

robert-zaremba reviewed Mar 25, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer sending tx_bytes to Simulate gRPC endpoint #8926

Prefer sending tx_bytes to Simulate gRPC endpoint #8926

amaury1093 commented Mar 18, 2021 •

edited

Loading

orijbot commented Mar 18, 2021

orijbot commented Mar 18, 2021

orijbot commented Mar 18, 2021

orijbot commented Mar 18, 2021

amaury1093 Mar 18, 2021

orijbot commented Mar 18, 2021

orijbot commented Mar 18, 2021

orijbot commented Mar 19, 2021

codecov bot commented Mar 19, 2021 •

edited

Loading

blushi left a comment

technicallyty left a comment

fdymylja left a comment

alessio left a comment

orijbot commented Mar 24, 2021

orijbot commented Mar 24, 2021

orijbot commented Mar 24, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

robert-zaremba Mar 25, 2021

robert-zaremba Mar 25, 2021

Prefer sending tx_bytes to Simulate gRPC endpoint #8926

Prefer sending tx_bytes to Simulate gRPC endpoint #8926

Conversation

amaury1093 commented Mar 18, 2021 • edited Loading

Description

orijbot commented Mar 18, 2021

orijbot commented Mar 18, 2021

orijbot commented Mar 18, 2021

orijbot commented Mar 18, 2021

amaury1093 Mar 18, 2021

Choose a reason for hiding this comment

orijbot commented Mar 18, 2021

orijbot commented Mar 18, 2021

orijbot commented Mar 19, 2021

codecov bot commented Mar 19, 2021 • edited Loading

Codecov Report

blushi left a comment

Choose a reason for hiding this comment

technicallyty left a comment

Choose a reason for hiding this comment

fdymylja left a comment

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

orijbot commented Mar 24, 2021

orijbot commented Mar 24, 2021

orijbot commented Mar 24, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

orijbot commented Mar 25, 2021

robert-zaremba Mar 25, 2021

Choose a reason for hiding this comment

robert-zaremba Mar 25, 2021

Choose a reason for hiding this comment

amaury1093 commented Mar 18, 2021 •

edited

Loading

codecov bot commented Mar 19, 2021 •

edited

Loading