Fix escrow proto E2E #3517

chatton · 2023-04-24T15:35:58Z

Description

closes: #XXXX

We were getting this error

panic: invalid Go type math.Int for field ibc.applications.transfer.v1.QueryTotalEscrowForDenomResponse.amount [recovered]
        panic: invalid Go type math.Int for field ibc.applications.transfer.v1.QueryTotalEscrowForDenomResponse.amount

In our E2E tests. This PR is being used to debug this proto issue.

By making the Amount field a string we can serialize it properly.

EDIT: After discussion with @colin-axner , we decided that an sdk.Coin makes more sense and will be less confusing. Serialization works as expected with this type.

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 and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

chatton · 2023-04-24T15:48:48Z

@mark-rushakoff provided the following insight to help debug the problem. Thank you!

=================================================================

The SDK is using gogo/proto (by way of our fork, https://github.com/cosmos/gogoproto). The TLDR is incompatibilities with gogoproto and official google.golang.org/protobuf.

The one I was running into was just a plain sdk.Coin. It uses a custom type, with the gogoproto.customtype option, to specify sdkmath.Int for the Amount field.

We tell it to serialize on the wire as a string (effectively (*big.Int).String(). But the gogoproto-generated struct still uses a sdkmath.Int field.

I think the correct answer here is:

Define your new message with a string field, not a math.Int customtype
Use plain google.golang.org/protobuf to generate the Go types
Add an “application layer” wrapper struct that contains the protobuf type, that exposes a sdkmath.Int by parsing the underlying protobuf struct’s string field

crodriguezvega

Thank you for the fix, @chatton; and thank you @mark-rushakoff for the insight that got us here!

I need this is in draft yet, but I just left a couple of suggestions. If the test pass, I am good going with this direction.

crodriguezvega · 2023-04-24T19:51:00Z

modules/apps/transfer/keeper/grpc_query.go


 	return &types.QueryTotalEscrowForDenomResponse{
-		Amount: denomAmount,


If we want to return a string, we can just do denomAmount.String(), right?

yeah this would be a bit nicer, I will play with this idea

e2e/testsuite/grpc_query.go

codecov-commenter · 2023-04-25T10:16:08Z

Codecov Report

Merging #3517 (fb063c2) into main (c3b12b1) will decrease coverage by 0.02%.
The diff coverage is 87.50%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3517      +/-   ##
==========================================
- Coverage   78.78%   78.76%   -0.02%     
==========================================
  Files         182      182              
  Lines       12688    12688              
==========================================
- Hits         9996     9994       -2     
- Misses       2261     2262       +1     
- Partials      431      432       +1

Impacted Files	Coverage Δ
modules/apps/transfer/keeper/keeper.go	`87.03% <75.00%> (-1.86%)`	⬇️
modules/apps/transfer/keeper/genesis.go	`92.30% <100.00%> (ø)`
modules/apps/transfer/keeper/grpc_query.go	`73.91% <100.00%> (ø)`
modules/apps/transfer/keeper/migrations.go	`95.45% <100.00%> (ø)`
modules/apps/transfer/keeper/relay.go	`90.90% <100.00%> (ø)`

chatton · 2023-04-27T07:07:43Z

modules/apps/transfer/keeper/keeper_test.go

 			} else {
-				suite.Require().PanicsWithValue("amount cannot be negative: -1", func() {
-					suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(ctx, denom, expAmount)
+				suite.Require().PanicsWithError("negative coin amount: -1", func() {


the panic now happens in the sdk.NewCoin function instead.

if the original behaviour is desirable, we can create a coin literal instead of using the NewCoin function.

proto/ibc/applications/transfer/v1/query.proto

e2e/testsuite/grpc_query.go

modules/apps/transfer/keeper/relay.go

colin-axner · 2023-04-27T16:42:11Z

@damiannolan can you take over this pr next week? 🙏

modules/apps/transfer/keeper/keeper.go

damiannolan

I've addressed the comments left by @colin-axner. PR looks good to me and tests are all passing!

colin-axner

Changes look good. Thanks @chatton and @damiannolan ❤️

colin-axner

Followup ACK, thanks @damiannolan!

* wip: experiement with string value for proto * apply PR suggestions and fix unit tests * added extension method to gRPC response * refactor, change response return type to sdk Coin * rm commented out proto import * remove empty sdk.Coin literal in e2e/grpc_query * reference same type in relay.go in favour of creating new sdk.Coin * adding code comment regarding zero value coin return * fix typo, linter --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit 1006426) # Conflicts: # e2e/tests/transfer/base_test.go # e2e/testsuite/grpc_query.go # modules/apps/transfer/keeper/grpc_query.go # modules/apps/transfer/keeper/keeper.go # modules/apps/transfer/keeper/relay.go

* modify total escrow to take in sdk.Coin in function APIs/proto (#3517) * wip: experiement with string value for proto * apply PR suggestions and fix unit tests * added extension method to gRPC response * refactor, change response return type to sdk Coin * rm commented out proto import * remove empty sdk.Coin literal in e2e/grpc_query * reference same type in relay.go in favour of creating new sdk.Coin * adding code comment regarding zero value coin return * fix typo, linter --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit 1006426) # Conflicts: # e2e/tests/transfer/base_test.go # e2e/testsuite/grpc_query.go # modules/apps/transfer/keeper/grpc_query.go # modules/apps/transfer/keeper/keeper.go # modules/apps/transfer/keeper/relay.go * rm -rf e2e --------- Co-authored-by: Cian Hatton <cian@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com>

wip: experiement with string value for proto

01b343f

chatton mentioned this pull request Apr 24, 2023

Chore: Make a state entry to find total amount of native assets IBC'd out (backport #3019) #3505

Merged

crodriguezvega reviewed Apr 24, 2023

View reviewed changes

crodriguezvega added the backport-to-v7.1.x label Apr 24, 2023

apply PR suggestions and fix unit tests

b1bf9b7

chatton changed the title ~~[WIP] Fix escrow proto E2E~~ Fix escrow proto E2E Apr 25, 2023

chatton marked this pull request as ready for review April 25, 2023 10:08

chatton requested review from colin-axner, AdityaSripal, damiannolan, charleenfei and DimitrisJim as code owners April 25, 2023 10:08

chatton and others added 3 commits April 25, 2023 11:19

added extension method to gRPC response

2e026cd

refactor: change response return type to sdk Coin

9cda050

Merge branch 'main' into escrow-proto-fix

555659a

chatton commented Apr 27, 2023

View reviewed changes

colin-axner reviewed Apr 27, 2023

View reviewed changes

proto/ibc/applications/transfer/v1/query.proto Outdated Show resolved Hide resolved

e2e/testsuite/grpc_query.go Outdated Show resolved Hide resolved

modules/apps/transfer/keeper/relay.go Outdated Show resolved Hide resolved

damiannolan added 4 commits May 1, 2023 19:13

Merge branch 'main' into escrow-proto-fix

8c0dc24

rm commented out proto import

fa096ca

remove empty sdk.Coin literal in e2e/grpc_query

ed5f1e5

reference same type in relay.go in favour of creating new sdk.Coin

102e2de

damiannolan reviewed May 2, 2023

View reviewed changes

modules/apps/transfer/keeper/keeper.go Show resolved Hide resolved

damiannolan approved these changes May 2, 2023

View reviewed changes

colin-axner approved these changes May 3, 2023

View reviewed changes

damiannolan added 3 commits May 3, 2023 13:45

Merge branch 'main' into escrow-proto-fix

15dc59a

adding code comment regarding zero value coin return

b5f831e

Merge branch 'main' into escrow-proto-fix

8a155c7

fix typo, linter

fb063c2

colin-axner approved these changes May 3, 2023

View reviewed changes

colin-axner merged commit 1006426 into main May 3, 2023

colin-axner deleted the escrow-proto-fix branch May 3, 2023 15:02

mergify bot mentioned this pull request May 3, 2023

Fix escrow proto E2E (backport #3517) #3548

Merged

crodriguezvega mentioned this pull request May 14, 2023

Follow up items for #3019 #3368

Closed

7 tasks

faddat mentioned this pull request Jun 23, 2023

v7.2.0 for sdk 50 #3948

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix escrow proto E2E #3517

Fix escrow proto E2E #3517

chatton commented Apr 24, 2023 •

edited

Loading

chatton commented Apr 24, 2023 •

edited

Loading

crodriguezvega left a comment

crodriguezvega Apr 24, 2023

chatton Apr 25, 2023

codecov-commenter commented Apr 25, 2023 •

edited

Loading

chatton Apr 27, 2023

chatton Apr 27, 2023

colin-axner commented Apr 27, 2023

damiannolan left a comment

colin-axner left a comment

colin-axner left a comment


		return &types.QueryTotalEscrowForDenomResponse{
		Amount: denomAmount,

Fix escrow proto E2E #3517

Fix escrow proto E2E #3517

Conversation

chatton commented Apr 24, 2023 • edited Loading

Description

chatton commented Apr 24, 2023 • edited Loading

crodriguezvega left a comment

Choose a reason for hiding this comment

crodriguezvega Apr 24, 2023

Choose a reason for hiding this comment

chatton Apr 25, 2023

Choose a reason for hiding this comment

codecov-commenter commented Apr 25, 2023 • edited Loading

Codecov Report

chatton Apr 27, 2023

Choose a reason for hiding this comment

chatton Apr 27, 2023

Choose a reason for hiding this comment

colin-axner commented Apr 27, 2023

damiannolan left a comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

chatton commented Apr 24, 2023 •

edited

Loading

chatton commented Apr 24, 2023 •

edited

Loading

codecov-commenter commented Apr 25, 2023 •

edited

Loading