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

ICS 20 Cleanup and Tests #5577

Merged
merged 23 commits into from
Feb 20, 2020
Merged

ICS 20 Cleanup and Tests #5577

merged 23 commits into from
Feb 20, 2020

Conversation

jackzampolin
Copy link
Member

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.

@jackzampolin jackzampolin changed the base branch from master to ibc-alpha January 27, 2020 23:57
x/ibc/20-transfer/handler.go Outdated Show resolved Hide resolved
x/ibc/20-transfer/handler.go Show resolved Hide resolved
x/ibc/20-transfer/keeper/relay.go Outdated Show resolved Hide resolved
x/ibc/20-transfer/module.go Outdated Show resolved Hide resolved
x/ibc/20-transfer/module.go Show resolved Hide resolved
@@ -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?
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

cwgoes and others added 2 commits February 3, 2020 12:40
* 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>
@melekes melekes closed this Feb 7, 2020
@fedekunze fedekunze reopened this Feb 7, 2020
@fedekunze fedekunze self-assigned this Feb 18, 2020
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

@cwgoes I'll work on this tomorrow morning

@fedekunze fedekunze changed the title ICS 20 Cleanup and Questions ICS 20 Cleanup and Tests Feb 19, 2020
@jackzampolin
Copy link
Member Author

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 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
proof = commitment.Proof{
Proof: res.Proof,
if tc.expPass {
suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

x/ibc/20-transfer/keeper/relay_test.go Show resolved Hide resolved
x/ibc/20-transfer/keeper/relay_test.go Show resolved Hide resolved
@cwgoes
Copy link
Contributor

cwgoes commented Feb 19, 2020

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.

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
cwgoes and others added 3 commits February 20, 2020 16:40
Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
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 fedekunze added R4R and removed WIP labels Feb 20, 2020
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

tACK - thanks for the excellent comments & testcases!

@fedekunze fedekunze merged commit 6f42d82 into ibc-alpha Feb 20, 2020
@fedekunze fedekunze deleted the jack/ics-20 branch February 20, 2020 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants