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: ics27 channel capability migrations #2134

Merged
merged 24 commits into from
Aug 31, 2022

Conversation

damiannolan
Copy link
Member

@damiannolan damiannolan commented Aug 26, 2022

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

codecov-commenter commented Aug 26, 2022

Codecov Report

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

Impacted file tree graph

@@            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%> (ø)

suite.Require().Nil(cap)
suite.Require().False(found)

suite.ResetMemStore() // empty the x/capability in-memory store
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 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.

)

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@seantking seantking left a comment

Choose a reason for hiding this comment

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

Super clean 🤝

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.

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

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.

Looks great! Just left a few comments/questions

index := capabilitytypes.IndexFromKey(iterator.Key())

var owners capabilitytypes.CapabilityOwners
cdc.MustUnmarshal(iterator.Value(), &owners)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@damiannolan damiannolan Aug 30, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Excellent test!!

index := capabilitytypes.IndexFromKey(iterator.Key())

var owners capabilitytypes.CapabilityOwners
cdc.MustUnmarshal(iterator.Value(), &owners)
Copy link
Member

Choose a reason for hiding this comment

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

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

@damiannolan damiannolan merged commit 0a8602c into main Aug 31, 2022
@damiannolan damiannolan deleted the damian/ics27-chan-capability-migrations branch August 31, 2022 10:42
mergify bot pushed a commit that referenced this pull request Aug 31, 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

* 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
damiannolan added a commit that referenced this pull request Aug 31, 2022
* 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 <damiannolan@gmail.com>
Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants