[OTE-817] update trading rewards with new logic #2300
Conversation
|
Warning Rate limit exceeded@affanv14 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 49 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces significant updates to the revenue sharing mechanism, including the renaming and value increase of the constant Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| // 25 million USDC | ||
| Max30dRefereeVolumeQuantums = uint64(25_000_000_000_000) | ||
| // 50 million USDC | ||
| Max30dRefereeVolumeQuantums = uint64(50_000_000_000_000) |
There was a problem hiding this comment.
| Max30dRefereeVolumeQuantums = uint64(50_000_000_000_000) | |
| MaxReferee30dVolumeForAffiliateShareQuantums= uint64(50_000_000_000_000) |
For accuracy
protocol/x/rewards/keeper/keeper.go
Outdated
| if value, ok := revSharesForFill.FeeSourceToQuoteQuantums[revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE]; ok { | ||
| totalTakerFeeRevShareQuantums = value | ||
|
|
||
| // taker revshare is not returned if taker volume is greater than Max30dTakerVolumeQuantums |
There was a problem hiding this comment.
We aren't using Max30dRefereeVolumeQuantums below?
There was a problem hiding this comment.
We dont need to - since no taker revshares are returned over 50 million - the if condition is never hit for value > 0
There was a problem hiding this comment.
The new trading rewards logic is below:
If taker volume is <50M (equal statement: they would generate affiliate rev share if referred), then the max possible affiliate rev share is deducted from their trading rewards score.
The following implementation doesn't work correctly for a non-referred user under 50M volume
| @@ -144,8 +145,14 @@ func (k Keeper) AddRewardSharesForFill( | |||
| totalNetFeeRevSharePpm = value | |||
There was a problem hiding this comment.
Let's modify function documentation as needed
| @@ -483,7 +483,7 @@ func TestAddRewardSharesForFill(t *testing.T) { | |||
| }, | |||
| expectedTakerShare: types.RewardShare{ | |||
| Address: takerAddress, | |||
| Weight: dtypes.NewInt(900_000), // (2 - 0.1% * 800 - 0.2) * (1 - 0.1) | |||
| Weight: dtypes.NewInt(180_000), // (2 - 0.1% * 800 - 1) * (1 - 0.1) | |||
There was a problem hiding this comment.
Could we cover the following (may already be covered in current tests):
- Deduct
MaxPossibleAffiliateRevSharefor non-referred user <50M trailing volume - Deduct
MaxPossibleAffiliateRevSharefor referred user <50M trailing volume - Don't deduct
MaxPossibleAffiliateRevSharefor non-referred or referred user >=50M trailing volume
There was a problem hiding this comment.
first two are covered by existing tests
There was a problem hiding this comment.
wrote a test for the 3rd one
There was a problem hiding this comment.
Thanks, which one tests 1 (Deduct MaxPossibleAffiliateRevShare for non-referred user <50M trailing volume)?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
protocol/x/rewards/keeper/keeper_test.go (1)
486-486: Address suggested test cases from previous commentRegarding the previous comment by teddyding, it's important to ensure comprehensive test coverage. While the new test cases cover some aspects, please consider adding explicit tests for the following scenarios:
- Deduct
MaxPossibleAffiliateRevSharefor non-referred user <50M trailing volume- Deduct
MaxPossibleAffiliateRevSharefor referred user <50M trailing volume- Don't deduct
MaxPossibleAffiliateRevSharefor non-referred or referred user >=50M trailing volumeAdding these test cases will improve the robustness of the test suite and ensure that all edge cases are properly handled.
protocol/x/revshare/keeper/revshare_test.go (1)
507-507: LGTM! Consider adding a comment explaining the significance of this value.The change from
Max30dRefereeVolumeQuantumstoMaxReferee30dVolumeForAffiliateShareQuantumsis appropriate and more descriptive. It accurately reflects the purpose of this constant in the context of affiliate share calculations.Consider adding a brief comment explaining why we're using
MaxReferee30dVolumeForAffiliateShareQuantums + 1. This would help clarify that we're testing the case where the volume just exceeds the maximum threshold for affiliate share eligibility.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/x/revshare/keeper/revshare.go (1 hunks)
- protocol/x/revshare/keeper/revshare_test.go (1 hunks)
- protocol/x/revshare/types/constants.go (1 hunks)
- protocol/x/rewards/keeper/keeper.go (3 hunks)
- protocol/x/rewards/keeper/keeper_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/revshare/types/constants.go
- protocol/x/rewards/keeper/keeper.go
Additional comments not posted (2)
protocol/x/rewards/keeper/keeper_test.go (2)
493-547: New test case for high-volume traders looks goodThe new test case for monthly rolling taker volume > 50 million is a valuable addition. It covers an important scenario for high-volume traders and includes revenue sharing for both net fee and taker fee. The expected weights are:
- Taker weight:
(2 - 0.1% * 800 - 0) * (1 - 0.1)= 1,080,000- Maker weight:
1 * (1 - 0.1)= 900,000These calculations seem consistent with the intended logic. However, to ensure completeness:
- Consider adding a test case for a scenario just below the 50 million threshold to verify the boundary condition.
- Verify if there are any other volume-based thresholds that should be tested.
#!/bin/bash # Check for other volume thresholds in the codebase rg --type go "MonthlyRollingTakerVolumeQuantums"
486-491: Verify the calculation of taker and maker weightsThe calculation of taker and maker weights has been updated. Please ensure these calculations are correct:
- Taker weight:
(2 - 0.1% * 800 - 1) * (1 - 0.1)= 180,000- Maker weight:
1 * (1 - 0.1)= 900,000These calculations seem to account for fee reductions and revenue sharing. However, it's important to verify that these align with the intended business logic.
| @@ -227,7 +227,7 @@ func (k Keeper) getAffiliateRevShares( | |||
| ) ([]types.RevShare, error) { | |||
| takerAddr := fill.TakerAddr | |||
| takerFee := fill.TakerFeeQuoteQuantums | |||
| if fill.MonthlyRollingTakerVolumeQuantums >= types.Max30dRefereeVolumeQuantums { | |||
| if fill.MonthlyRollingTakerVolumeQuantums >= types.MaxReferee30dVolumeForAffiliateShareQuantums { | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Issues Detected with the Recent Change.
-
The old threshold constant
Max30dRefereeVolumeQuantumswas not found in the codebase. Please confirm if its removal is intentional. -
No existing test cases were identified for
getAffiliateRevSharesor the new thresholdMaxReferee30dVolumeForAffiliateShareQuantums. It is crucial to add appropriate tests to validate the new threshold's functionality and ensure no regressions.
Analysis chain
Approve change with suggestions for verification and documentation.
The update to use types.MaxReferee30dVolumeForAffiliateShareQuantums as the threshold for affiliate revenue shares looks good. This change appears to refine the eligibility criteria for affiliate shares based on the monthly rolling taker volume.
To ensure this change doesn't have unintended consequences:
- Please verify the relative values of
types.Max30dRefereeVolumeQuantums(old) andtypes.MaxReferee30dVolumeForAffiliateShareQuantums(new). - Confirm that this change aligns with the intended business logic for affiliate revenue sharing.
Consider updating any related documentation or comments that explain the affiliate revenue share eligibility criteria to reflect this change.
Ensure thorough testing of this change:
- Test cases where
MonthlyRollingTakerVolumeQuantumsis just below, at, and just above the new threshold. - Verify that the behavior for edge cases (e.g.,
MonthlyRollingTakerVolumeQuantums= 0 or very large values) remains correct.
If no relevant tests are found, please add appropriate test cases to cover this change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare the old and new threshold values
# Expected result: The new threshold should be intentionally different from the old one
# Search for the old constant
echo "Old threshold:"
rg --type go "Max30dRefereeVolumeQuantums\s*=\s*" --no-filename
# Search for the new constant
echo "New threshold:"
rg --type go "MaxReferee30dVolumeForAffiliateShareQuantums\s*=\s*" --no-filename
Length of output: 301
Script:
#!/bin/bash
# Description: Check for existing tests related to this threshold
# Expected result: There should be test cases covering various scenarios around the threshold
rg --type go -C 5 "TestGetAffiliateRevShares|TestMaxReferee30dVolumeForAffiliateShareQuantums" ./protocol/x/revshare
Length of output: 118
protocol/x/rewards/keeper/keeper.go
Outdated
| @@ -152,7 +153,7 @@ func (k Keeper) AddRewardSharesForFill( | |||
|
|
|||
| // taker revshare is not returned if taker volume is greater than Max30dTakerVolumeQuantums | |||
| if value, ok := revSharesForFill.FeeSourceToRevSharePpm[revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE]; ok && | |||
| value > 0 { | |||
| value > 0 && fill.MonthlyRollingTakerVolumeQuantums < revsharetypes.MaxReferee30dVolumeForAffiliateShareQuantums { | |||
| totalTakerFeeRevShareQuantums = lib.BigMulPpm(fill.TakerFeeQuoteQuantums, | |||
There was a problem hiding this comment.
Naming of variable is inaccurate, should be max_possible_taker_fee_rev_share
protocol/x/rewards/keeper/keeper.go
Outdated
| @@ -152,7 +153,7 @@ func (k Keeper) AddRewardSharesForFill( | |||
|
|
|||
| // taker revshare is not returned if taker volume is greater than Max30dTakerVolumeQuantums | |||
| if value, ok := revSharesForFill.FeeSourceToRevSharePpm[revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE]; ok && | |||
There was a problem hiding this comment.
Shouldn't we remove FeeSourceToRevSharePpm[revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE from the if condition and only rely on fill.MonthlyRollingTakerVolumeQuantums < revsharetypes.MaxReferee30dVolumeForAffiliateShareQuantums only?
There was a problem hiding this comment.
my b - didnt realize this when i was working through it fast 😓
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (2)
protocol/testing/e2e/trading_rewards/trading_rewards_test.go (2)
Line range hint
510-520: Consistent use of constants for reward amountsThe reward amounts
"3980734153846153845"for Alice and Bob are hardcoded. Consider defining these as constants or computing them programmatically to maintain consistency and facilitate updates.
Line range hint
530-540: Define reward amounts as constants or compute dynamicallyThe hardcoded reward amounts
"2281569230769230769"for Carl and Dave should be defined as constants or calculated dynamically to ensure accuracy and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/testing/e2e/trading_rewards/trading_rewards_test.go (13 hunks)
- protocol/x/rewards/keeper/keeper.go (4 hunks)
- protocol/x/rewards/keeper/keeper_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/rewards/keeper/keeper.go
- protocol/x/rewards/keeper/keeper_test.go
Additional comments not posted (3)
protocol/testing/e2e/trading_rewards/trading_rewards_test.go (3)
8-9: ApprovedThe added imports
sdkmathandprotoare appropriate for math operations and protocol buffer manipulations used in the updated test code.
281-284: Verify the calculation of the treasury balanceThe updated treasury balance
"28417709308164802417"seems to be based on previous calculations. Ensure that this value accurately reflects the expected treasury balance after rewards distribution.You can verify the treasury balance calculation with the following script:
#!/bin/bash # Description: Calculate the expected treasury balance. # Assume vested amount per block is 2.534006365423989945 full coins # Total vested over 12 blocks: 2.534006365423989945 * 12 # Subtract total rewards distributed to users from total vested amount # Expected treasury balance = (Total vested) - (Total rewards to users) echo "Expected treasury balance calculation needs to be done based on actual parameters."Please adjust the script with the accurate values from your test case.
486-487: Update comments to reflect accurate calculationsThe comments mention "Entitled reward tokens:
- Alice and Bob: 3.98073
- Carl and Dave: 2.28156"
Ensure that these values are consistent with the actual calculations in the code or update them to reflect the correct amounts.
To verify the correctness of these values, you can run the following script:
Note: Adjust the calculations as per the actual parameters used in the test.
| // Total of ~5.06 full coins have vested, calculated rewards are | ||
| // ~1.99 full coins. So remaining rewards are ~3.07 full coins. | ||
| Balance: big_testutil.MustFirst(new(big.Int).SetString( | ||
| "0", | ||
| "3077645653924902967", |
There was a problem hiding this comment.
Define magic numbers as constants for clarity
The hardcoded value "3077645653924902967" represents a significant numerical value in the balance calculations. Defining this value as a named constant would improve readability and maintainability.
| // starting balance + ~1.99 full coins rewards | ||
| Balance: new(big.Int).Add( | ||
| TestAccountStartingTokenBalance, | ||
| big_testutil.MustFirst(new(big.Int).SetString( | ||
| "5068012730847979890", | ||
| "1990367076923076923", |
There was a problem hiding this comment.
Define repeated magic numbers as constants
The value "1990367076923076923" is used multiple times in the code. Consider defining it as a constant to enhance code clarity and reduce potential errors due to manual updates.
| @@ -258,7 +259,7 @@ | |||
| TradingRewards: []*indexerevents.AddressTradingReward{ | |||
| { | |||
| Owner: constants.AliceAccAddress.String(), | |||
| DenomAmount: dtypes.NewIntFromUint64(5068012730847979890), | |||
| DenomAmount: dtypes.NewIntFromUint64(1990367076923076923), | |||
There was a problem hiding this comment.
Consistent use of constants for repeated values
The DenomAmount of 1990367076923076923 is hardcoded here. Defining this repeated value as a constant would improve consistency and maintainability across the test cases.
| // balance + ~25.34 full coins. Note this is exactly 10x the amount vested per block, | ||
| // since 10 blocks has passed since the last check. | ||
| Balance: big_testutil.MustFirst(new(big.Int).SetString( | ||
| "25340063654239899450", | ||
| "28417709308164802417", |
There was a problem hiding this comment.
Consider defining large numeric literals as constants
The hardcoded balance "28417709308164802417" could be defined as a constant to improve readability and make future updates easier.
| Balance: big_testutil.MustFirst(new(big.Int).SetString( | ||
| "22329476019663889395", | ||
| "28961348596665715439", |
There was a problem hiding this comment.
Define large numeric literals as constants
The hardcoded value "28961348596665715439" in the balance calculation represents a significant amount. Defining it as a constant would improve code readability and ease future maintenance.
| DenomAmount: dtypes.NewIntFromUint64(3980734153846153845), | ||
| }, | ||
| { | ||
| Owner: constants.AliceAccAddress.String(), | ||
| DenomAmount: dtypes.NewIntFromUint64(8053910091363583686), | ||
| DenomAmount: dtypes.NewIntFromUint64(3980734153846153845), | ||
| }, | ||
| { | ||
| Owner: constants.CarlAccAddress.String(), | ||
| DenomAmount: dtypes.NewIntFromUint64(4616121735756366038), | ||
| DenomAmount: dtypes.NewIntFromUint64(2281569230769230769), | ||
| }, | ||
| { | ||
| Owner: constants.DaveAccAddress.String(), | ||
| DenomAmount: dtypes.NewIntFromUint64(4616121735756366038), | ||
| DenomAmount: dtypes.NewIntFromUint64(2281569230769230769), |
There was a problem hiding this comment.
Avoid hardcoding values in event assertions
In the ExpectedTradingRewardEvents, the DenomAmount values are hardcoded. Calculating these values within the test or defining them as constants would reduce the risk of errors and improve test robustness.
| // Total rewards = (TakerFee - TakerVolume * MaxMakerRebate - (takerFee * MaxPossibleTakerFeeRevShare)) * 0.99 | ||
| // = ($28003 * 0.05% - $28003 * 0.011% - $28003 * 0.05% * 0.5) * 0.99 | ||
| // = ($14.0015 - $3.08033 - $7.00075) 0.99 = $3.8812158 | ||
| // Reward tokens = $3.8812158 / $1.95 = 1.9903670769 full coins | ||
| Balance: new(big.Int).Add( | ||
| TestAccountStartingTokenBalance, | ||
| big_testutil.MustFirst(new(big.Int).SetString( | ||
| "5544594000000000000", | ||
| "1990367076923076923", |
There was a problem hiding this comment.
Compute expected reward amounts programmatically
The calculation for the rewards is detailed in comments, but the final value "1990367076923076923" is hardcoded. To prevent discrepancies between the comments and the code, consider calculating this value programmatically within the test.
Apply this change to compute the reward amount:
Balance: new(big.Int).Add(
TestAccountStartingTokenBalance,
- big_testutil.MustFirst(new(big.Int).SetString(
- "1990367076923076923",
- 10,
- )),
+ calculateExpectedRewardAmount(),
),Where calculateExpectedRewardAmount() is a function that performs the reward calculation based on the parameters defined in the comments.
Committable suggestion was skipped due to low confidence.
| // = ($28003 * 0.05% - $28003 * 0.011%) * 0.99 | ||
| // = ($14.0015 - $3.08033) 0.99 = $10.8119583 | ||
| // Reward tokens = $10.8119583 / $1.95 = 5.544594 full coins | ||
| // Total rewards = (TakerFee - TakerVolume * MaxMakerRebate - (takerFee * MaxPossibleTakerFeeRevShare)) * 0.99 |
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes