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

feat: update x/ibc to support github.com/cosmos/ibc-go@v3.0.0 #651

Merged
merged 34 commits into from
Sep 19, 2022

Conversation

ulbqb
Copy link
Member

@ulbqb ulbqb commented Aug 26, 2022

Description

This PR enable to support github.com/cosmos/ibc-go@v3.0.0.

The directory structure between x/ibc and ibc-go is different. The directories correspond as follows.

x/ibc ibc-go
x/ibc/applications modules/apps
x/ibc/core modules/core
x/ibc/light-clients modules/light-clients
x/ibc/testing testing

This PR contains the following changes:

  • Update x/ibc to github.com/cosmos/ibc-go@v3.0.0
  • Fix x/wasm to support updated x/ibc
  • Merge github.com/cosmos/ibc-go/testing/simapp and github.com/CosmWasm/wasmd/app into github.com/line/lbm-sdk/simapp

This PR contains the following lbm-sdk specific changes:

  • Merge github.com/cosmos/ibc-go/testing/simapp and github.com/CosmWasm/wasmd/app into github.com/line/lbm-sdk/simapp
  • Change this test to support x/bankplus/keeper.SendCoinsFromModuleToAccount that does not have blacklist check (commit: acb3a9d)
  • Change github.com/cosmos/ibc-go to support ostracon (commit: c57f8e3)
  • Fix swagger-combine config to merge github.com/cosmos/ibc-go/proto into lbm-sdk/proto (commit: ce9e58b)
  • Not use the inter-tx module in simapp and there are the following notes regarding inter-tx.
    https://github.com/CosmWasm/wasmd/blob/v0.27.0/app/app.go#L96-L100

    // Note: please do your research before using this in production app, this is a demo and not an officially
    // supported IBC team implementation. It has no known issues, but do your own research before using it.
    intertx "github.com/cosmos/interchain-accounts/x/inter-tx"
    intertxkeeper "github.com/cosmos/interchain-accounts/x/inter-tx/keeper"
    intertxtypes "github.com/cosmos/interchain-accounts/x/inter-tx/types"

closes: #585

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@ulbqb ulbqb marked this pull request as draft August 26, 2022 06:16
@ulbqb ulbqb requested review from zemyblue, Kynea0b and tnasu August 26, 2022 08:31
@ulbqb ulbqb self-assigned this Aug 26, 2022
@ulbqb ulbqb requested review from 0Tech and dudong2 August 26, 2022 08:34
@ulbqb ulbqb changed the title feat: update lbm-sdk/x/ibc to support cosmos/ibc-go/v3 feat: update x/ibc to support github.com/cosmos/ibc-go/v3 Aug 29, 2022
@ulbqb ulbqb changed the title feat: update x/ibc to support github.com/cosmos/ibc-go/v3 feat: update x/ibc to support github.com/cosmos/ibc-go@v3.0.0 Aug 29, 2022
@ulbqb ulbqb requested a review from shiki-tak August 29, 2022 08:28
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

@ulbqb
Copy link
Member Author

ulbqb commented Sep 14, 2022

@ulbqb ulbqb requested a review from zemyblue September 14, 2022 02:48
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

simapp/app.go Show resolved Hide resolved
}

func TestIBCTestSuite(t *testing.T) {
suite.Run(t, new(IBCTestSuite))
}

func (suite *IBCTestSuite) TestValidateGenesis() {
header := suite.chainA.CreateOCClientHeader(suite.chainA.ChainID, suite.chainA.CurrentHeader.Height, clienttypes.NewHeight(0, uint64(suite.chainA.CurrentHeader.Height-1)), suite.chainA.CurrentHeader.Time, suite.chainA.Vals, suite.chainA.Vals, suite.chainA.Voters, suite.chainA.Voters, suite.chainA.Signers)
header := suite.chainA.CreateTMClientHeader(suite.chainA.ChainID, suite.chainA.CurrentHeader.Height, clienttypes.NewHeight(0, uint64(suite.chainA.CurrentHeader.Height-1)), suite.chainA.CurrentHeader.Time, suite.chainA.Vals, suite.chainA.Vals, suite.chainA.Voters, suite.chainA.Voters, suite.chainA.Signers)
Copy link
Member

Choose a reason for hiding this comment

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

This logic can communicate with other cosmos-sdk ibc modules?

Copy link
Member Author

@ulbqb ulbqb Sep 15, 2022

Choose a reason for hiding this comment

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

Are you asking if the name was changed on purpose? This fix was unintentional. I fixed it.
This logic is not related with cosmos-sdk.

Copy link
Member

Choose a reason for hiding this comment

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

No, I just wanted to know this x/ibc module can communicate with other cosmos-sdk base chains. Is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be a big task, so I'll tackle it as a separate issue. I created the issue as #676.

x/wasm/ibc.go Show resolved Hide resolved
x/wasm/keeper/relay.go Outdated Show resolved Hide resolved
@tnasu
Copy link
Member

tnasu commented Sep 15, 2022

@tnasu
I wondered if I should change the variable name and type name from tendermint to ostracon. I thought it would be better not to change to ostracon, because when comparing with the original, there will be a mix of non-important differences and important logic differences. What do you think?

I agree with your concerns/meanings. But I know that lbm-sdk uses Ostracon/ostracon/oc in the codes by us:

Okay, then, let's separates into other PR.

@tnasu
Copy link
Member

tnasu commented Sep 15, 2022

@tnasu

Could you leave about this in the description? https://github.com/CosmWasm/wasmd/blob/main/app/app.go#L96-L100

	// Note: please do your research before using this in production app, this is a demo and not an officially
	// supported IBC team implementation. It has no known issues, but do your own research before using it.
	intertx "github.com/cosmos/interchain-accounts/x/inter-tx"
	intertxkeeper "github.com/cosmos/interchain-accounts/x/inter-tx/keeper"
	intertxtypes "github.com/cosmos/interchain-accounts/x/inter-tx/types"

Currently this PR does not contain inter-tx module.

I mean that I'd like to update the PR description about inter-tx module.

@ulbqb
Copy link
Member Author

ulbqb commented Sep 15, 2022

I agree with your concerns/meanings. But I know that lbm-sdk uses Ostracon/ostracon/oc in the codes by us:

Okay, then, let's separates into other PR.

@tnasu
I created the issue #674.

@ulbqb
Copy link
Member Author

ulbqb commented Sep 15, 2022

Please check https://github.com/line/lbm-sdk/pull/651/files#diff-44578a53fec2e61d283949550afd5632581e30744b7dc6810fbd4e103b375d67R42.

@zemyblue
That link does not work. Maybe, it points to these.
https://github.com/line/lbm-sdk/blob/5dbd91d4b37f0dd933c9bb4f901887611f3c34f3/x/wasm/keeper/relay_test.go#L37
https://github.com/line/lbm-sdk/blob/5dbd91d4b37f0dd933c9bb4f901887611f3c34f3/x/wasm/keeper/relay_test.go#L42

There is no problem as it is, but the FIXME can be eliminate by setting the correct storage gas value. So, I fixed it in 38f772f.

Copy link
Contributor

@dudong2 dudong2 left a comment

Choose a reason for hiding this comment

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

LGTM

@ulbqb ulbqb requested a review from zemyblue September 16, 2022 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide whether to import x/ibc from ibc-go and use it or update it.
5 participants