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

Add a new field to ica controller genesis to track middleware enabled state #2165

Closed
3 tasks
colin-axner opened this issue Aug 31, 2022 · 1 comment · Fixed by #2177
Closed
3 tasks

Add a new field to ica controller genesis to track middleware enabled state #2165

colin-axner opened this issue Aug 31, 2022 · 1 comment · Fixed by #2177
Assignees
Milestone

Comments

@colin-axner
Copy link
Contributor

Summary

A new key has been added to ica controller keeper. We need to be able to export/import it

Proposal

Add a new field to genesis state for controller (change to be v2 in proto url). Handle init and export genesis appropriately. A GetAll function will need to be added to do a range query on the controller keeper


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@damiannolan
Copy link
Member

damiannolan commented Sep 1, 2022

change to be v2 in proto url

I don't think this is required when adding new fields. I briefly discussed this with @marbar3778 to see how the sdk handles proto changes and I think the proto version only needs to be bumped if we are changing the ordering of fields or breaking existing types. Adding an additional field should be fine, and JSON marshalling/unmarshalling should behave fine for genesis types as the omitempty tag is used as standard with all protobuf generated types.

I've opened two draft PRs linked on this issue.

  1. chore(draft): adding MiddlewareEnabled type to v2 controller genesis  #2176

This PR adds a new MiddlewareEnabled type and bumps the proto version as suggested in this issue. This required a decent bit of refactoring and adding of new go packages..etc. It adds a GetAllMiddlewareEnabledChannels keeper func to the controller submodule.

  1. chore: adding IsMiddlewareEnabled to ActiveChannel genesis type #2177

This PR, which I propose we go ahead with if at all possible, adds a new bool field IsMiddlewareEnabled to the ActiveChannel genesis type - which already includes the port/channel ID pair which we need to set the middleware flag for a particular channel. I think given that we eventually aim to remove the concept of active channels with the new channel type, and also aim to eventually fully deprecate and remove the controller middleware then this is a pretty nice approach with minimal noise. I think it should be safe to add the additional bool field here.

Would like to hear thoughts on this and see if we can proceed with the least disruptive approach possible. cc. @colin-axner @seantking @AdityaSripal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment