Ensure gauges can only be created for assets that exist on-chain #855
Conversation
|
Awesome! Can you fix the tests for this as well? @AlpinYukseloglu |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #855 +/- ##
==========================================
+ Coverage 20.00% 20.61% +0.61%
==========================================
Files 189 191 +2
Lines 24549 25792 +1243
==========================================
+ Hits 4910 5318 +408
- Misses 18783 19567 +784
- Partials 856 907 +51 ☔ View full report in Codecov by Sentry. |
Should be good to go! |
| @@ -39,8 +39,10 @@ enum LockQueryType { | |||
| } | |||
|
|
|||
| message QueryCondition { | |||
| LockQueryType lock_query_type = 1; // type of lock query, ByLockDuration | ByLockTime | |||
| string denom = 2; // What token denomination are we looking for lockups of | |||
| // type of lock query, ByLockDuration | ByLockTime | |||
There was a problem hiding this comment.
Nice change, as we found out these don't get into the go docs. Can you also make an issue in this repo to update all of our proto files for this? (Moving comments above the line they affect, rather than in the same line, to the side?)
There was a problem hiding this comment.
Created one here: #863
| mintLPtokens := sdk.Coins{sdk.NewInt64Coin(distrTo.Denom, 200)} | ||
| err = simapp.FundAccount(suite.app.BankKeeper, suite.ctx, gaugeCreator, mintLPtokens) | ||
| suite.Require().NoError(err) | ||
|
|
||
| gaugeId, err := suite.app.IncentivesKeeper.CreateGauge(suite.ctx, true, gaugeCreator, coins, distrTo, time.Now(), 1) |
There was a problem hiding this comment.
Why are we minting / funding account here? I understand why it should be happening in setupGaugeForLPIncentives but I dont understand why we're minting LP Tokens here(Don't have opinions btw, really just curous)
There was a problem hiding this comment.
| mintLPtokens := sdk.Coins{sdk.NewInt64Coin(distrTo.Denom, 200)} | ||
| err = simapp.FundAccount(app.BankKeeper, ctx, addr, mintLPtokens) | ||
| require.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
Why are we minting / funding account here? I understand why it should be happening in setupGaugeForLPIncentives but I dont understand why we're minting LP Tokens here(Don't have opinions btw, really just curious)
There was a problem hiding this comment.
I think maybe the denom lptoken might be a bit misleading - I didn't pick apart what the entire test is doing and whether or not it is necessary for it to create a gauge that distributes to the denom lptoken, but since it does, we have to make sure the denom has supply on-chain since otherwise, CreateGauge fails due to the new check we've added. It shouldn't affect the test's direct functionality - it just makes sure that the test is compliant with the new constraint that we've added to CreateGauge.
Closes: #530
Description
Adds a check in the gauge creation process that prevents gauges from being created for denoms that do not exist on-chain.
For contributor use:
docs/) or specification (x/<module>/spec/)Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorer