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-816] Update Revshare logic for affiliates #2298

Merged
merged 14 commits into from
Sep 24, 2024
Merged

[OTE-816] Update Revshare logic for affiliates #2298

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
809a0ce
update revshare logic
affanv14 Sep 19, 2024
b8d0f0d
Merge remote-tracking branch 'origin/main' into affan/update-revshare
affanv14 Sep 19, 2024
5c2fa13
fix test
affanv14 Sep 19, 2024
2060d21
update comment
affanv14 Sep 19, 2024
3f21cf6
rename
affanv14 Sep 19, 2024
f162535
address comments
affanv14 Sep 23, 2024
71e434a
changes
affanv14 Sep 23, 2024
ab729a3
remove unused param
affanv14 Sep 23, 2024
5fcdffa
update
affanv14 Sep 23, 2024
e69bbb7
Merge remote-tracking branch 'origin/main' into affan/update-revshare
affanv14 Sep 23, 2024
962942e
nits
affanv14 Sep 23, 2024
75b30fc
address comment
affanv14 Sep 24, 2024
53b26b2
add error
affanv14 Sep 24, 2024
e98790b
Merge remote-tracking branch 'origin/main' into affan/update-revshare
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
18 changes: 9 additions & 9 deletions protocol/x/revshare/keeper/revshare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,17 +171,17 @@ func (k Keeper) GetAllRevShares(
makerFees := fill.MakerFeeQuoteQuantums
Copy link
Contributor

@teddyding teddyding Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's also change REV_SHARE_FEE_SOURCE_NET_FEE to REV_SHARE_FEE_SOURCE_NET_PROTOCOL_REVENUE to disambiguate

NET_FEE could be understood as taker fee minus rebates

netFees := big.NewInt(0).Add(takerFees, makerFees)

affiliateRevShares, err := k.getAffiliateRevShares(ctx, fill, affiliatesWhitelistMap)
affiliateRevShares, affiliateFeesShared, err := k.getAffiliateRevShares(ctx, fill, affiliatesWhitelistMap)
if err != nil {
return types.RevSharesForFill{}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on caller function persistMatchedOrders:

Instead propagating error returned from GetAllRevShares (which would ultimately fail the match transaction), it's better to just pass in empty RevSharesForFill to DistributeFees. This way if any rev share calculation encounters issue, the chain will continue to be able to process matches (without rev share).

Order matching + rev share both working > Only order matching working > Order matching not working

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good. I'll add some error logs to catch in case it fails

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we do this in this PR? Or plan to do in another one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done in the PR . Check here. Also updated tests in revshare_test.go

Copy link
Contributor

@teddyding teddyding Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, the original comment was suggesting that we keep GetAllRevShares behavior as this but handle error differently in persistMatchedOrders, since it's the caller responsibility to determine what to do with the returned error from GetAllRevShares

Changing GetAllRevShares behavior works too but we are changing the signature/expectation of the function (e.g. it doesn't make sense to have this function return error anymore, it's always nil)

We should either:

  • Keep GetAllRevShares as is, and just log out error (don't propogate errors further up) in persistMatchedOrders.
  • Fully change signature of GetAllRevShares so it doesn't return errors anymore, maybe even rename it to GetAllValidRevShares

The first seems slightly cleaner to me since we just need one error log instead of multiple

}

unconditionalRevShares, err := k.getUnconditionalRevShares(ctx, netFees)
netFeesSubAffiliateFeesShared := big.NewInt(0).Sub(netFees, affiliateFeesShared)
teddyding marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
netFeesSubAffiliateFeesShared := big.NewInt(0).Sub(netFees, affiliateFeesShared)
netFeesSubAffiliateFeesShared := new(big.Int).Sub(
netFees,
affiliateFeesShared,
)

unconditionalRevShares, err := k.getUnconditionalRevShares(ctx, netFeesSubAffiliateFeesShared)
Copy link
Contributor

Choose a reason for hiding this comment

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

Verify that netFeesSubAffiliateFeesShared is non-negative after subtraction

Subtracting affiliateFeesShared from netFees may result in netFeesSubAffiliateFeesShared becoming negative if affiliateFeesShared exceeds netFees. This could lead to unexpected behavior in subsequent revenue share calculations.

Consider adding a check to ensure netFeesSubAffiliateFeesShared is not negative, or handle scenarios where it could be zero or negative appropriately.

teddyding marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return types.RevSharesForFill{}, err
}

marketMapperRevShares, err := k.getMarketMapperRevShare(ctx, fill.MarketId, netFees)
marketMapperRevShares, err := k.getMarketMapperRevShare(ctx, fill.MarketId, netFeesSubAffiliateFeesShared)
if err != nil {
return types.RevSharesForFill{}, err
}
Expand Down Expand Up @@ -224,20 +224,20 @@ func (k Keeper) getAffiliateRevShares(
ctx sdk.Context,
fill clobtypes.FillForProcess,
affiliatesWhitelistMap map[string]uint32,
) ([]types.RevShare, error) {
) ([]types.RevShare, *big.Int, error) {
takerAddr := fill.TakerAddr
takerFee := fill.TakerFeeQuoteQuantums
if fill.MonthlyRollingTakerVolumeQuantums >= types.Max30dRefereeVolumeQuantums {
return nil, nil
return nil, big.NewInt(0), nil
}

takerAffiliateAddr, feeSharePpm, exists, err := k.affiliatesKeeper.GetTakerFeeShare(
ctx, takerAddr, affiliatesWhitelistMap)
if err != nil {
return nil, err
return nil, big.NewInt(0), err
Copy link
Contributor

Choose a reason for hiding this comment

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

Return nil for affiliateFeesShared when an error occurs

In the error case at line 237, consider returning nil for affiliateFeesShared instead of big.NewInt(0). This makes it clear that the value is invalid due to the error, aligning with common Go practices.

Apply this diff to adjust the return value:

-return nil, big.NewInt(0), err
+return nil, nil, err
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return nil, big.NewInt(0), err
return nil, nil, err

}
if !exists {
return nil, nil
return nil, big.NewInt(0), nil
}
feesShared := lib.BigMulPpm(takerFee, lib.BigU(feeSharePpm), false)
return []types.RevShare{
Expand All @@ -248,7 +248,7 @@ func (k Keeper) getAffiliateRevShares(
QuoteQuantums: feesShared,
RevSharePpm: feeSharePpm,
},
}, nil
}, feesShared, nil
}

func (k Keeper) getUnconditionalRevShares(
Expand Down
76 changes: 15 additions & 61 deletions protocol/x/revshare/keeper/revshare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,21 +334,21 @@ func TestKeeper_GetAllRevShares_Valid(t *testing.T) {
Recipient: constants.BobAccAddress.String(),
RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_FEE,
RevShareType: types.REV_SHARE_TYPE_UNCONDITIONAL,
QuoteQuantums: big.NewInt(2_400_000),
QuoteQuantums: big.NewInt(2_100_000),
RevSharePpm: 200_000,
},
{
Recipient: constants.AliceAccAddress.String(),
RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_FEE,
RevShareType: types.REV_SHARE_TYPE_UNCONDITIONAL,
QuoteQuantums: big.NewInt(3_600_000),
QuoteQuantums: big.NewInt(3_150_000),
RevSharePpm: 300_000,
},
{
Recipient: constants.AliceAccAddress.String(),
RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_FEE,
RevShareType: types.REV_SHARE_TYPE_MARKET_MAPPER,
QuoteQuantums: big.NewInt(1_200_000),
QuoteQuantums: big.NewInt(1_050_000),
RevSharePpm: 100_000,
},
},
Expand All @@ -361,7 +361,7 @@ func TestKeeper_GetAllRevShares_Valid(t *testing.T) {
},
FeeSourceToQuoteQuantums: map[types.RevShareFeeSource]*big.Int{
types.REV_SHARE_FEE_SOURCE_TAKER_FEE: big.NewInt(1_500_000),
types.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(7_200_000),
types.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(6_300_000),
},
FeeSourceToRevSharePpm: map[types.RevShareFeeSource]uint32{
types.REV_SHARE_FEE_SOURCE_TAKER_FEE: 150_000,
Expand Down Expand Up @@ -422,27 +422,27 @@ func TestKeeper_GetAllRevShares_Valid(t *testing.T) {
Recipient: constants.BobAccAddress.String(),
RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_FEE,
RevShareType: types.REV_SHARE_TYPE_UNCONDITIONAL,
QuoteQuantums: big.NewInt(1_600_000),
QuoteQuantums: big.NewInt(1_300_000),
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.

At least for one of the test cases, could we add comment on math behind these expected values? For example,

						QuoteQuantums:     big.NewInt(1_300_000), // (10 - 2 - 10 * 0.15) * 20% = 1.3

RevSharePpm: 200_000,
},
{
Recipient: constants.AliceAccAddress.String(),
RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_FEE,
RevShareType: types.REV_SHARE_TYPE_UNCONDITIONAL,
QuoteQuantums: big.NewInt(2_400_000),
QuoteQuantums: big.NewInt(1_950_000),
RevSharePpm: 300_000,
},
{
Recipient: constants.AliceAccAddress.String(),
RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_FEE,
RevShareType: types.REV_SHARE_TYPE_MARKET_MAPPER,
QuoteQuantums: big.NewInt(800_000),
QuoteQuantums: big.NewInt(650_000),
RevSharePpm: 100_000,
},
},
FeeSourceToQuoteQuantums: map[types.RevShareFeeSource]*big.Int{
types.REV_SHARE_FEE_SOURCE_TAKER_FEE: big.NewInt(1_500_000),
types.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(4_800_000),
types.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(3_900_000),
},
FeeSourceToRevSharePpm: map[types.RevShareFeeSource]uint32{
types.REV_SHARE_FEE_SOURCE_TAKER_FEE: 150_000,
Expand Down Expand Up @@ -584,12 +584,12 @@ func TestKeeper_GetAllRevShares_Valid(t *testing.T) {
Recipient: constants.AliceAccAddress.String(),
RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_FEE,
RevShareType: types.REV_SHARE_TYPE_MARKET_MAPPER,
QuoteQuantums: big.NewInt(1_200_000),
QuoteQuantums: big.NewInt(1_050_000),
RevSharePpm: 100_000, // 10%
},
},
FeeSourceToQuoteQuantums: map[types.RevShareFeeSource]*big.Int{
types.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(1_200_000),
types.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(1_050_000),
types.REV_SHARE_FEE_SOURCE_TAKER_FEE: big.NewInt(1_500_000),
},
FeeSourceToRevSharePpm: map[types.RevShareFeeSource]uint32{
Expand Down Expand Up @@ -645,12 +645,12 @@ func TestKeeper_GetAllRevShares_Valid(t *testing.T) {
Recipient: constants.BobAccAddress.String(),
RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_FEE,
RevShareType: types.REV_SHARE_TYPE_UNCONDITIONAL,
QuoteQuantums: big.NewInt(2_400_000),
QuoteQuantums: big.NewInt(2_100_000),
RevSharePpm: 200_000, // 20%
},
},
FeeSourceToQuoteQuantums: map[types.RevShareFeeSource]*big.Int{
types.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(2_400_000),
types.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(2_100_000),
types.REV_SHARE_FEE_SOURCE_TAKER_FEE: big.NewInt(1_500_000),
},
FeeSourceToRevSharePpm: map[types.RevShareFeeSource]uint32{
Expand Down Expand Up @@ -707,7 +707,7 @@ func TestKeeper_GetAllRevShares_Valid(t *testing.T) {
Recipient: constants.AliceAccAddress.String(),
RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_FEE,
RevShareType: types.REV_SHARE_TYPE_MARKET_MAPPER,
QuoteQuantums: big.NewInt(1_200_000),
QuoteQuantums: big.NewInt(950_000),
RevSharePpm: 100_000, // 10%
},
},
Expand All @@ -719,7 +719,7 @@ func TestKeeper_GetAllRevShares_Valid(t *testing.T) {
RevSharePpm: 250_000, // 25%
},
FeeSourceToQuoteQuantums: map[types.RevShareFeeSource]*big.Int{
types.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(1_200_000),
types.REV_SHARE_FEE_SOURCE_NET_FEE: big.NewInt(950_000),
types.REV_SHARE_FEE_SOURCE_TAKER_FEE: big.NewInt(2_500_000),
},
FeeSourceToRevSharePpm: map[types.RevShareFeeSource]uint32{
Expand Down Expand Up @@ -845,7 +845,7 @@ func TestKeeper_GetAllRevShares_Invalid(t *testing.T) {
Configs: []types.UnconditionalRevShareConfig_RecipientConfig{
{
Address: constants.BobAccAddress.String(),
SharePpm: 150_000, // 15%
SharePpm: 250_000, // 25%
},
},
})
Expand All @@ -856,52 +856,6 @@ func TestKeeper_GetAllRevShares_Invalid(t *testing.T) {
require.NoError(t, err)
},
},
{
name: "Total fees shared exceeds net fees from market mapper and affiliates",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests wont work anymore

revenueSharePpmNetFees: 950_000, // 95%,
revenueSharePpmTakerFees: 150_000, // 15%
expectedError: types.ErrTotalFeesSharedExceedsNetFees,
monthlyRollingTakerVolumeQuantums: 1_000_000_000_000, // 1 million USDC
setup: func(tApp *testapp.TestApp, ctx sdk.Context, keeper *keeper.Keeper,
affiliatesKeeper *affiliateskeeper.Keeper) {
err := keeper.SetMarketMapperRevenueShareParams(ctx, types.MarketMapperRevenueShareParams{
Address: constants.AliceAccAddress.String(),
RevenueSharePpm: 950_000, // 95%
ValidDays: 1,
})
require.NoError(t, err)

err = affiliatesKeeper.UpdateAffiliateTiers(ctx, affiliatetypes.DefaultAffiliateTiers)
require.NoError(t, err)
err = affiliatesKeeper.RegisterAffiliate(ctx, constants.AliceAccAddress.String(),
constants.BobAccAddress.String())
require.NoError(t, err)
},
},
{
name: "Total fees shared exceeds net fees from affiliates and unconditional rev shares",
revenueSharePpmNetFees: 950_000, // 95%,
revenueSharePpmTakerFees: 150_000, // 15%
expectedError: types.ErrTotalFeesSharedExceedsNetFees,
monthlyRollingTakerVolumeQuantums: 1_000_000_000_000, // 1 million USDC
setup: func(tApp *testapp.TestApp, ctx sdk.Context, keeper *keeper.Keeper,
affiliatesKeeper *affiliateskeeper.Keeper) {
keeper.SetUnconditionalRevShareConfigParams(ctx, types.UnconditionalRevShareConfig{
Configs: []types.UnconditionalRevShareConfig_RecipientConfig{
{
Address: constants.BobAccAddress.String(),
SharePpm: 950_000, // 95%
},
},
})

err := affiliatesKeeper.UpdateAffiliateTiers(ctx, affiliatetypes.DefaultAffiliateTiers)
require.NoError(t, err)
err = affiliatesKeeper.RegisterAffiliate(ctx, constants.AliceAccAddress.String(),
constants.BobAccAddress.String())
require.NoError(t, err)
},
},
{
name: "Total fees shared exceeds net fees - no affiliate rev shares",
revenueSharePpmNetFees: 1_150_000, // 115%,
Expand Down
6 changes: 3 additions & 3 deletions protocol/x/subaccounts/keeper/transfer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1500,10 +1500,10 @@ func TestDistributeFees(t *testing.T) {
MonthlyRollingTakerVolumeQuantums: 1_000_000,
},
expectedSubaccountsModuleAccBalance: big.NewInt(100), // 600 - 500
expectedFeeModuleAccBalance: big.NewInt(2888), // 2500 + 500 - 50
expectedMarketMapperAccBalance: big.NewInt(50), // 10% of 500
expectedFeeModuleAccBalance: big.NewInt(2892), // 2500 + 500 - 108
Copy link
Contributor

Choose a reason for hiding this comment

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

Realized that we aren't purely testing DistributeFees function in here, but it's more of an e2e test on affiliate + rev share + distribute fees. This isn't the best practice - should have a function that exclusively tests DistributeFees, which doesn't rely on affiliatesKeeper or revshareKeeper. (We should also remove delete the revShareKeeper on subaccountsKeeper

expectedMarketMapperAccBalance: big.NewInt(48), // 10% of 488
expectedAffiliateAccBalance: big.NewInt(12), // 5% of 250
expectedUnconditionalRevShareAccBalance: big.NewInt(50), // 10% of 500
expectedUnconditionalRevShareAccBalance: big.NewInt(48), // 10% of 488
collateralPoolAddr: authtypes.NewModuleAddress(
types.ModuleName + ":" + lib.IntToString(4),
),
Expand Down
Loading