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

x/incentives: refactor create gauge and add to gauge fees to use txfees denom #2283

Merged
merged 5 commits into from
Aug 3, 2022

Conversation

stackman27
Copy link
Contributor

@stackman27 stackman27 commented Aug 2, 2022

Closes: #2236

What is the purpose of the change

In #2227 we hard coded the fee denom for create gauge and add to gauge fees. This was done to minimize the changes for a smooth and expedited v11 release.

However, the hard-coded denom causes issues with the simulator. Simulator assumes "stake" denom for majority of its tests by default. As a result, no account has the hard coded "uosmo" denom to pay the fee. The simulator doesn't run because no account has "uosmo", only "stake"

Brief Changelog

We should avoid backporting these changes to v11.x

Testing and Verifying

Use dynamic denom calculation instead of hardcoding it in these tests ;

  1. TestCreateGauge_Fee
  2. TestAddToGauge_Fee
  3. TestChargeFeeIfSufficientFeeDenomBalance

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? (yes)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@stackman27 stackman27 requested a review from a team August 2, 2022 23:30
@github-actions github-actions bot added C:app-wiring Changes to the app folder C:simulator Edits simulator or simulations C:x/incentives labels Aug 2, 2022
@stackman27 stackman27 requested review from czarcas7ic and p0mvn August 2, 2022 23:31
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, nice work

x/incentives/simulation/operations.go Outdated Show resolved Hide resolved
x/incentives/simulation/operations.go Outdated Show resolved Hide resolved
@p0mvn
Copy link
Member

p0mvn commented Aug 2, 2022

Could you please add a changelog entry for this?

This would be API breaking due to the constructor update. However, I think we should still backport this to v11 since it's not state-breaking

@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Aug 2, 2022
@stackman27
Copy link
Contributor Author

@p0mvn addressed your feedback here 720f3bd

@p0mvn p0mvn added the A:backport/v11.x backport patches to v11.x branch label Aug 2, 2022
Comment on lines +290 to +292
if err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

This is technically a state break but the likelihood of this non-deterministically happening is low so I think we can still backport this

Copy link
Member

Choose a reason for hiding this comment

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

Actually, since this is on the message path, it is possible to run out of gas. So I think we should avoid backporting

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, now the simulator will be happy again 😄

x/incentives/simulation/operations.go Outdated Show resolved Hide resolved
x/incentives/simulation/operations.go Outdated Show resolved Hide resolved
@stackman27
Copy link
Contributor Author

@czarcas7ic good to go f245297

Can't upset the simulator for too long :D

@p0mvn p0mvn removed the A:backport/v11.x backport patches to v11.x branch label Aug 3, 2022
@p0mvn p0mvn merged commit d0aaca0 into osmosis-labs:main Aug 3, 2022
@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
C:app-wiring Changes to the app folder C:docs Improvements or additions to documentation C:simulator Edits simulator or simulations C:x/incentives
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x/incentives: refactor create gauge and add to gauge fees to use txfees denom
3 participants