[OTE-816] Update Revshare logic for affiliates #2298
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,7 +134,6 @@ func ListingKeepers( | |
| bankKeeper, | ||
| perpetualsKeeper, | ||
| blockTimeKeeper, | ||
| transientStoreKey, | ||
| true, | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,7 +3,6 @@ package keeper | |||||
| import ( | ||||||
| "math/big" | ||||||
|
|
||||||
| "cosmossdk.io/store/prefix" | ||||||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||||||
| "github.com/dydxprotocol/v4-chain/protocol/lib" | ||||||
|
|
@@ -163,25 +162,28 @@ func (k Keeper) GetAllRevShares( | |||||
| feeSourceToRevSharePpm := make(map[types.RevShareFeeSource]uint32) | ||||||
| feeSourceToQuoteQuantums[types.REV_SHARE_FEE_SOURCE_TAKER_FEE] = big.NewInt(0) | ||||||
| feeSourceToRevSharePpm[types.REV_SHARE_FEE_SOURCE_TAKER_FEE] = 0 | ||||||
| feeSourceToQuoteQuantums[types.REV_SHARE_FEE_SOURCE_NET_PROTOCOL_REVENUE] = big.NewInt(0) | ||||||
| feeSourceToRevSharePpm[types.REV_SHARE_FEE_SOURCE_NET_PROTOCOL_REVENUE] = 0 | ||||||
|
|
||||||
| totalFeesShared := big.NewInt(0) | ||||||
| takerFees := fill.TakerFeeQuoteQuantums | ||||||
| makerFees := fill.MakerFeeQuoteQuantums | ||||||
|
There was a problem hiding this comment. Let's also change
|
||||||
| netFees := big.NewInt(0).Add(takerFees, makerFees) | ||||||
|
|
||||||
| affiliateRevShares, affiliateFeesShared, err := k.getAffiliateRevShares(ctx, fill, affiliatesWhitelistMap) | ||||||
| if err != nil { | ||||||
| return types.RevSharesForFill{}, err | ||||||
|
There was a problem hiding this comment. Comment on caller function Instead propagating error returned from Order matching + rev share both working > Only order matching working > Order matching not working There was a problem hiding this comment. sounds good. I'll add some error logs to catch in case it fails There was a problem hiding this comment. Did we do this in this PR? Or plan to do in another one? There was a problem hiding this comment. This is done in the PR . Check here. Also updated tests in revshare_test.go There was a problem hiding this comment. Oh, the original comment was suggesting that we keep Changing We should either:
The first seems slightly cleaner to me since we just need one error log instead of multiple |
||||||
| } | ||||||
| netFeesSubAffiliateFeesShared := new(big.Int).Sub(netFees, affiliateFeesShared) | ||||||
| if netFeesSubAffiliateFeesShared.Sign() <= 0 { | ||||||
|
There was a problem hiding this comment. Nit: move to immediately below line 129 |
||||||
| return types.RevSharesForFill{}, types.ErrAffiliateFeesSharedExceedsNetFees | ||||||
| } | ||||||
|
|
||||||
| unconditionalRevShares, err := k.getUnconditionalRevShares(ctx, netFeesSubAffiliateFeesShared) | ||||||
teddyding marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| if err != nil { | ||||||
| return types.RevSharesForFill{}, err | ||||||
| } | ||||||
| marketMapperRevShares, err := k.getMarketMapperRevShare(ctx, fill.MarketId, netFeesSubAffiliateFeesShared) | ||||||
| if err != nil { | ||||||
| return types.RevSharesForFill{}, err | ||||||
| } | ||||||
|
|
@@ -208,8 +210,7 @@ func (k Keeper) GetAllRevShares( | |||||
| } | ||||||
| //check total fees shared is less than or equal to net fees | ||||||
| if totalFeesShared.Cmp(netFees) > 0 { | ||||||
| return types.RevSharesForFill{}, types.ErrTotalFeesSharedExceedsNetFees | ||||||
| } | ||||||
|
|
||||||
| return types.RevSharesForFill{ | ||||||
|
|
@@ -224,20 +225,20 @@ func (k Keeper) getAffiliateRevShares( | |||||
| ctx sdk.Context, | ||||||
| fill clobtypes.FillForProcess, | ||||||
| affiliatesWhitelistMap map[string]uint32, | ||||||
| ) ([]types.RevShare, *big.Int, error) { | ||||||
| takerAddr := fill.TakerAddr | ||||||
| takerFee := fill.TakerFeeQuoteQuantums | ||||||
| if fill.MonthlyRollingTakerVolumeQuantums >= types.MaxReferee30dVolumeForAffiliateShareQuantums { | ||||||
| return nil, big.NewInt(0), nil | ||||||
| } | ||||||
|
|
||||||
| takerAffiliateAddr, feeSharePpm, exists, err := k.affiliatesKeeper.GetTakerFeeShare( | ||||||
| ctx, takerAddr, affiliatesWhitelistMap) | ||||||
| if err != nil { | ||||||
| return nil, big.NewInt(0), err | ||||||
|
There was a problem hiding this comment. Return In the error case at line 237, consider returning Apply this diff to adjust the return value: -return nil, big.NewInt(0), err
+return nil, nil, errCommittable suggestion
Suggested change
|
||||||
| } | ||||||
| if !exists { | ||||||
| return nil, big.NewInt(0), nil | ||||||
| } | ||||||
| feesShared := lib.BigMulPpm(takerFee, lib.BigU(feeSharePpm), false) | ||||||
| return []types.RevShare{ | ||||||
|
|
@@ -248,23 +249,23 @@ func (k Keeper) getAffiliateRevShares( | |||||
| QuoteQuantums: feesShared, | ||||||
| RevSharePpm: feeSharePpm, | ||||||
| }, | ||||||
| }, feesShared, nil | ||||||
| } | ||||||
|
|
||||||
| func (k Keeper) getUnconditionalRevShares( | ||||||
| ctx sdk.Context, | ||||||
| netFeesSubAffiliateFeesShared *big.Int, | ||||||
| ) ([]types.RevShare, error) { | ||||||
| revShares := []types.RevShare{} | ||||||
| unconditionalRevShareConfig, err := k.GetUnconditionalRevShareConfigParams(ctx) | ||||||
| if err != nil { | ||||||
| return nil, err | ||||||
| } | ||||||
| for _, revShare := range unconditionalRevShareConfig.Configs { | ||||||
| feeShared := lib.BigMulPpm(netFeesSubAffiliateFeesShared, lib.BigU(revShare.SharePpm), false) | ||||||
| revShare := types.RevShare{ | ||||||
| Recipient: revShare.Address, | ||||||
| RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_PROTOCOL_REVENUE, | ||||||
| RevShareType: types.REV_SHARE_TYPE_UNCONDITIONAL, | ||||||
| QuoteQuantums: feeShared, | ||||||
| RevSharePpm: revShare.SharePpm, | ||||||
|
|
@@ -277,7 +278,7 @@ func (k Keeper) getUnconditionalRevShares( | |||||
| func (k Keeper) getMarketMapperRevShare( | ||||||
| ctx sdk.Context, | ||||||
| marketId uint32, | ||||||
| netFeesSubAffiliateFeesShared *big.Int, | ||||||
| ) ([]types.RevShare, error) { | ||||||
| revShares := []types.RevShare{} | ||||||
| marketMapperRevshareAddress, revenueSharePpm, err := k.GetMarketMapperRevenueShareForMarket(ctx, marketId) | ||||||
|
|
@@ -288,10 +289,10 @@ func (k Keeper) getMarketMapperRevShare( | |||||
| return nil, nil | ||||||
| } | ||||||
|
|
||||||
| marketMapperRevshareAmount := lib.BigMulPpm(netFeesSubAffiliateFeesShared, lib.BigU(revenueSharePpm), false) | ||||||
| revShares = append(revShares, types.RevShare{ | ||||||
| Recipient: marketMapperRevshareAddress.String(), | ||||||
| RevShareFeeSource: types.REV_SHARE_FEE_SOURCE_NET_PROTOCOL_REVENUE, | ||||||
| RevShareType: types.REV_SHARE_TYPE_MARKET_MAPPER, | ||||||
| QuoteQuantums: marketMapperRevshareAmount, | ||||||
| RevSharePpm: revenueSharePpm, | ||||||
|
|
||||||
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.
Consider revising the error handling strategy for
GetAllRevSharesThe addition of error handling for
GetAllRevSharesis a good practice. However, the current implementation might mask potential issues:revSharesForFillstruct when an error occurs.Consider the following suggestions:
revSharesForFillis appropriate in all scenarios.Here's a potential alternative implementation:
revSharesForFill, err := k.revshareKeeper.GetAllRevShares(ctx, fillForProcess, affiliatesWhitelistMap) if err != nil { - revSharesForFill = revsharetypes.RevSharesForFill{} - log.ErrorLog(ctx, "error getting rev shares for fill", "error", err) + // Log the error + log.ErrorLog(ctx, "error getting rev shares for fill", "error", err) + // Consider whether to return the error or continue with a flag + return takerUpdateResult, makerUpdateResult, affiliateRevSharesQuoteQuantums, + fmt.Errorf("failed to get rev shares: %w", err) }This approach would propagate the error, allowing the caller to decide how to handle it.