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

Migrate epoching and btccheckpointing module to new way of handling params #344

Merged
merged 3 commits into from
Apr 5, 2023

Conversation

KonradStaniec
Copy link
Collaborator

Migrates btcheckpoint module and epoching module to new way of handling params i.e storing them themselves and giving authority to gov module to execute UpdateParamsMsg on their behalf.

We sadly can't yet remove params module from app.go totally as IBC modules did not migrate yet

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

Nice! Would be interesting to test doing a governance proposal using that in practice. Also, lint fails

@KonradStaniec
Copy link
Collaborator Author

Nice! Would be interesting to test doing a governance proposal using that in practice. Also, lint fails

Yea thought about that. It should be possible to do using either e2e/integration testing. My main objection to it, is that such test does not really test our logic but rather governance module logic which is outside of our control (as logic of handling UpdateParamsMsg is super simple in either btccheckpoint or epoching modules.), and adding additional test means more CI traffic.

Maybe it is the time to define some e2e tests to run nightly on dev branch, and put such less important/longer running tests there ? We could probably re-use e2e testing framework for this just use different build tag or maybe some env variables to match on test which should run on nightly basis.

@vitsalis
Copy link
Member

vitsalis commented Apr 4, 2023

Yep that would be an interesting idea. @philmln thoughts? We could also run those with a devnet once we do the kubernetes migration.

@filippos47
Copy link
Contributor

I like the nightly run idea; we can use CircleCI's scheduled pipelines to configure them. I also agree that e2e tests should be automatically executed on our future devnets.

Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

Great work! The new approach of handling param updates would be really useful. No blocker from my side, but a question in this design in general: would this give the authority (i.e., the governance account) too much power, given that it can change parameters anytime as its wish?

Another thing is that for certain param changes, e.g., epoch interval, the change has to be trigger until the end of an epoch. Might be good if we have a TODO for this.

Maybe it is the time to define some e2e tests to run nightly on dev branch, and put such less important/longer running tests there ? We could probably re-use e2e testing framework for this just use different build tag or maybe some env variables to match on test which should run on nightly basis.

I also like the idea of running e2e tests, or even integration tests nightly. It saves cost in CI and wait time for merging a PR. It's not that necessary to run it for every small PR anyway.

@KonradStaniec
Copy link
Collaborator Author

would this give the authority (i.e., the governance account) too much power, given that it can change parameters anytime as its wish?

Possibly but as I understand this is the current way of handling all gov proposals i.e gov module need to have authority to executes module messages on their behalf, and all cosmos modules in v47 migrated to that approach.

Another thing is that for certain param changes, e.g., epoch interval, the change has to be trigger until the end of an epoch. Might be good if we have a TODO for this.

Oh this is good point I will leave TODO in code to investigate it in separate task/pr as there are few ways to approach it like:

  • queue update params message and execute it and epoch boundary
  • execute msg immediately, but make it take effect in the next epoch only i.e make sure param dependency is only used at start of the epoch
  • execute msg immediately and extend current epoch, make sure nothing depends on current epoch lengh until it is finished.

I feel that the first one is conceptually simplest, but never the less it is worth separate task.

@KonradStaniec KonradStaniec force-pushed the feature/new-way-of-handling-params branch from 3d6b25d to 2e233cd Compare April 5, 2023 06:14
@KonradStaniec KonradStaniec merged commit 7dc3b57 into dev Apr 5, 2023
@KonradStaniec KonradStaniec deleted the feature/new-way-of-handling-params branch April 5, 2023 06:34
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.

4 participants