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

[Incentives Module][bugfix] Scale gas costs by denoms in gauge #4830

Merged
merged 7 commits into from
Apr 21, 2023

Conversation

stackman27
Copy link
Contributor

Closes: #4738

What is the purpose of the change

Similarly to what we did for incentive records in CL, we should be scaling gas costs for adding to gauges to prevent spam, as distribution logic linearly iterates over all denoms in a gauge at epoch.

Add scaling gas costs to AddToGaugeRewards, similar to how we did it for incentive records

Brief Changelog

  • Add gas cost when we AddToGaugeRewards, linearly increase with coins to add

Testing and Verifying

added new tests

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)

@stackman27 stackman27 added V:state/compatible/backport State machine compatible PR, should be backported V:state/breaking State machine breaking PR and removed V:state/compatible/backport State machine compatible PR, should be backported labels Apr 4, 2023
Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu left a comment

Choose a reason for hiding this comment

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

LGTM! Left a couple minor comments. Approving rn, but we should add the test case I suggested before merging.

x/incentives/keeper/gauge.go Outdated Show resolved Hide resolved
sdk.NewCoin("atom", sdk.NewInt(99999)),
),
gaugeId: 1,
minimumGasConsumed: uint64(2 * types.BaseGasFeeForAddRewardToGauge),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the base gas cost for txns like these are usually in the 10k-20k gas range, we should add a test case with many denoms (>4) to ensure that the minimumGasConsumed check isn't passing trivially

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good added! for 8 tokens it looks like this;

gasConsumed: 161441  tc. minimumGasConsumed: 80000

@@ -152,6 +152,10 @@ func (k Keeper) AddToGaugeRewards(ctx sdk.Context, owner sdk.AccAddress, coins s
if gauge.IsFinishedGauge(ctx.BlockTime()) {
return errors.New("gauge is already completed")
}

// Fixed gas consumption adding reward to gauges based on the number of coins to add
ctx.GasMeter().ConsumeGas(uint64(types.BaseGasFeeForAddRewardToGauge*len(coins)), "scaling gas cost for adding to gauge rewards")
Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu Apr 4, 2023

Choose a reason for hiding this comment

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

Just realized that this should actually be charging baseAmount * (len(gauge.Coins) + len(coins)) since we allow for adding multiple coins at once here. The current implementation only scales by the number of coins being added, not the ones that already exist.

Copy link
Contributor Author

@stackman27 stackman27 Apr 4, 2023

Choose a reason for hiding this comment

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

i see, i originally thought it was just for new denoms. I'll change it to accomodate existing denoms

Re 
gasConsumed: 171474  minimumGasConsumed: 80000

@p0mvn
Copy link
Member

p0mvn commented Apr 11, 2023

@stackman27 please add a CHANGELOG entry prior to merge

@stackman27 stackman27 requested a review from mattverse as a code owner April 11, 2023 19:11
@stackman27
Copy link
Contributor Author

@stackman27 please add a CHANGELOG entry prior to merge

added!

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.

Nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/incentives V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants