chore: ics27 channel capability migrations #2134

damiannolan · 2022-08-26T12:20:39Z

Description

Adding upgrade handler for ICS27 channel capabilities migrations
Adding tests

TODO:

Add migration docs

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

codecov-commenter · 2022-08-26T14:08:26Z

Codecov Report

Merging #2134 (8571ecc) into main (f8f879b) will increase coverage by 0.02%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##             main    #2134      +/-   ##
==========================================
+ Coverage   79.40%   79.42%   +0.02%     
==========================================
  Files         170      171       +1     
  Lines       11837    11866      +29     
==========================================
+ Hits         9399     9425      +26     
- Misses       2022     2024       +2     
- Partials      416      417       +1

Impacted Files	Coverage Δ
...in-accounts/controller/migrations/v5/migrations.go	`89.65% <89.65%> (ø)`

…cosmos/ibc-go into damian/ics27-chan-capability-migrations

modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go

… tests

modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go

damiannolan · 2022-08-30T09:01:54Z

modules/apps/27-interchain-accounts/controller/migrations/v5/migrations_test.go

+	suite.Require().Nil(cap)
+	suite.Require().False(found)
+
+	suite.ResetMemStore() // empty the x/capability in-memory store


I decided to add this to the tests to mock/simulate a new chain binary running the migration handler against persisted state only. I think with this I feel better about the assertions done below after capabilityKeeper.InitMemStore(ctx) is called in the migration handler.

seantking · 2022-08-30T11:53:52Z

modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go

+)
+
+// MigrateICS27ChannelCapability performs a search on a prefix store using the provided store key and module name.
+// It retrieves the associated channel capability index and reassigns ownership to the ICS27 controller submodule.


Might be worth linking to an issue describing why the migration is necessary 🤔

seantking

Super clean 🤝

colin-axner

Wow, excellent! Super clean and very easy to follow. Could you add a changelog entry?

modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go

chatton

Looks great! Just left a few comments/questions

chatton · 2022-08-30T12:05:13Z

modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go

+		index := capabilitytypes.IndexFromKey(iterator.Key())
+
+		var owners capabilitytypes.CapabilityOwners
+		cdc.MustUnmarshal(iterator.Value(), &owners)


if the function already has an error as a method signature, should be return an error instead? (or do we want the panic to be handled elsewhere?)

This error would be very unexpected. It would imply a bug in the code, so I think a panic makes sense

Yeah, I can update this to return the err. I would expect the panic to be caught by the sdk, but it's probably cleaner to return the error. I just grabbed this from some code in x/capability.

chatton · 2022-08-30T12:08:02Z

modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go

+
+	for ; iterator.Valid(); iterator.Next() {
+		// unmarshal the capability index value and set of owners
+		index := capabilitytypes.IndexFromKey(iterator.Key())


what's the significance of this index rather than using the index in the loop below, are these separate?

This index refers to the capability index itself rather than the loop index.
The x/capability module essentially maintains a monotonically increasing uint64 sequence or index which it uses to create and maintain capabilities. This index is a field on the capability pointer which is generated per node. So the index should always be the same but obviously there will be different pointer values on each node.

Is this a fair description @AdityaSripal?

modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go

AdityaSripal

Excellent test!!

AdityaSripal · 2022-08-30T12:16:55Z

modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go

+		index := capabilitytypes.IndexFromKey(iterator.Key())
+
+		var owners capabilitytypes.CapabilityOwners
+		cdc.MustUnmarshal(iterator.Value(), &owners)


This error would be very unexpected. It would imply a bug in the code, so I think a panic makes sense

modules/apps/27-interchain-accounts/controller/migrations/v5/migrations.go

…cosmos/ibc-go into damian/ics27-chan-capability-migrations

* 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 * updating migration code to run against persisted state only, adapting tests * updating inline comments * adding changelog entry (cherry picked from commit 0a8602c) # Conflicts: # CHANGELOG.md

* chore: ics27 channel capability migrations (#2134) * 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 * updating migration code to run against persisted state only, adapting tests * updating inline comments * adding changelog entry (cherry picked from commit 0a8602c) # Conflicts: # CHANGELOG.md * fix conflicts * add back changelog entry Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Colin Axnér <[email protected]>

damiannolan added 5 commits August 26, 2022 12:00

wip initial commit

a6575eb

draft migration completed

94091cf

removing unnecessary storekey arg

3c95389

additional cleanup

2bb7c3f

adding updates to migrations and tests additional assertions

d546d20

damiannolan added 6 commits August 26, 2022 16:56

updating and moving migrations code

a79d10e

adding additional checks to tests

c2da8b2

cleaning up tests

6ea2014

cleaning up tests

c981bcc

updating inline doc comments, checking err return

88124f4

Merge branch 'main' into damian/ics27-chan-capability-migrations

cf3bfaf

damiannolan marked this pull request as ready for review August 26, 2022 16:00

damiannolan requested review from seantking, colin-axner and AdityaSripal as code owners August 26, 2022 16:00

damiannolan requested review from charleenfei, chatton and crodriguezvega August 26, 2022 16:01

damiannolan added 2 commits August 29, 2022 12:53

using InitMemStore in favour of InitializeCapability, adjusting tests

443b548

Merge branch 'damian/ics27-chan-capability-migrations' of github.com:…

bdec1bc

…cosmos/ibc-go into damian/ics27-chan-capability-migrations

colin-axner requested changes Aug 29, 2022

View reviewed changes

updating migration code to run against persisted state only, adapting…

e11be4c

… tests

damiannolan commented Aug 30, 2022

View reviewed changes

damiannolan added 2 commits August 30, 2022 11:04

Merge branch 'main' into damian/ics27-chan-capability-migrations

b5ad7aa

updating inline comments

8571ecc

seantking reviewed Aug 30, 2022

View reviewed changes

seantking approved these changes Aug 30, 2022

View reviewed changes

colin-axner approved these changes Aug 30, 2022

View reviewed changes

chatton reviewed Aug 30, 2022

View reviewed changes

AdityaSripal approved these changes Aug 30, 2022

View reviewed changes

damiannolan added 2 commits August 30, 2022 20:48

adding changelog entry

bf8457b

Merge branch 'main' into damian/ics27-chan-capability-migrations

5840e16

damiannolan enabled auto-merge (squash) August 30, 2022 18:49

damiannolan added automerge backport-to-v5.1.x labels Aug 30, 2022

damiannolan added 6 commits August 31, 2022 10:13

Merge branch 'main' into damian/ics27-chan-capability-migrations

96b733b

Merge branch 'damian/ics27-chan-capability-migrations' of github.com:…

738135d

…cosmos/ibc-go into damian/ics27-chan-capability-migrations

adding inline comment re. module name

a1e34b9

updating tests

8fba15f

assert channel-0 ownership by controller

dc258a2

Merge branch 'main' into damian/ics27-chan-capability-migrations

1cee4fc

damiannolan merged commit 0a8602c into main Aug 31, 2022

damiannolan deleted the damian/ics27-chan-capability-migrations branch August 31, 2022 10:42

mergify bot mentioned this pull request Aug 31, 2022

chore: ics27 channel capability migrations (backport #2134) #2155

Merged

This was referenced Sep 5, 2022

Add migration docs for ibc-go v5 -> v6 #2184

Closed

E2E: Run icad upgrade test with ICS27 migrations #2188

Closed

chatton mentioned this pull request Nov 7, 2022

Add git cliff toml to support generation of changelog #2697

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: ics27 channel capability migrations #2134

chore: ics27 channel capability migrations #2134

damiannolan commented Aug 26, 2022 •

edited

Loading

codecov-commenter commented Aug 26, 2022 •

edited

Loading

damiannolan Aug 30, 2022

seantking Aug 30, 2022

seantking left a comment

colin-axner left a comment

chatton left a comment

chatton Aug 30, 2022

AdityaSripal Aug 30, 2022

damiannolan Aug 30, 2022

chatton Aug 30, 2022

damiannolan Aug 30, 2022 •

edited

Loading

AdityaSripal left a comment

AdityaSripal Aug 30, 2022

chore: ics27 channel capability migrations #2134

chore: ics27 channel capability migrations #2134

Conversation

damiannolan commented Aug 26, 2022 • edited Loading

Description

codecov-commenter commented Aug 26, 2022 • edited Loading

Codecov Report

damiannolan Aug 30, 2022

Choose a reason for hiding this comment

seantking Aug 30, 2022

Choose a reason for hiding this comment

seantking left a comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

chatton Aug 30, 2022

Choose a reason for hiding this comment

AdityaSripal Aug 30, 2022

Choose a reason for hiding this comment

damiannolan Aug 30, 2022

Choose a reason for hiding this comment

chatton Aug 30, 2022

Choose a reason for hiding this comment

damiannolan Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

AdityaSripal Aug 30, 2022

Choose a reason for hiding this comment

damiannolan commented Aug 26, 2022 •

edited

Loading

codecov-commenter commented Aug 26, 2022 •

edited

Loading

damiannolan Aug 30, 2022 •

edited

Loading