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.

Sign up for GitHub

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

Jump to bottom

docs: adding migration doc for ibc-go v6 #2417

Merged
merged 30 commits into from
Oct 14, 2022
Merged

docs: adding migration doc for ibc-go v6 #2417

merged 30 commits into from
Oct 14, 2022

Conversation

damiannolan
Copy link
Member

Description

  • Adding migration doc for ibc-go/v6

closes: #2184

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.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@damiannolan damiannolan mentioned this pull request Sep 28, 2022
Merged
9 tasks
@damiannolan damiannolan mentioned this pull request Sep 28, 2022
Merged
9 tasks
@damiannolan damiannolan marked this pull request as ready for review September 30, 2022 12:39
damiannolan
damiannolan commented Sep 30, 2022
View reviewed changes
docs/migrations/v5-to-v6.md
These functions have been deprecated in favour of the new `controller` submodule `MsgServer` and will be removed in later releases.
Both APIs remain functional and maintain backwards compatibility in `ibc-go/v6`, however consumers of these APIs are now recommended to follow the message passing paradigm outlined in Cosmos SDK [ADR 031](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-031-msg-service.md) and [ADR 033](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-033-protobuf-inter-module-comm.md). This is facilitated by the Cosmos SDK [`MsgServiceRouter`](https://github.com/cosmos/cosmos-sdk/blob/main/baseapp/msg_service_router.go#L17) and chain developers creating custom application logic can now omit the ICS27 `ControllerKeeper` from their module and instead depend on message routing.

For more information see the new [ICS27 integration documentation](TODO: add link to new docs).
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll put together some new docs for message routing examples for MsgRegisterInterchainAccount and MsgSendTx.

chatton
chatton reviewed Sep 30, 2022
View reviewed changes
Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

Really clear write up, nice job! 🥇

docs/migrations/v5-to-v6.md Show resolved Hide resolved
colin-axner
colin-axner approved these changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Great work :) Really nice migration docs!

docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
crodriguezvega
crodriguezvega reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Awesome docs, @damiannolan!

I think we need to add some information about the following API breaking PRs: #2395, #2035, #2034, #2025, #2024, #2433, #2446 and maybe also for the changes I mention here.

Sorry, I know that this is still a lot of work, so happy to help here.

docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md
These functions have been deprecated in favour of the new `controller` submodule `MsgServer` and will be removed in later releases.
Both APIs remain functional and maintain backwards compatibility in `ibc-go/v6`, however consumers of these APIs are now recommended to follow the message passing paradigm outlined in Cosmos SDK [ADR 031](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-031-msg-service.md) and [ADR 033](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-033-protobuf-inter-module-comm.md). This is facilitated by the Cosmos SDK [`MsgServiceRouter`](https://github.com/cosmos/cosmos-sdk/blob/main/baseapp/msg_service_router.go#L17) and chain developers creating custom application logic can now omit the ICS27 `ControllerKeeper` from their module and instead depend on message routing.

Legacy APIs are still required if application developers wish to consume IBC packet callbacks and react upon packet acknowledgements. In future this will be replaced by IBC Actor Callbacks, see [ADR 008](https://github.com/cosmos/ibc-go/pull/1976).
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to get it merged, so that we can use a relative link...

docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
docs/migrations/v5-to-v6.md Outdated Show resolved Hide resolved
@damiannolan
Copy link
Member Author

damiannolan commented Oct 14, 2022 •
edited
Loading

Thanks for the detailed review @crodriguezvega .

I think we need to add some information about the following API breaking PRs: #2395, #2035, #2034, #2025, #2024, #2433, #2446 and maybe also for the changes I mention here.

I've documented changes in #2395 and #2446 as they effect end users. I've omitted adding info for the ScopedKeeper changes as we discussed it may add unnecessary noise - this won't cause any compiler to complain as most, if not all users are using the standard x/capability module. I've also omitted the change of PortPrefix -> ControllerPortPrefix and Port to HostPort as these const are likely only used within 27-interchain-accounts.
Migration docs from #1703 have been pulled in.

@damiannolan damiannolan enabled auto-merge (squash) October 14, 2022 16:23
@damiannolan damiannolan merged commit 90b051e into main Oct 14, 2022
@damiannolan damiannolan deleted the damian/2184-migration-docs-v6 branch October 14, 2022 16:26
mergify bot pushed a commit that referenced this pull request Oct 14, 2022
@damiannolan @mergify
docs: adding migration doc for ibc-go v6 (#2417)
fea0cff 
* WIP ics27 v6 migration doc

* updating migration doc

* updating migration doc

* updating migration doc

* updating migration doc

* updating migration doc

* updating migration doc

* updating migration doc

* readding migration doc after merge nuked my file -_-

* updating migration doc with ics27 host params updates

* cleanup

* adding controller api deprecation to migration doc

* updating links

* updating formatting

* formatting

* moving upgrade handler and migration details to chains section

* Apply suggestions from code review

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* mitigate copy pasta by breaking code snippet compilation

* added note about legacy APIs for packet cbs and ADR 008 ref

* Apply suggestions from code review

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

* removing backticks in all refs to ibc-go, controller and host

* addressing indentation

* adding ics29 NewKeeper api breaking changes and removal of ics20  SendTransfer

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
(cherry picked from commit 90b051e)
@mergify mergify bot mentioned this pull request Oct 14, 2022
Merged
damiannolan added a commit that referenced this pull request Oct 19, 2022
@mergify @damiannolan
docs: adding migration doc for ibc-go v6 (#2417) (#2553)
19fb37c 
* WIP ics27 v6 migration doc

* updating migration doc

* updating migration doc

* updating migration doc

* updating migration doc

* updating migration doc

* updating migration doc

* updating migration doc

* readding migration doc after merge nuked my file -_-

* updating migration doc with ics27 host params updates

* cleanup

* adding controller api deprecation to migration doc

* updating links

* updating formatting

* formatting

* moving upgrade handler and migration details to chains section

* Apply suggestions from code review

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* mitigate copy pasta by breaking code snippet compilation

* added note about legacy APIs for packet cbs and ADR 008 ref

* Apply suggestions from code review

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

* removing backticks in all refs to ibc-go, controller and host

* addressing indentation

* adding ics29 NewKeeper api breaking changes and removal of ics20  SendTransfer

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
(cherry picked from commit 90b051e)

Co-authored-by: Damian Nolan <damiannolan@gmail.com>
@chatton chatton mentioned this pull request Nov 7, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crodriguezvega crodriguezvega crodriguezvega left review comments

@chatton chatton chatton left review comments

@charleenfei charleenfei charleenfei approved these changes

@colin-axner colin-axner colin-axner approved these changes

@AdityaSripal AdityaSripal Awaiting requested review from AdityaSripal AdityaSripal is a code owner

@seantking seantking Awaiting requested review from seantking

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add migration docs for ibc-go v5 -> v6
5 participants
@damiannolan @crodriguezvega @chatton @charleenfei @colin-axner