ICS 20 Cleanup and Tests #5577

jackzampolin · 2020-01-27T23:17:17Z

Ok I took a run through the current ICS 20 implementation while reading the current spec. It looks like there is some work required to get this code functional but unsure how much I've added a few TODOs and other comments here to get started on that work.

… the spec is being implemented

x/ibc/20-transfer/handler.go

x/ibc/20-transfer/keeper/relay.go

x/ibc/20-transfer/module.go

x/ibc/20-transfer/types/packet.go

fedekunze · 2020-01-28T08:04:48Z

x/ibc/20-transfer/genesis.go

@@ -10,6 +10,7 @@ import (
 // InitGenesis sets distribution information for genesis
 func InitGenesis(ctx sdk.Context, keeper Keeper) {
 	// check if the module account exists
+	// TODO: should we create the module account if it doesn't exist?


the desired logic is to panic the application if the module account is not initialized on app.go

… jack/ics-20

x/ibc/20-transfer/types/msgs.go

x/ibc/20-transfer/keeper/keeper.go

x/ibc/20-transfer/keeper/keeper_test.go

* Simulation docs (#5033) * simulation docs * update docs with the latest simulation changes * minor imporvments * clean up of simulation.md * expand section on weights * minor reword * minor wording fix Co-authored-by: Marko <marbar3778@yahoo.com> * Merge PR #5597: Include Amount in Complete Unbonding/Redelegation Events * Add bank alias for gaia * Moar bank alias gaia * Moar bank alias gaia * Call `TimeoutExecuted`, add wrappers * Remove unused `MsgRecvPacket` Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Jack Zampolin <jack.zampolin@gmail.com>

… jack/ics-20

cwgoes

I think the substantive changes in the PR (acknowledgement interface, ICS 20 fixes) are fine, but more tests would be nice, especially end-to-end tests of the ICS 20 Handler for transfer messages, packet receipt, and packet timeout.

fedekunze · 2020-02-18T22:27:22Z

@cwgoes I'll work on this tomorrow morning

jackzampolin · 2020-02-19T21:24:12Z

One failing test case left:

--- FAIL: TestHandlerTestSuite (0.01s)
    --- FAIL: TestHandlerTestSuite/TestHandleMsgTransfer (0.01s)
        suite.go:62: test panicked: invalid denom: bank/firstchannel/testportid/secondchannel/atom
            goroutine 9 [running]:
            runtime/debug.Stack(0xc000b8a040, 0x11812c0, 0xc000bf8500)
            	/usr/local/go/src/runtime/debug/stack.go:24 +0x9d
            github.com/stretchr/testify/suite.failOnPanic(0xc000434000)
            	/home/jackzampolin/go/pkg/mod/github.com/stretchr/testify@v1.4.0/suite/suite.go:62 +0x57
            panic(0x11812c0, 0xc000bf8500)
            	/usr/local/go/src/runtime/panic.go:679 +0x1b2
            github.com/cosmos/cosmos-sdk/types.NewCoin(...)
            	/home/jackzampolin/go/src/github.com/cosmos/cosmos-sdk/types/coin.go:18
            github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/keeper.Keeper.SendTransfer(0x16b3de0, 0xc000b133e0, 0xc0003188c0, 0x16b3da0, 0xc000b13440, 0x0, 0x0, 0x0, 0x0, 0x16ce5a0, ...)
            	/home/jackzampolin/go/src/github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/keeper/relay.go:46 +0x722
            github.com/cosmos/cosmos-sdk/x/ibc/20-transfer.handleMsgTransfer(0x16c5500, 0xc0000bc010, 0x16da4c0, 0xc00009fd80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
            	/home/jackzampolin/go/src/github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/handler.go:38 +0x172
            github.com/cosmos/cosmos-sdk/x/ibc/20-transfer.NewHandler.func1(0x16c5500, 0xc0000bc010, 0x16da4c0, 0xc00009fd80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
            	/home/jackzampolin/go/src/github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/handler.go:15 +0x7fc
            github.com/cosmos/cosmos-sdk/x/ibc/20-transfer_test.(*HandlerTestSuite).TestHandleMsgTransfer(0xc0000ef080)
            	/home/jackzampolin/go/src/github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/handler_test.go:194 +0xd37
            reflect.Value.call(0xc000def800, 0xc0000bed08, 0x13, 0x131a28d, 0x4, 0xc000b8def8, 0x1, 0x1, 0xc000b8dd40, 0x40c0ca, ...)
            	/usr/local/go/src/reflect/value.go:460 +0x5f6
            reflect.Value.Call(0xc000def800, 0xc0000bed08, 0x13, 0xc000b8def8, 0x1, 0x1, 0x44d247, 0x184a218, 0x1f87a40)
            	/usr/local/go/src/reflect/value.go:321 +0xb4
            github.com/stretchr/testify/suite.Run.func2(0xc000434000)
            	/home/jackzampolin/go/pkg/mod/github.com/stretchr/testify@v1.4.0/suite/suite.go:133 +0x31f
            testing.tRunner(0xc000434000, 0xc000df4a80)
            	/usr/local/go/src/testing/testing.go:909 +0xc9
            created by testing.(*T).Run
            	/usr/local/go/src/testing/testing.go:960 +0x350
FAIL

x/ibc/20-transfer/keeper/relay_test.go

golangcibot · 2020-02-19T21:28:40Z

x/ibc/20-transfer/keeper/relay_test.go

-	proof = commitment.Proof{
-		Proof: res.Proof,
+			if tc.expPass {
+				suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg)


Using the variable on range scope i in function literal (from scopelint)

x/ibc/20-transfer/keeper/relay_test.go

cwgoes · 2020-02-19T22:15:35Z

One failing test case left:

--- FAIL: TestHandlerTestSuite (0.01s)
    --- FAIL: TestHandlerTestSuite/TestHandleMsgTransfer (0.01s)
        suite.go:62: test panicked: invalid denom: bank/firstchannel/testportid/secondchannel/atom
            goroutine 9 [running]:
            runtime/debug.Stack(0xc000b8a040, 0x11812c0, 0xc000bf8500)
            	/usr/local/go/src/runtime/debug/stack.go:24 +0x9d
            github.com/stretchr/testify/suite.failOnPanic(0xc000434000)
            	/home/jackzampolin/go/pkg/mod/github.com/stretchr/testify@v1.4.0/suite/suite.go:62 +0x57
            panic(0x11812c0, 0xc000bf8500)
            	/usr/local/go/src/runtime/panic.go:679 +0x1b2
            github.com/cosmos/cosmos-sdk/types.NewCoin(...)
            	/home/jackzampolin/go/src/github.com/cosmos/cosmos-sdk/types/coin.go:18
            github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/keeper.Keeper.SendTransfer(0x16b3de0, 0xc000b133e0, 0xc0003188c0, 0x16b3da0, 0xc000b13440, 0x0, 0x0, 0x0, 0x0, 0x16ce5a0, ...)
            	/home/jackzampolin/go/src/github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/keeper/relay.go:46 +0x722
            github.com/cosmos/cosmos-sdk/x/ibc/20-transfer.handleMsgTransfer(0x16c5500, 0xc0000bc010, 0x16da4c0, 0xc00009fd80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
            	/home/jackzampolin/go/src/github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/handler.go:38 +0x172
            github.com/cosmos/cosmos-sdk/x/ibc/20-transfer.NewHandler.func1(0x16c5500, 0xc0000bc010, 0x16da4c0, 0xc00009fd80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
            	/home/jackzampolin/go/src/github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/handler.go:15 +0x7fc
            github.com/cosmos/cosmos-sdk/x/ibc/20-transfer_test.(*HandlerTestSuite).TestHandleMsgTransfer(0xc0000ef080)
            	/home/jackzampolin/go/src/github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/handler_test.go:194 +0xd37
            reflect.Value.call(0xc000def800, 0xc0000bed08, 0x13, 0x131a28d, 0x4, 0xc000b8def8, 0x1, 0x1, 0xc000b8dd40, 0x40c0ca, ...)
            	/usr/local/go/src/reflect/value.go:460 +0x5f6
            reflect.Value.Call(0xc000def800, 0xc0000bed08, 0x13, 0xc000b8def8, 0x1, 0x1, 0x44d247, 0x184a218, 0x1f87a40)
            	/usr/local/go/src/reflect/value.go:321 +0xb4
            github.com/stretchr/testify/suite.Run.func2(0xc000434000)
            	/home/jackzampolin/go/pkg/mod/github.com/stretchr/testify@v1.4.0/suite/suite.go:133 +0x31f
            testing.tRunner(0xc000434000, 0xc000df4a80)
            	/usr/local/go/src/testing/testing.go:909 +0xc9
            created by testing.(*T).Run
            	/usr/local/go/src/testing/testing.go:960 +0x350
FAIL

I believe this is failing because the denomination is too long - per types/coin.go, denominations can only be up to 32 characters long. We might want to relax this restriction a bit and allow longer denoms - it used to be more of an issue since they were stored in the sdk.Coin object, but now that we've split out store keys for each denom it matters less. Still, we'll need to have some length limit, and we'll probably need to check in ICS 20 that ports / channels won't create denoms which exceed that limit.

… jack/ics-20

x/ibc/20-transfer/keeper/relay.go

x/ibc/20-transfer/keeper/relay_test.go

x/ibc/20-transfer/keeper/relay.go

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>

x/ibc/20-transfer/keeper/relay.go

x/ibc/20-transfer/keeper/relay_test.go

cwgoes

tACK - thanks for the excellent comments & testcases!

Add comments, remove unused code and attempt to point to places where…

d3ee967

… the spec is being implemented

jackzampolin requested review from alessio, alexanderbez, fedekunze and rigelrozanski as code owners January 27, 2020 23:17

jackzampolin changed the base branch from master to ibc-alpha January 27, 2020 23:57

dtribble reviewed Jan 28, 2020

View reviewed changes

fedekunze reviewed Jan 28, 2020

View reviewed changes

x/ibc/20-transfer/types/packet.go Show resolved Hide resolved

fedekunze reviewed Jan 28, 2020

View reviewed changes

x/ibc/20-transfer/types/packet.go Outdated Show resolved Hide resolved

fedekunze reviewed Jan 28, 2020

View reviewed changes

Merge branch 'ibc-alpha' of https://github.com/cosmos/cosmos-sdk into…

6adb6ed

… jack/ics-20

golangcibot reviewed Jan 31, 2020

View reviewed changes

x/ibc/20-transfer/types/msgs.go Outdated Show resolved Hide resolved

close channel when transfer fails

c6e2321

golangcibot reviewed Jan 31, 2020

View reviewed changes

x/ibc/20-transfer/keeper/keeper.go Outdated Show resolved Hide resolved

rename packer data transfer to align to spec; refactor table tests

965440f

golangcibot reviewed Feb 3, 2020

View reviewed changes

x/ibc/20-transfer/keeper/keeper_test.go Show resolved Hide resolved

x/ibc/20-transfer/keeper/keeper_test.go Show resolved Hide resolved

cwgoes and others added 2 commits February 3, 2020 12:40

Merge branch 'ibc-alpha' into jack/ics-20

4ddaf28

melekes closed this Feb 7, 2020

fedekunze reopened this Feb 7, 2020

fedekunze added 2 commits February 18, 2020 15:39

merge ibc-alpha

f068bf8

Merge branch 'ibc-alpha' of https://github.com/cosmos/cosmos-sdk into…

61b7eee

… jack/ics-20

fedekunze self-assigned this Feb 18, 2020

Merge PR #5603: Remove acknowledgement interface in favour of []byte

7dfd6b9

cwgoes reviewed Feb 18, 2020

View reviewed changes

fedekunze added 2 commits February 19, 2020 10:49

fixes and cleanup

6f5ad5c

ibc alpha changes

f9fcce8

fedekunze changed the title ~~ICS 20 Cleanup and Questions~~ ICS 20 Cleanup and Tests Feb 19, 2020

fedekunze added the T: Tests label Feb 19, 2020

fedekunze added WIP labels Feb 19, 2020

fedekunze added 2 commits February 19, 2020 11:50

spec compliance

f56fd31

refactor relay prefixes and tests

1068bb5

Fix test compilation

37436b1

golangcibot reviewed Feb 19, 2020

View reviewed changes

fedekunze added 2 commits February 20, 2020 12:24

Merge branch 'ibc-alpha' of https://github.com/cosmos/cosmos-sdk into…

dac12e8

… jack/ics-20

cleanup; add notes and additional test case

05b3e48

golangcibot reviewed Feb 20, 2020

View reviewed changes

x/ibc/20-transfer/keeper/relay.go Outdated Show resolved Hide resolved

x/ibc/20-transfer/keeper/relay.go Outdated Show resolved Hide resolved

x/ibc/20-transfer/keeper/relay.go Outdated Show resolved Hide resolved

x/ibc/20-transfer/keeper/relay_test.go Outdated Show resolved Hide resolved

Receive transfer test

4f9763e

golangcibot reviewed Feb 20, 2020

View reviewed changes

x/ibc/20-transfer/keeper/relay_test.go Outdated Show resolved Hide resolved

cwgoes reviewed Feb 20, 2020

View reviewed changes

x/ibc/20-transfer/keeper/relay.go Outdated Show resolved Hide resolved

cwgoes and others added 3 commits February 20, 2020 16:40

Apply suggestions from code review

4db79ba

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>

Fix autolinter application

e8cca15

Add testcase with incorrect prefix

4d14d36

golangcibot reviewed Feb 20, 2020

View reviewed changes

x/ibc/20-transfer/keeper/relay.go Outdated Show resolved Hide resolved

x/ibc/20-transfer/keeper/relay_test.go Outdated Show resolved Hide resolved

x/ibc/20-transfer/keeper/relay_test.go Outdated Show resolved Hide resolved

x/ibc/20-transfer/keeper/relay_test.go Show resolved Hide resolved

fedekunze added 3 commits February 20, 2020 17:00

golangcibot fixes

b31d01b

timeout test

06fa17c

delete extra comment

74815b6

fedekunze added R4R and removed WIP labels Feb 20, 2020

cwgoes approved these changes Feb 20, 2020

View reviewed changes

fedekunze merged commit 6f42d82 into ibc-alpha Feb 20, 2020

fedekunze deleted the jack/ics-20 branch February 20, 2020 16:31

This was referenced Feb 20, 2020

Split out interfaces for acknowledgements #5509

Closed

Fix timeout execution handling #5511

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICS 20 Cleanup and Tests #5577

ICS 20 Cleanup and Tests #5577

jackzampolin commented Jan 27, 2020

fedekunze Jan 28, 2020

cwgoes left a comment

fedekunze commented Feb 18, 2020

jackzampolin commented Feb 19, 2020

golangcibot Feb 19, 2020

cwgoes commented Feb 19, 2020

cwgoes left a comment

ICS 20 Cleanup and Tests #5577

ICS 20 Cleanup and Tests #5577

Conversation

jackzampolin commented Jan 27, 2020

fedekunze Jan 28, 2020

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

fedekunze commented Feb 18, 2020

jackzampolin commented Feb 19, 2020

golangcibot Feb 19, 2020

Choose a reason for hiding this comment

cwgoes commented Feb 19, 2020

cwgoes left a comment

Choose a reason for hiding this comment