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: refactor genesis types to separate directory, mv controller interfaces registration to module.go #2133

Merged
merged 7 commits into from
Aug 29, 2022

Conversation

charleenfei
Copy link
Contributor

Description

prior to this pr, the genesis protos were in interchain_account/v1/types but imported controllertypes in the testing file, and the controller interfaces registration was also being handled in interchain account types dir.

this caused a circular import problem when trying to import the InterchainAccountPacketData type from the ICA types directory to the controller types directory, as part of the PR addressing #2052 (#2052.)

this pr moves the genesis proto, genesis.go and genesis_testing file to it's own directory in a structure similar to host and controller


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

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

I'm happy enough with this layout if it solves the issues of importing InterchainAccountPacketData :)

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.

LGTM, I think the proto package name should remain unchanged though. The go option can be modified to generate the proto files in the new genesis directory as you have done. Can you add a changelog indicating that the genesis types have been moved to their own directory?

@damiannolan
Copy link
Member

The go option can be modified to generate the proto files in the new genesis directory as you have done.

As far as I remember you can't specify two different go_package options in the same proto pkg directory. I think we had issues generating the .pb.go files when I looked at this with @charleenfei

@charleenfei

This comment was marked as resolved.

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.

ACK, just a few notes. This is protobuf breaking since we are changing the package name. We are moving the types though which is why we aren't changing the v1 to v2. So long as the genesis types are not packed to an Any (no reason to do so), then this should not be a problem.

There isn't really a good alternative to avoid this situation if we are to have separate message/query servers for controller and host. It doesn't make too much sense to combine the message/query server into one keeper and could potentially lead to more issues. This seems like the least disruptive approach

CHANGELOG.md Outdated
@@ -71,6 +71,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (apps/27-interchain-accounts) [\#2133](https://github.com/cosmos/ibc-go/pull/2133) generates genesis protos in a separate directory to avoid circular import errors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* (apps/27-interchain-accounts) [\#2133](https://github.com/cosmos/ibc-go/pull/2133) generates genesis protos in a separate directory to avoid circular import errors
* (apps/27-interchain-accounts) [\#2133](https://github.com/cosmos/ibc-go/pull/2133) Generates genesis protos in a separate directory to avoid circular import errors. The protobuf package name has changed for the genesis types.

@charleenfei charleenfei enabled auto-merge (squash) August 29, 2022 15:36
@charleenfei charleenfei merged commit f8f879b into main Aug 29, 2022
@charleenfei charleenfei deleted the ics27/refactor-genesis-types branch August 29, 2022 16:29
mergify bot pushed a commit that referenced this pull request Aug 30, 2022
…terfaces registration to module.go (#2133)

Co-authored-by: Damian Nolan <damiannolan@gmail.com>
(cherry picked from commit f8f879b)

# Conflicts:
#	CHANGELOG.md
colin-axner added a commit that referenced this pull request Aug 31, 2022
…terfaces registration to module.go (backport #2133) (#2141)

* chore: refactor genesis types to separate directory, mv controller interfaces registration to module.go (#2133)

Co-authored-by: Damian Nolan <damiannolan@gmail.com>
(cherry picked from commit f8f879b)

# Conflicts:
#	CHANGELOG.md

* fix conflicts

Co-authored-by: Charly <charly@interchain.berlin>
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants