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

chore: adding assertion of channel capabilities migration #2140

Merged
merged 22 commits into from
Sep 7, 2022

Conversation

damiannolan
Copy link
Member

Description

closes: #XXXX


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
Copy link
Member Author

Opening initially as draft. Happy path tests on this require #2033.

Base automatically changed from damian/ics27-chan-capability-migrations to main August 31, 2022 10:42

// AssertChannelCapabilityMigrations checks that all channel capabilities generated using the interchain accounts controller port prefix
// are owned by the controller submodule and ibc.
func (m Migrator) AssertChannelCapabilityMigrations(ctx sdk.Context) error {
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'm open to suggestions on improving the name of this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the name

@damiannolan damiannolan marked this pull request as ready for review September 1, 2022 09:09
@codecov-commenter
Copy link

Codecov Report

Merging #2140 (0bd27e9) into main (9019adf) will decrease coverage by 0.01%.
The diff coverage is 73.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2140      +/-   ##
==========================================
- Coverage   79.48%   79.47%   -0.02%     
==========================================
  Files         173      174       +1     
  Lines       12011    12029      +18     
==========================================
+ Hits         9547     9560      +13     
- Misses       2042     2045       +3     
- Partials      422      424       +2     
Impacted Files Coverage Δ
modules/apps/27-interchain-accounts/module.go 51.57% <60.00%> (-0.07%) ⬇️
...nterchain-accounts/controller/keeper/migrations.go 78.57% <78.57%> (ø)

@damiannolan
Copy link
Member Author

I believe we also need to call SetMiddlewareEnabled() for all active channels we iterate over in this migration handler.
Any existing channels will need this set to continue to operate with the underlying app callbacks.

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.

Excellent work. Super clean! Could you add a changelog entry?

@colin-axner
Copy link
Contributor

I believe we also need to call SetMiddlewareEnabled() for all active channels we iterate over in this migration handler. Any existing channels will need this set to continue to operate with the underlying app callbacks.

Makes sense to me. And if the chain removes the underlying app entirely, those middleware enabled checks won't be reached since the underlyingApp != nil check comes first

@damiannolan damiannolan self-assigned this Sep 6, 2022
@damiannolan damiannolan enabled auto-merge (squash) September 7, 2022 17:39
@damiannolan damiannolan merged commit 8d56b94 into main Sep 7, 2022
@damiannolan damiannolan deleted the damian/ics27-assert-chan-capabilities branch September 7, 2022 18:09
mergify bot pushed a commit that referenced this pull request Sep 7, 2022
* wip initial commit

* draft migration completed

* removing unnecessary storekey arg

* additional cleanup

* adding updates to migrations and tests additional assertions

* updating and moving migrations code

* adding additional checks to tests

* cleaning up tests

* cleaning up tests

* updating inline doc comments, checking err return

* using InitMemStore in favour of InitializeCapability, adjusting tests

* adding assertion of channel capabilities migration

* adapting migration code to use get/authenticate capability

* adding changelog

* updating tests

* set middleware enabled for existing channels

* assert middleware enabled in tests

* updating changelog with middleware enabled flag

(cherry picked from commit 8d56b94)

# Conflicts:
#	CHANGELOG.md
damiannolan added a commit that referenced this pull request Sep 8, 2022
…2140) (#2219)

* chore: adding assertion of channel capabilities migration (#2140)

* wip initial commit

* draft migration completed

* removing unnecessary storekey arg

* additional cleanup

* adding updates to migrations and tests additional assertions

* updating and moving migrations code

* adding additional checks to tests

* cleaning up tests

* cleaning up tests

* updating inline doc comments, checking err return

* using InitMemStore in favour of InitializeCapability, adjusting tests

* adding assertion of channel capabilities migration

* adapting migration code to use get/authenticate capability

* adding changelog

* updating tests

* set middleware enabled for existing channels

* assert middleware enabled in tests

* updating changelog with middleware enabled flag

(cherry picked from commit 8d56b94)

# Conflicts:
#	CHANGELOG.md

* resolving conflicts

Co-authored-by: Damian Nolan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo 🏃
Development

Successfully merging this pull request may close these issues.

5 participants