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

feat(incentives): rollapp gauges upgrade handler and burn creation fee #1113

Merged

Conversation

mtsitrin
Copy link
Contributor

Closes #1005 , #1083, #1081

This PR:

  1. adds upgrade handler for rollapp gauges
  2. clean up a bit x/incentives (uses NewGauge methods for creation instead of .Gauge{})
  3. creation fee is burned instead of community pool

@mtsitrin mtsitrin requested a review from a team as a code owner August 19, 2024 06:44
@mtsitrin mtsitrin linked an issue Aug 19, 2024 that may be closed by this pull request
@omritoptix omritoptix changed the title feat(incentives): minor refactor feat(incentives): rollapp gauges upgrade handler and burn creation fee Aug 19, 2024
Copy link
Contributor

@zale144 zale144 left a comment

Choose a reason for hiding this comment

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

small thing

x/incentives/keeper/msg_server.go Show resolved Hide resolved
x/incentives/keeper/msg_server.go Outdated Show resolved Hide resolved
@mtsitrin mtsitrin requested a review from zale144 August 25, 2024 07:08
// migrateRollappGauges creates a gauge for each rollapp in the store
func migrateRollappGauges(ctx sdk.Context, rollappkeeper *rollappkeeper.Keeper, incentivizeKeeper *incentiveskeeper.Keeper) error {
rollapps := rollappkeeper.GetAllRollapps(ctx)
for _, rollapp := range rollapps {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we not skip frozen rollapps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer not.
It doesn't really matter, as this gaguge won't be active for frozen rollapp, but it breaks the assumption of rollapp gauge for each rollapp

app/upgrades/v4/upgrade_test.go Show resolved Hide resolved
x/incentives/keeper/msg_server.go Show resolved Hide resolved
x/incentives/keeper/msg_server.go Outdated Show resolved Hide resolved
}
return nil
return k.bk.SendCoinsFromAccountToModule(ctx, address, txfeestypes.ModuleName, sdk.NewCoins(sdk.NewCoin(feeDenom, types.CreateGaugeFee)))
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens to these coins then? They stay in the module, should we burn them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the txfees module burns the accumulated tokens periodiocally

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 9 lines in your changes missing coverage. Please review.

Project coverage is 25.36%. Comparing base (61d5ab1) to head (5edb098).
Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
app/upgrades/v4/upgrade.go 45.45% 4 Missing and 2 partials ⚠️
x/incentives/keeper/msg_server.go 83.33% 2 Missing ⚠️
x/sequencer/keeper/msg_server_create_sequencer.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1113      +/-   ##
==========================================
- Coverage   28.22%   25.36%   -2.87%     
==========================================
  Files         332      496     +164     
  Lines       52494    97746   +45252     
==========================================
+ Hits        14818    24789    +9971     
- Misses      35346    69783   +34437     
- Partials     2330     3174     +844     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -225,99 +224,3 @@ func (suite *KeeperTestSuite) TestGaugeOperations() {
}
}
}

func (suite *KeeperTestSuite) TestChargeFeeIfSufficientFeeDenomBalance() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@omritoptix omritoptix merged commit ac7d25e into main Sep 23, 2024
99 of 126 checks passed
@omritoptix omritoptix deleted the mtsitrin/1083-possible-unused-code-in-incentives-module branch September 23, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

possible unused code in incentives module feat(incentives): rollapp gauges upgrade handler
3 participants