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

Added rate limits in upgrade #4340

Merged
merged 51 commits into from
Feb 23, 2023
Merged

Added rate limits in upgrade #4340

merged 51 commits into from
Feb 23, 2023

Conversation

nicolaslara
Copy link
Contributor

@nicolaslara nicolaslara commented Feb 16, 2023

Closes: #4303

What is the purpose of the change

This adds the rate limits in the v15 upgrade for when https://www.mintscan.io/osmosis/proposals/427 passes.

Brief Changelog

  • Add rate limits in v15

Testing and Verifying

  • The upgrade has tests.
  • The upgrade has E2E tests
  • This should also be tested on a testnet with state export.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Feb 16, 2023
@nicolaslara nicolaslara self-assigned this Feb 16, 2023
@nicolaslara nicolaslara added the V:state/compatible/backport State machine compatible PR, should be backported label Feb 16, 2023
@ValarDragon ValarDragon added V:state/breaking State machine breaking PR A:backport/v15.x backport patches to v15.x branch and removed V:state/compatible/backport State machine compatible PR, should be backported labels Feb 17, 2023
@ValarDragon
Copy link
Member

RIP goimports error. Do you have your editor set to format on save?

@nicolaslara
Copy link
Contributor Author

RIP goimports error. Do you have your editor set to format on save?

I do, but it doesn't work when I fix conflicts and some other weird situations like that.

@ValarDragon but!! this is not ready. The E2E tests are passing because I deactivated them after failing to get them to work (rip).

I asked Nicco to test the upgrade with a state export to make sure the issue is with E2E and not with the upgrade

@nicolaslara
Copy link
Contributor Author

That change makes a lot of sense! I thought that was just initialization. Thanks for the find!

@nicolaslara
Copy link
Contributor Author

hmm, this is still failing when there contracts exist on v14. I'll try to test this again tomorrow on a real node and see what happens

@p0mvn
Copy link
Member

p0mvn commented Feb 22, 2023

Latest updates:

  • wired ibc-rate-limit as SDK module to have init and export genesis logic
  • changed querier to be code-gen compatible and fixed CLI
  • fixed bug with not getting params correctly due to not having a pointer receiver on ParamSetPairs:
    func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs {
  • fixed bug with overriding contract parameter due to ibc-rate-limit consensus version being set to 1, leading to overwrite during the upgrade. Fixed with:
    fromVM[ibcratelimittypes.ModuleName] = 0

We manually checked the presence of the contract parameter and rate limits post-upgrade. However, we should add a test in e2e_test.go doing that for us. I think this is the only remaining TODO here

@nicolaslara nicolaslara marked this pull request as ready for review February 22, 2023 12:33
tests/e2e/e2e_test.go Outdated Show resolved Hide resolved
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

LGTM

@nicolaslara nicolaslara added A:backport/v14.x backport patches to v14.x branch and removed A:backport/v14.x backport patches to v14.x branch labels Feb 23, 2023
@nicolaslara nicolaslara merged commit a1e2b3d into main Feb 23, 2023
@nicolaslara nicolaslara deleted the nicolas/set-rate-limits branch February 23, 2023 12:42
mergify bot pushed a commit that referenced this pull request Feb 23, 2023
* added rate limits in upgrade

* added upgrade test

* refactor rate limiting tests and added rate limits pre-upgrade

* lint

* goimports

* experiments for E2E

* post merge lints

* added RL before upgrade

* just get the latest contract

* experiments

* updated wasm file

* remove unnecessary prints

* bad check

* experiments without e2e

* removed unnecessary fmt

* param space initialization for rate limiting (#4360)

* param space initialization for rate limiting

* lint

* better test

* properly initialize empty params

* fix typo

* adding RL to E2E

* removing RL again from E2E

* debug

* genesis

* genesis 2

* add genesis and solve params bug

* uncomment setup rate limiting

* added configurable gov module

* clean up

* clean up and genesis test

* upgrade handler

* genesis clean up

* clean up e2e

* wire querier

* route fix

* fix router

* debug things

* fix init genesis bug

* push fix

* lint

* fix

* clean up upgrades

* testing propper params after upgrade in E2E

* "properly" unmarshaling the parm

* goimports

* allowing for types other than objects in cosmwasm queries

* fix check for param reset

* remove unnecessary prints

* compiled with the proper vesion of workspace optimizer

* Revert "compiled with the proper vesion of workspace optimizer"

This reverts commit ab5cf6c.

* added length check

* params experiment for test with state-export

---------

Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: Roman <ackhtariev@gmail.com>
(cherry picked from commit a1e2b3d)

# Conflicts:
#	app/keepers/modules.go
#	app/modules.go
#	app/upgrades/v15/export_test.go
#	app/upgrades/v15/upgrade_test.go
#	app/upgrades/v15/upgrades.go
#	tests/e2e/configurer/chain/commands.go
#	tests/e2e/e2e_test.go
mergify bot pushed a commit that referenced this pull request Feb 23, 2023
* added rate limits in upgrade

* added upgrade test

* refactor rate limiting tests and added rate limits pre-upgrade

* lint

* goimports

* experiments for E2E

* post merge lints

* added RL before upgrade

* just get the latest contract

* experiments

* updated wasm file

* remove unnecessary prints

* bad check

* experiments without e2e

* removed unnecessary fmt

* param space initialization for rate limiting (#4360)

* param space initialization for rate limiting

* lint

* better test

* properly initialize empty params

* fix typo

* adding RL to E2E

* removing RL again from E2E

* debug

* genesis

* genesis 2

* add genesis and solve params bug

* uncomment setup rate limiting

* added configurable gov module

* clean up

* clean up and genesis test

* upgrade handler

* genesis clean up

* clean up e2e

* wire querier

* route fix

* fix router

* debug things

* fix init genesis bug

* push fix

* lint

* fix

* clean up upgrades

* testing propper params after upgrade in E2E

* "properly" unmarshaling the parm

* goimports

* allowing for types other than objects in cosmwasm queries

* fix check for param reset

* remove unnecessary prints

* compiled with the proper vesion of workspace optimizer

* Revert "compiled with the proper vesion of workspace optimizer"

This reverts commit ab5cf6c.

* added length check

* params experiment for test with state-export

---------

Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: Roman <ackhtariev@gmail.com>
(cherry picked from commit a1e2b3d)

# Conflicts:
#	app/modules.go
#	tests/e2e/e2e_test.go
This was referenced Feb 23, 2023
p0mvn pushed a commit that referenced this pull request Feb 23, 2023
* Added rate limits in upgrade (#4340)

* added rate limits in upgrade

* added upgrade test

* refactor rate limiting tests and added rate limits pre-upgrade

* lint

* goimports

* experiments for E2E

* post merge lints

* added RL before upgrade

* just get the latest contract

* experiments

* updated wasm file

* remove unnecessary prints

* bad check

* experiments without e2e

* removed unnecessary fmt

* param space initialization for rate limiting (#4360)

* param space initialization for rate limiting

* lint

* better test

* properly initialize empty params

* fix typo

* adding RL to E2E

* removing RL again from E2E

* debug

* genesis

* genesis 2

* add genesis and solve params bug

* uncomment setup rate limiting

* added configurable gov module

* clean up

* clean up and genesis test

* upgrade handler

* genesis clean up

* clean up e2e

* wire querier

* route fix

* fix router

* debug things

* fix init genesis bug

* push fix

* lint

* fix

* clean up upgrades

* testing propper params after upgrade in E2E

* "properly" unmarshaling the parm

* goimports

* allowing for types other than objects in cosmwasm queries

* fix check for param reset

* remove unnecessary prints

* compiled with the proper vesion of workspace optimizer

* Revert "compiled with the proper vesion of workspace optimizer"

This reverts commit ab5cf6c.

* added length check

* params experiment for test with state-export

---------

Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: Roman <ackhtariev@gmail.com>
(cherry picked from commit a1e2b3d)

# Conflicts:
#	app/modules.go
#	tests/e2e/e2e_test.go

* fix conflicts

---------

Co-authored-by: Nicolas Lara <nicolaslara@gmail.com>
@github-actions github-actions bot mentioned this pull request Apr 1, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v14.x backport patches to v14.x branch A:backport/v15.x backport patches to v15.x branch C:app-wiring Changes to the app folder C:CLI V:state/breaking State machine breaking PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add IBC rate limits in the v15 upgrade
3 participants