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: apply the ibc module of lbm-sdk #1

Merged
merged 14 commits into from
Jan 19, 2023
Merged

feat: apply the ibc module of lbm-sdk #1

merged 14 commits into from
Jan 19, 2023

Conversation

dudong2
Copy link

@dudong2 dudong2 commented Jan 11, 2023

Description

Apply all the ibc modules changes of lbm-sdk.
The x/ibc module is copied from the 0fbc2fcae6dba90fa80b815cf3219d6fcf46fc64 commit hash in lbm-sdk.

closes: #859

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

@dudong2 dudong2 added the enhancement New feature or request label Jan 11, 2023
@dudong2 dudong2 self-assigned this Jan 11, 2023
@dudong2 dudong2 marked this pull request as draft January 11, 2023 05:54
@dudong2 dudong2 marked this pull request as ready for review January 17, 2023 00:23
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.

Please add tendermint proto. It is necessary for communicate with other cosmos chain.

@dudong2 dudong2 requested a review from zemyblue January 17, 2023 08:32
@zemyblue
Copy link
Member

Please add tendermint proto. It is necessary for communicate with other cosmos chain.

I meant the tendermint proto file in the ibc/lightclients. :)
But I think it may need to many changes.
Please add it via another PR.


module github.com/cosmos/ibc-go/v3
Copy link
Member

Choose a reason for hiding this comment

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

Is it no problem to remove /v3 path?

Copy link
Author

Choose a reason for hiding this comment

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

It's just a specifier indicating the path and version for that module. Also, for line/ibc-go, I thought that v3 had no meaning, so I removed it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it has some reason, because each version of cosmos/ibc-go has each module path.
https://github.com/cosmos/ibc-go/blob/8f0cfb0682bb6dc6cf311d4644b300041e277866/go.mod#L3

docs/versions Outdated
Comment on lines 1 to 4
release/v2.0.x v2.0.0
release/v1.2.x v1.2.0
release/v1.1.x v1.1.0
main main
Copy link
Member

Choose a reason for hiding this comment

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

Is it no need?

Copy link
Author

Choose a reason for hiding this comment

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

This file is the latest patch versions for the currently maintained minor versions. The line/ibc-go will continue to follow a specific version of ibc-go, so i decided that the doc specifying the version of cosmos/ibc-go is not necessary. If it is thought that there is a need to manage multiple minor versions of line/ibc-go in the future, it would be a good idea to add it then.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is necessary if we wan to communicate with other cosmos chain.

@dudong2
Copy link
Author

dudong2 commented Jan 17, 2023

Please add tendermint proto. It is necessary for communicate with other cosmos chain.

I meant the tendermint proto file in the ibc/lightclients. :) But I think it may need to many changes. Please add it via another PR.

Because all the parts corresponding to ibc/light-client/tendermint have been changed to ostracon, adding tendermint seems to change a lot. After bump up to v3.3.1, i will proceed with adding tendermint.

@dudong2 dudong2 requested a review from zemyblue January 18, 2023 04:44
@ulbqb ulbqb self-requested a review January 19, 2023 01:49
@ulbqb ulbqb self-requested a review January 19, 2023 06:09
Copy link
Member

@ulbqb ulbqb left a comment

Choose a reason for hiding this comment

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

LGTM

@dudong2 dudong2 merged commit c366ca9 into Finschia:main Jan 19, 2023
@dudong2 dudong2 deleted the dudong2/feat/detach-ibc branch January 19, 2023 06:51
Copy link

@0Tech 0Tech left a comment

Choose a reason for hiding this comment

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

Minor comments.

What problems may be addressed by introducing this feature?
What benefits does the ibc-go stand to gain by including this feature?
What benefits does the SDK stand to gain by including this feature?
Copy link

Choose a reason for hiding this comment

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

It would be better to revert the change.

Suggested change
What benefits does the SDK stand to gain by including this feature?
What benefits does the ibc-go stand to gain by including this feature?

Comment on lines -232 to +233
var cache sdk.MultiStorePersistentCache
var cccc sdk.MultiStorePersistentCache // TODO(dudong2): change cccc -> ??
Copy link

Choose a reason for hiding this comment

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

Are you done here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

detach ibc module
5 participants