osmosis icon indicating copy to clipboard operation
osmosis copied to clipboard

Move gov module into Osmosis

Open ValarDragon opened this issue 3 years ago • 2 comments

Closes: #2481

What is the purpose of the change

Moves the governance module from our SDK fork into osmosis. This will let us more easily work on features for gov, as gov is truly a chain-specific module.

This PR should do everything needed for the initial move. Currently there is a blocker on protobuf generation, where it does not work at the moment.

We need to assess a strategy for this. Options

  • Change the proto path for all of these types to be 'osmosis' rather than 'cosmos'. Does this have client side impacts? My understanding is yes, but cc @pyramation
  • Override the cosmos in the paths.

We generally want to aim for being query and message compatible with the SDK if possible, but with cadence mismatches.

Brief Changelog

  • Move gov package from our SDK fork into this repo.

Testing and Verifying

  • This brings over the gov tests as they are, which actually aren't working.
    • We need to replace the tests with either Osmosis specific app usage, or by reusing the mock package in SDK gov mainline https://github.com/cosmos/cosmos-sdk/tree/3de5aa87416667305b5606e42d5617aa1126d73e/x/gov
    • I prefer the latter, since then we will be able to split this into its own go mod easier & get better test caching improvements.

The first commit just copies the proto & gov folder from the SDK which could be verified by copying these and comparing the diff from https://github.com/osmosis-labs/cosmos-sdk/tree/36adfc0d988d5d67a40e55b560c564988245e7c9/x/gov

Everything else can be reviewed commit by commit

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? TODO
  • How is the feature or change documented? in its own spec

ValarDragon avatar Sep 25 '22 11:09 ValarDragon

Oh no, proto registers globals within init blobs :(

So if we want to keep cosmos import paths, we must delete gov from the osmosis fork (even more complicated)

I think this points to then making osmosis import paths :(

ValarDragon avatar Sep 25 '22 12:09 ValarDragon

woaaa amazing work @ValarDragon -- I'm a bit confused on the proto PR needed (ref: https://github.com/osmosis-labs/cosmos-sdk/pull/341). Can you maybe help describe the issue on the next chain dev call? 🙏

alexanderbez avatar Oct 05 '22 17:10 alexanderbez

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

github-actions[bot] avatar Oct 20 '22 00:10 github-actions[bot]

hey @ValarDragon @alexanderbez I think this was a tag just day before cosmosverse, I must have missed this, pardon the delay!

I think it's totally fine to move the module from cosmos -> osmosis technically, but unless the old gov messages/queries still work, it would definitely break any mintscan, keplr, osmojs voting until the protos are regenerated, any methods/apis exposing protos are also updated, and any software using those methods/apis would also need to be updated.

So in this case, I would say it would require coordination with mintscan and keplr at least.

pyramation avatar Oct 20 '22 08:10 pyramation

The old messages/queries should still work under this

ValarDragon avatar Oct 20 '22 16:10 ValarDragon

I think we still want to have gov in our repo, even if we switch back to mainline.

ValarDragon avatar Oct 25 '22 01:10 ValarDragon

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

github-actions[bot] avatar Nov 09 '22 00:11 github-actions[bot]

Last commit is just trying to re-run CI

ValarDragon avatar Sep 08 '23 11:09 ValarDragon

@ValarDragon Are we still good to do this?

czarcas7ic avatar Sep 23 '23 18:09 czarcas7ic

Sorry was opened as an example to get CI to run, re-closing for now.

ValarDragon avatar Sep 25 '23 14:09 ValarDragon