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.

Sign up for GitHub

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

Jump to bottom

[OTE-817] update trading rewards with new logic #2300

Merged
merged 13 commits into from
Sep 24, 2024
Merged

[OTE-817] update trading rewards with new logic #2300

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
213977b
update trading rewards
affanv14 Sep 19, 2024
368f359
add comment
affanv14 Sep 19, 2024
9e422bb
lint
affanv14 Sep 19, 2024
8750e0d
address comments
affanv14 Sep 19, 2024
f9d89c7
update logic
affanv14 Sep 23, 2024
61a4542
update comment
affanv14 Sep 23, 2024
972f274
lint
affanv14 Sep 23, 2024
3deb52e
update to quote quantums
affanv14 Sep 23, 2024
2184b6a
address comments
affanv14 Sep 23, 2024
581f861
update e2e tests
affanv14 Sep 23, 2024
6fada43
update comment
affanv14 Sep 23, 2024
15ba5e6
Update protocol/x/rewards/keeper/keeper.go
teddyding Sep 24, 2024
f4d0f93
lint
affanv14 Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion protocol/x/revshare/keeper/revshare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Issues Detected with the Recent Change.

  • The old threshold constant Max30dRefereeVolumeQuantums was not found in the codebase. Please confirm if its removal is intentional.

  • No existing test cases were identified for getAffiliateRevShares or the new threshold MaxReferee30dVolumeForAffiliateShareQuantums. 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:

  1. Please verify the relative values of types.Max30dRefereeVolumeQuantums (old) and types.MaxReferee30dVolumeForAffiliateShareQuantums (new).
  2. 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:

  1. Test cases where MonthlyRollingTakerVolumeQuantums is just below, at, and just above the new threshold.
  2. 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

return nil, nil
}

Expand Down
2 changes: 1 addition & 1 deletion protocol/x/revshare/keeper/revshare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ func TestKeeper_GetAllRevShares_Valid(t *testing.T) {
MakerFeeQuoteQuantums: big.NewInt(2_000_000),
FillQuoteQuantums: big.NewInt(100_000_000_000),
ProductId: perpetualId,
MonthlyRollingTakerVolumeQuantums: types.Max30dRefereeVolumeQuantums + 1,
MonthlyRollingTakerVolumeQuantums: types.MaxReferee30dVolumeForAffiliateShareQuantums + 1,
MarketId: marketId,
},
expectedRevSharesForFill: types.RevSharesForFill{
Expand Down
4 changes: 2 additions & 2 deletions protocol/x/revshare/types/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package types

const (
// 25 million USDC
Max30dRefereeVolumeQuantums = uint64(25_000_000_000_000)
// 50 million USDC
MaxReferee30dVolumeForAffiliateShareQuantums = uint64(50_000_000_000_000)
)
20 changes: 16 additions & 4 deletions protocol/x/rewards/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/dydxprotocol/v4-chain/protocol/lib"
"github.com/dydxprotocol/v4-chain/protocol/lib/log"
"github.com/dydxprotocol/v4-chain/protocol/lib/metrics"
affiliatetypes "github.com/dydxprotocol/v4-chain/protocol/x/affiliates/types"
clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
revsharetypes "github.com/dydxprotocol/v4-chain/protocol/x/revshare/types"
"github.com/dydxprotocol/v4-chain/protocol/x/rewards/types"
Expand Down Expand Up @@ -120,15 +121,20 @@ func (k Keeper) GetRewardShare(
//
// Within each block, total reward share score for an address is defined as:
//
// reward_share_score = total_taker_fees_paid - total_rev_shared_taker_fee
// reward_share_score = total_taker_fees_paid - max_possible_taker_fee_rev_share
// - max_possible_maker_rebate * taker_volume + total_positive_maker_fees - total_rev_shared_maker_fee
//
// Hence, for each fill, increment reward share score as follow:
// - Let F = sum(percentages of general rev-share) (excluding taker only rev share i.e. affiliate)
// - For maker address, positive_maker_fees * (1 - F) are added to reward share score.
// - For taker address, (positive_taker_fees - max_possible_maker_rebate
// * fill_quote_quantum - taker_fee_rev_share) * (1 - F)
// * fill_quote_quantum - max_possible_taker_fee_rev_share) * (1 - F)
// are added to reward share score.
// max_possible_taker_fee_rev_share is 0 when taker trailing volume is > MaxReferee30dVolumeForAffiliateShareQuantums,
// since taker_fee_share is only affiliate at the moment, and they don’t generate affiliate rev share.
// When taker volume ≤ MaxReferee30dVolumeForAffiliateShareQuantums,
// max_possible_taker_fee_rev_share = max_vip_affiliate_share * taker_fee
// regardless of if the taker has an affiliate or not.

func (k Keeper) AddRewardSharesForFill(
ctx sdk.Context,
Expand All @@ -144,8 +150,14 @@ func (k Keeper) AddRewardSharesForFill(
totalNetFeeRevSharePpm = value
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's modify function documentation as needed

}
totalTakerFeeRevShareQuantums := big.NewInt(0)
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
Copy link
Contributor

Choose a reason for hiding this comment

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

We aren't using Max30dRefereeVolumeQuantums below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We dont need to - since no taker revshares are returned over 50 million - the if condition is never hit for value > 0

Copy link
Contributor

@teddyding teddyding Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

if value, ok := revSharesForFill.FeeSourceToRevSharePpm[revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE]; ok &&
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@affanv14 affanv14 Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

my b - didnt realize this when i was working through it fast 😓

value > 0 && fill.MonthlyRollingTakerVolumeQuantums < revsharetypes.MaxReferee30dVolumeForAffiliateShareQuantums {
totalTakerFeeRevShareQuantums = lib.BigMulPpm(fill.TakerFeeQuoteQuantums,
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming of variable is inaccurate, should be max_possible_taker_fee_rev_share

lib.BigU(affiliatetypes.AffiliatesRevSharePpmCap),
false,
)
}

totalFeeSubNetRevSharePpm := lib.OneMillion - totalNetFeeRevSharePpm
Expand Down
63 changes: 62 additions & 1 deletion protocol/x/rewards/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,68 @@ 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)
Copy link
Contributor

@teddyding teddyding Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could we cover the following (may already be covered in current tests):

  • Deduct MaxPossibleAffiliateRevShare for non-referred user <50M trailing volume
  • Deduct MaxPossibleAffiliateRevShare for referred user <50M trailing volume
  • Don't deduct MaxPossibleAffiliateRevShare for non-referred or referred user >=50M trailing volume

Copy link
Contributor Author

Choose a reason for hiding this comment

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

first two are covered by existing tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrote a test for the 3rd one

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, which one tests 1 (Deduct MaxPossibleAffiliateRevShare for non-referred user <50M trailing volume)?

},
expectedMakerShare: types.RewardShare{
Address: makerAddress,
Weight: dtypes.NewInt(900_000), // 1 * (1 - 0.1)
},
},
"positive maker + taker fees reduced by maker rebate, taker + net fee revshare, monthly rolling taker volume > 50 mil": {
prevTakerRewardShare: nil,
prevMakerRewardShare: nil,
fill: clobtypes.FillForProcess{
TakerAddr: takerAddress,
TakerFeeQuoteQuantums: big.NewInt(2_000_000),
MakerAddr: makerAddress,
MakerFeeQuoteQuantums: big.NewInt(1_000_000),
FillQuoteQuantums: big.NewInt(800_000_000),
ProductId: uint32(1),
MarketId: uint32(1),
MonthlyRollingTakerVolumeQuantums: 50_000_000_000,
},
revSharesForFill: revsharetypes.RevSharesForFill{
AllRevShares: []revsharetypes.RevShare{
{
Recipient: constants.AliceAccAddress.String(),
RevShareFeeSource: revsharetypes.REV_SHARE_FEE_SOURCE_NET_FEE,
RevShareType: revsharetypes.REV_SHARE_TYPE_UNCONDITIONAL,
QuoteQuantums: big.NewInt(200_000),
RevSharePpm: 100_000, // 10%
},
{
Recipient: takerAddress,
RevShareFeeSource: revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE,
RevShareType: revsharetypes.REV_SHARE_TYPE_AFFILIATE,
QuoteQuantums: big.NewInt(200_000),
RevSharePpm: 100_000, // 10%
},
},
FeeSourceToQuoteQuantums: map[revsharetypes.RevShareFeeSource]*big.Int{
revsharetypes.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(200_000),
revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE: big.NewInt(200_000),
},
FeeSourceToRevSharePpm: map[revsharetypes.RevShareFeeSource]uint32{
revsharetypes.REV_SHARE_FEE_SOURCE_NET_FEE: 100_000, // 10%
revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE: 100_000, // 10%
},
AffiliateRevShare: &revsharetypes.RevShare{
Recipient: takerAddress,
RevShareFeeSource: revsharetypes.REV_SHARE_FEE_SOURCE_TAKER_FEE,
RevShareType: revsharetypes.REV_SHARE_TYPE_AFFILIATE,
QuoteQuantums: big.NewInt(200_000),
RevSharePpm: 100_000, // 10%
},
},
feeTiers: []*feetierstypes.PerpetualFeeTier{
{
MakerFeePpm: -1_000, // -0.1%
TakerFeePpm: 2_000, // 0.2%
},
},
expectedTakerShare: types.RewardShare{
Address: takerAddress,
Weight: dtypes.NewInt(1_080_000), // (2 - 0.1% * 800 - 0) * (1 - 0.1)
},
expectedMakerShare: types.RewardShare{
Address: makerAddress,
Expand Down
Loading