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

fix(e2e): rate limiting contract configuration settings #4703

Closed
Tracked by #4700
p0mvn opened this issue Mar 23, 2023 · 3 comments · Fixed by #4771
Closed
Tracked by #4700

fix(e2e): rate limiting contract configuration settings #4703

p0mvn opened this issue Mar 23, 2023 · 3 comments · Fixed by #4771
Assignees
Labels
C:e2e T:bug 🐛 Something isn't working

Comments

@p0mvn
Copy link
Member

p0mvn commented Mar 23, 2023

Background

In v15, we configured the rate limiting contract via upgrade handler.

Now, that we are performing the upgrade from v15 to v16 in #4575 , the v15 upgrade handler code is not going to run. As a result, we need to configure the contract in the genesis from an updated image

Suggested Design

  • create a PR to main where:
    • these configurations are executed on the rate limiter contract
    • contract instantiation pre-upgrade is uncommented

Acceptance Criteria

  • rate limiter e2e test works
  • e2e passes
@p0mvn
Copy link
Member Author

p0mvn commented Mar 25, 2023

In #4575, had to disable the rate limiting param tests until the params are set via genesis overwrite.

Ref: https://github.com/osmosis-labs/osmosis/pull/4575/files#r1148441164

@nicolaslara nicolaslara moved this from Needs Triage 🔍 to Todo 🕒 in Osmosis Chain Development Mar 27, 2023
@nicolaslara
Copy link
Contributor

To do this, we would need the rate limiting module to have access to a ContractKeeper with higher permissions (so it can instantiate the contract on genesis). This is easy enough to do, but expands the permissions of the module unnecessarily.

Does it make more sense to just remove this test on e2e? I don't think we need it. If we want to test intearactions with RL, we can instantiate a new contract in the test and configure it specifically for that.

@p0mvn
Copy link
Member Author

p0mvn commented Mar 27, 2023

Does it make more sense to just remove this test on e2e? I don't think we need it. If we want to test intearactions with RL, we can instantiate a new contract in the test and configure it specifically for that.

Thanks for looking into it. This makes sense to me

@nicolaslara nicolaslara moved this from Todo 🕒 to In Progress🏃 in Osmosis Chain Development Mar 27, 2023
@nicolaslara nicolaslara moved this from In Progress🏃 to Needs PR Review in Osmosis Chain Development Mar 28, 2023
@github-project-automation github-project-automation bot moved this from Needs PR Review to Done ✅ in Osmosis Chain Development Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:e2e T:bug 🐛 Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants