-
Notifications
You must be signed in to change notification settings - Fork 104
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
[OTE-788] Update revshare safety #2284
Conversation
WalkthroughThe changes introduced in this pull request primarily focus on enhancing the initialization and interdependencies of various keeper components within the protocol's application. Key modifications include the reordering of keeper initializations, the addition of new fields and methods for fee management, and updates to validation functions to incorporate additional fee parameters. These adjustments aim to improve the overall structure and functionality of the affiliate and revenue sharing systems. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
protocol/x/feetiers/keeper/keeper.go
Outdated
@@ -131,3 +131,16 @@ func (k Keeper) GetLowestMakerFee(ctx sdk.Context) int32 { | |||
|
|||
return lowestMakerFee | |||
} | |||
|
|||
func (k Keeper) GetHighestTakerFee(ctx sdk.Context) int32 { |
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.
Can we add a unit test?
affiliateTiers affiliatetypes.AffiliateTiers, | ||
unconditionalRevShareConfig types.UnconditionalRevShareConfig, | ||
marketMapperRevShareParams types.MarketMapperRevenueShareParams, | ||
) bool { | ||
highestTakerFeeSharePpm := k.feetiersKeeper.GetHighestTakerFee(ctx) |
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.
Ideally we stick to the same pattern and pass in everything as input so this function is static. Since technically you want to call ValidateRevShareSafety
before updating fee tiers too. Wdyt?
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.
Wrong variable name: this is just taker fee - not relevant to any fee share
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.
Agreed that it would be better to check fee tiers as well - dont want to rely on someone remembering these
@@ -124,15 +125,17 @@ func (k Keeper) SetUnconditionalRevShareConfigParams(ctx sdk.Context, config typ | |||
} | |||
|
|||
// ValidateRevShareSafety roughly checks if the total rev share is valid using the formula below |
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.
// ValidateRevShareSafety roughly checks if the total rev share is valid using the formula below | |
// ValidateRevShareSafety roughly checks if the total rev share is valid using the formula below: |
// Note: this is just an estimate as affiliate rev share is based on taker fees, while | ||
// the rest of the rev share is based on net fees. | ||
// TODO(OTE-788): Revisit this formula to ensure accuracy. | ||
// highest_affiliate_taker_share * (highest_taker_fee_share_ppm / |
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.
Nit: add new line and indents to make formula more readable:
// ValidateRevShareSafety roughly checks if the total rev share is valid using the formula below
//
// highest_affiliate_taker_share * (highest_taker_fee_share_ppm /
// (lowest_maker_fee_share_ppm + highest_taker_fee_share_ppm))
// sum(unconditional_rev_shares) + market_mapper_rev_share < 100%
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.
(lowest_maker_fee_share_ppm + highest_taker_fee_share_ppm)
I think you meanmaker_fee
andtaker_fee
here, instead of rev share- Since maker_rebate is negative, I think you want
highest_affiliate_taker_share * lowest_taker_fee / (lowest_maker_fee + lowest_taker_feee)
which gives you the highest possible effect of affiliate rev share on net fee
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.
I agree... just realized that lowest taker fee and lower maker fee would also probably be in the same tier
@@ -257,10 +257,10 @@ func TestValidateRevShareSafety(t *testing.T) { | |||
for name, tc := range tests { |
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.
Can we add some new test case that would've passed before, but now fail the new requirement?
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
protocol/x/revshare/keeper/revshare.go (1)
127-130
: Improve the function documentation.The updated formula looks good. However, please consider the following suggestions to improve the function documentation based on past review comments:
- Add a new line and indents to make the formula more readable.
- Use the correct variable names in the formula description. For example, use
lowest_taker_fee
andlowest_maker_fee
instead oflowest_taker_fee_share_ppm
andlowest_maker_fee_share_ppm
.// ValidateRevShareSafety roughly checks if the total rev share is valid using the formula below: // // highest_affiliate_taker_share * (lowest_taker_fee / // (lowest_maker_fee + lowest_taker_fee)) // + sum(unconditional_rev_shares) + market_mapper_rev_share < 100%
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- protocol/app/app.go (1 hunks)
- protocol/x/affiliates/keeper/keeper.go (2 hunks)
- protocol/x/affiliates/keeper/msg_server.go (1 hunks)
- protocol/x/affiliates/types/expected_keepers.go (1 hunks)
- protocol/x/feetiers/keeper/keeper.go (2 hunks)
- protocol/x/feetiers/keeper/params.go (2 hunks)
- protocol/x/feetiers/types/errors.go (1 hunks)
- protocol/x/feetiers/types/expected_keepers.go (2 hunks)
- protocol/x/feetiers/types/types.go (1 hunks)
- protocol/x/revshare/keeper/msg_set_marketmapper_revenue_share.go (1 hunks)
- protocol/x/revshare/keeper/msg_update_unconditional_revshare_config.go (1 hunks)
- protocol/x/revshare/keeper/revshare.go (3 hunks)
- protocol/x/revshare/keeper/revshare_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/app/app.go
- protocol/x/affiliates/keeper/msg_server.go
- protocol/x/affiliates/types/expected_keepers.go
- protocol/x/revshare/keeper/msg_set_marketmapper_revenue_share.go
Additional context used
Learnings (1)
protocol/x/feetiers/types/expected_keepers.go (1)
Learnt from: teddyding PR: dydxprotocol/v4-chain#2141 File: protocol/x/affiliates/types/expected_keepers.go:9-11 Timestamp: 2024-08-22T20:17:56.228Z Learning: Always use pointers for `big.Int` return types in interfaces to avoid unnecessary copying.
Additional comments not posted (22)
protocol/x/feetiers/types/types.go (1)
9-9
: LGTM!The addition of the
GetAffiliateRefereeLowestTakerFee
method to theFeeTiersKeeper
interface is a valid enhancement. It expands the interface's capabilities by providing a way to retrieve the lowest taker fee for an affiliate referee.The method signature is consistent with the existing methods, and the name clearly conveys its purpose. The
sdk.Context
parameter andint32
return type align with the method's intended functionality.This change maintains backward compatibility as it doesn't modify the existing interface methods.
protocol/x/feetiers/types/errors.go (1)
33-37
: LGTM!The new error constant
ErrRevShareSafetyViolation
is correctly registered with a unique error code and a descriptive message. The addition enhances the error handling capabilities of the module by providing a specific error type for revenue sharing safety violations.The error constant is added in the correct location within the file and follows the existing pattern of error constant declarations.
protocol/x/revshare/keeper/msg_update_unconditional_revshare_config.go (2)
36-37
: LGTM!The code segment correctly retrieves the lowest taker fee and lowest maker fee from the
feetiersKeeper
.
40-47
: Excellent enhancement to the safety validation!The code segment correctly invokes the
ValidateRevShareSafety
method with the additional parameterslowestTakerFee
andlowestMakerFee
. This enhances the safety validation by considering the lowest taker fee and lowest maker fee, improving the robustness of the revenue sharing mechanism.protocol/x/feetiers/types/expected_keepers.go (4)
12-13
: LGTM!The return types have been updated to use more specific types from the
statstypes
package, which improves the modularity and clarity of the code.
24-25
: LGTM!The new
GetAllAffiliateTiers
method expands the functionality of theAffiliatesKeeper
interface, allowing for the retrieval of all affiliate tiers.
28-40
: LGTM!The new
RevShareKeeper
interface introduces well-defined revenue sharing functionality to the module, including methods for retrieving configuration parameters and validating the safety of the revenue share configuration.
Line range hint
1-40
: Skipping the learning.The provided learning about using pointers for
big.Int
return types in interfaces is not directly applicable to the code changes in this file, as the changes do not involve anybig.Int
return types.protocol/x/feetiers/keeper/params.go (5)
33-42
: LGTM!The logic for finding the lowest maker and taker fees from the tiers is implemented correctly.
43-46
: LGTM!The retrieval of affiliate tiers and error handling are implemented correctly.
48-51
: LGTM!The retrieval of the unconditional revenue share configuration and error handling are implemented correctly.
53-56
: LGTM!The retrieval of the market mapper revenue share parameters and error handling are implemented correctly.
58-65
: LGTM!The validation of revenue share safety is implemented correctly, taking into account all the necessary components. The error handling for validation failure is appropriate.
protocol/x/feetiers/keeper/keeper.go (2)
136-144
: LGTM!The function correctly retrieves the lowest taker fee for referees and includes a safety check to ensure the required number of tiers is met.
146-148
: LGTM!The function correctly sets the
RevShareKeeper
instance in theKeeper
, enabling interaction with the revenue sharing functionality.protocol/x/affiliates/keeper/keeper.go (2)
259-261
: LGTM!The
SetFeetiersKeeper
method looks good. It follows the naming convention and provides a way to set thefeetiersKeeper
field of theKeeper
struct.
27-27
: LGTM!The addition of the
feetiersKeeper
field to theKeeper
struct looks good.Verify that the
types.FeetiersKeeper
type is defined correctly in thetypes
package and that the necessary methods are implemented wherever this new field is used.Run the following script to check for the usage of
feetiersKeeper
:Verification successful
Usage of
feetiersKeeper
field verified and consistentThe
feetiersKeeper
field is used appropriately across multiple packages, primarily inrevshare
andaffiliates
. It's accessed to retrieve fee-related information (e.g.,GetAffiliateRefereeLowestTakerFee
,GetLowestMakerFee
) and is properly integrated into the testing infrastructure. The usage is consistent with its expected functionality, and no misuses or inconsistencies were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `feetiersKeeper` field. # Test: Search for the usage of `feetiersKeeper`. Expect: Only valid usages. rg --type go -A 5 $'feetiersKeeper'Length of output: 10927
protocol/x/revshare/keeper/revshare.go (2)
136-137
: LGTM!The addition of
lowestTakerFeePpm
andlowestMakerFeePpm
parameters is necessary for the updated formula.
149-151
: LGTM!The formula implementation looks correct and aligns with the updated documentation.
protocol/x/revshare/keeper/revshare_test.go (3)
263-292
: New test case looks good!The addition of the "invalid rev share config - violates safety condition" test case enhances the test coverage for the safety validation. The test case setup and expected behavior are clear and comprehensive.
299-299
: LGTM!The change from
ctx := tApp.InitChain()
toctx := tApp.InitChain()
is a minor refactor and does not affect the functionality of the test.
181-182
: Verify the new parameters are used correctly.The addition of
lowestTakerFee
andlowestMakerFee
parameters is a good change to enhance the safety validation. Please ensure that these parameters are being used correctly within theValidateRevShareSafety
function and that all calls to this function have been updated to provide these new parameters.Run the following script to verify the function usage:
Verification successful
New parameters are used correctly across the codebase
The verification process confirms that the
ValidateRevShareSafety
function has been consistently updated to include the new parameterslowestTakerFee
andlowestMakerFee
across all relevant parts of the codebase. This includes:
- The function signature in
protocol/x/revshare/keeper/revshare.go
- Test cases in
protocol/x/revshare/keeper/revshare_test.go
- Usage in other packages such as
feetiers
,affiliates
, and their respective typesThe changes appear to be implemented correctly and consistently.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `ValidateRevShareSafety` provide the new parameters. # Test: Search for the function usage. Expect: Only occurrences with the new parameters. rg --type go -A 5 $'ValidateRevShareSafety'Length of output: 5114
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.
func TestGetAffiliateRefereeLowestTakerFee(t *testing.T) { | ||
tests := map[string]struct { | ||
expectedLowestTakerFee int32 | ||
feeTiers types.PerpetualFeeParams | ||
}{ | ||
"tiers are ordered by absolute volume requirement": { | ||
feeTiers: types.StandardParams(), | ||
expectedLowestTakerFee: 350, | ||
}, | ||
} | ||
|
||
for name, tc := range tests { | ||
t.Run(name, func(t *testing.T) { | ||
tApp := testapp.NewTestAppBuilder(t).Build() | ||
ctx := tApp.InitChain() | ||
k := tApp.App.FeeTiersKeeper | ||
err := k.SetPerpetualFeeParams( | ||
ctx, | ||
tc.feeTiers, | ||
) | ||
require.NoError(t, err) | ||
|
||
}) | ||
} | ||
} |
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.
Add assertions and more test scenarios to improve test coverage.
The test function TestGetAffiliateRefereeLowestTakerFee
is a good start, but it can be improved in the following ways:
-
Add assertions to validate the actual behavior of
GetAffiliateRefereeLowestTakerFee
. The test should call the function and compare the returned value with the expected lowest taker fee. -
Consider adding more test scenarios to cover different fee tier configurations and edge cases. This will help ensure that the function behaves correctly under various conditions.
Here's an example of how you can add assertions and more test scenarios:
func TestGetAffiliateRefereeLowestTakerFee(t *testing.T) {
tests := map[string]struct {
expectedLowestTakerFee int32
feeTiers types.PerpetualFeeParams
}{
"tiers are ordered by absolute volume requirement": {
feeTiers: types.StandardParams(),
expectedLowestTakerFee: 350,
},
"tiers are not ordered": {
feeTiers: types.PerpetualFeeParams{
Tiers: []*types.PerpetualFeeTier{
{
Name: "tier2",
AbsoluteVolumeRequirement: 1000000,
TakerFeePpm: 400,
MakerFeePpm: 100,
},
{
Name: "tier1",
AbsoluteVolumeRequirement: 100000,
TakerFeePpm: 500,
MakerFeePpm: 200,
},
},
},
expectedLowestTakerFee: 400,
},
"no tiers": {
feeTiers: types.PerpetualFeeParams{},
expectedLowestTakerFee: 0,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
tApp := testapp.NewTestAppBuilder(t).Build()
ctx := tApp.InitChain()
k := tApp.App.FeeTiersKeeper
err := k.SetPerpetualFeeParams(ctx, tc.feeTiers)
require.NoError(t, err)
actualLowestTakerFee := k.GetAffiliateRefereeLowestTakerFee(ctx)
require.Equal(t, tc.expectedLowestTakerFee, actualLowestTakerFee)
})
}
}
protocol/x/feetiers/keeper/params.go
Outdated
@@ -27,6 +30,40 @@ func (k Keeper) SetPerpetualFeeParams( | |||
return err | |||
} | |||
|
|||
lowestMakerFee := int32(math.MaxInt32) | |||
lowestTakerFee := int32(math.MaxInt32) | |||
for _, tier := range params.Tiers { |
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.
Instead of duplicating logic let's create GetLowestMaker/TakerFeeFromFeeTiers
static methods which take in the fee tiers and call them here. The original GetLowestMaker/TakerFee
can call this helper to maintain their signature
// (lowest_maker_fee_share_ppm + highest_taker_fee_share_ppm)) | ||
// ValidateRevShareSafety roughly checks if the total rev share is valid using the formula below: | ||
// highest_affiliate_taker_share * (lowest_taker_fee_ppm / | ||
// (lowest_maker_ppm + lowest_taker_fee_ppm)) |
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.
Document that we always expect lowest_maker_ppm
to be negative
// (lowest_maker_fee_share_ppm + highest_taker_fee_share_ppm)) | ||
// ValidateRevShareSafety roughly checks if the total rev share is valid using the formula below: | ||
// highest_affiliate_taker_share * (lowest_taker_fee_ppm / | ||
// (lowest_maker_ppm + lowest_taker_fee_ppm)) | ||
// + sum(unconditional_rev_shares) + market_mapper_rev_share < 100% | ||
func (k Keeper) ValidateRevShareSafety( |
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.
Since both totalUnconditionalRevSharePpm
and totalMarketMapperRevSharePpm
are post maker rebates and taker affiliate rev share, I think we just need:
- totalUnconditionalRevSharePpm + totalMarketMapperRevSharePpm < 100
- Highest_affiliate_rev_share[protocol constant of 50%] * lowest_taker_fee - lowest_maker_fee < lowest_taker_fee
Which is actually a lot simpler 😓 Lmk if this makes sense
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.
This makes sense to me. Made the change!
@@ -38,25 +37,8 @@ func (k msgServer) UpdateAffiliateTiers(ctx context.Context, | |||
} | |||
|
|||
sdkCtx := sdk.UnwrapSDKContext(ctx) | |||
unconditionalRevShareConfig, err := k.revShareKeeper.GetUnconditionalRevShareConfigParams(sdkCtx) |
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.
Not needed anymore since we check only the max affiliate rev share cap
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.
Do we still need a reference to revShareKeeper
on affiliateKeeper
?
highestAffilliateTierRevSharePpm = tier.TakerFeeSharePpm | ||
} | ||
} | ||
highestAffilliateTierRevSharePpm := affiliatetypes.AffiliatesRevSharePpmCap |
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.
Use AffiliatesRevSharePpmCap
directly instead of redeclaring a new var
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.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
protocol/x/affiliates/types/expected_keepers.go (1)
Line range hint
1-17
: Summary of changes and potential impactThe changes in this file represent a significant shift in the affiliate system:
- Removal of
RevShareKeeper
: This suggests a move away from the previous revenue sharing mechanism.- Introduction of
FeetiersKeeper
: This new interface focuses on retrieving lowest fees, indicating a more fee-centric approach to affiliate management.These changes align with the PR objective of updating revenue share safety. The new approach could potentially provide better control and transparency in affiliate management. However, it's crucial to ensure that:
- All parts of the codebase that previously relied on
RevShareKeeper
have been updated accordingly.- The new fee-based system adequately addresses the safety concerns that led to this change.
- The removal of methods like
ValidateRevShareSafety
doesn't leave any security gaps in the system.Consider documenting the rationale behind this architectural change and updating any relevant documentation or specs to reflect the new fee-based affiliate system.
protocol/x/revshare/keeper/revshare_test.go (1)
227-227
: Consider adding more test cases for edge scenariosThe changes improve the test coverage for revenue share safety checks. However, to further enhance the robustness of the tests, consider adding the following scenarios:
- A test case where
lowestTakerFee
andlowestMakerFee
are both 0.- A test case where
lowestTakerFee
is positive andlowestMakerFee
is the minimum possible negative value.- A test case where the sum of revenue shares is exactly 100%.
These additional test cases would help ensure that the safety checks handle edge cases correctly.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
<details>
Files selected for processing (16)
- protocol/app/app.go (1 hunks)
- protocol/testing/e2e/gov/feetiers_test.go (1 hunks)
- protocol/testutil/keeper/clob.go (1 hunks)
- protocol/testutil/keeper/listing.go (1 hunks)
- protocol/testutil/keeper/subaccounts.go (1 hunks)
- protocol/x/affiliates/keeper/keeper.go (2 hunks)
- protocol/x/affiliates/keeper/msg_server.go (2 hunks)
- protocol/x/affiliates/types/expected_keepers.go (1 hunks)
- protocol/x/feetiers/keeper/keeper.go (3 hunks)
- protocol/x/feetiers/keeper/keeper_test.go (2 hunks)
- protocol/x/feetiers/keeper/params.go (2 hunks)
- protocol/x/feetiers/types/expected_keepers.go (2 hunks)
- protocol/x/revshare/keeper/msg_set_marketmapper_revenue_share.go (1 hunks)
- protocol/x/revshare/keeper/msg_update_unconditional_revshare_config.go (1 hunks)
- protocol/x/revshare/keeper/revshare.go (1 hunks)
- protocol/x/revshare/keeper/revshare_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (9)
- protocol/testutil/keeper/clob.go
- protocol/testutil/keeper/listing.go
- protocol/testutil/keeper/subaccounts.go
- protocol/x/feetiers/keeper/keeper.go
- protocol/x/feetiers/keeper/keeper_test.go
- protocol/x/feetiers/keeper/params.go
- protocol/x/feetiers/types/expected_keepers.go
- protocol/x/revshare/keeper/msg_set_marketmapper_revenue_share.go
- protocol/x/revshare/keeper/msg_update_unconditional_revshare_config.go
Additional comments not posted (19)
protocol/testing/e2e/gov/feetiers_test.go (1)
23-24
: Verify intentional swap of fee structures and alignment with fee strategy.The changes in the
testPerpetualFeeParams
variable have swapped the fee structures between "test_tier_0" and "test_tier_1":
- "test_tier_0": MakerFeePpm increased from 1,000 to 11,000, TakerFeePpm increased from 2,000 to 22,000.
- "test_tier_1": MakerFeePpm decreased from 11,000 to 1,000, TakerFeePpm decreased from 22,000 to 2,000.
Please confirm:
- Is this swap in fee structures intentional?
- Do these new fee values align with the project's fee strategy and expected real-world scenarios?
To ensure comprehensive test coverage, consider updating or adding test cases that cover both the old and new fee structures. This will help verify that the system behaves correctly under various fee configurations.
This script will help identify if there are test cases covering both the old and new fee structures.
Also applies to: 31-32
protocol/x/revshare/keeper/revshare.go (4)
129-135
: LGTM: Function signature updated to include all necessary parameters.The function signature has been updated to include
lowestTakerFeePpm
andlowestMakerFeePpm
as input parameters. This change aligns with the suggestion to pass everything as input, making the function more flexible and easier to test.
144-153
: LGTM: Implementation correctly checks the two safety conditions.The implementation accurately checks the two conditions mentioned in the comment:
- It verifies that the sum of total unconditional rev share and market mapper rev share is less than 100% (1 million PPM).
- It checks that the product of the highest affiliate rev share and the lowest taker fee, adjusted by the lowest maker fee, remains within acceptable limits.
The code also includes a safeguard against division by zero by checking if the lowest taker fee is not zero before performing the calculation.
136-136
: LGTM: Using AffiliatesRevSharePpmCap constant.The use of
AffiliatesRevSharePpmCap
directly, instead of redeclaring a new variable, aligns with a previous review comment and improves code maintainability.
126-155
: Overall assessment: The changes improve revenue sharing safety checks.The modifications to the
ValidateRevShareSafety
function effectively implement more comprehensive safety checks for revenue sharing. The function now considers both unconditional and market mapper revenue shares, as well as the impact of affiliate revenue shares on fees. These changes align with previous review comments and should enhance the robustness of the revenue sharing mechanism.protocol/x/affiliates/keeper/keeper.go (3)
281-283
: LGTM: NewSetFeetiersKeeper
method added.The new
SetFeetiersKeeper
method is consistent with the change in theKeeper
struct and provides a way to set thefeetiersKeeper
field.To ensure this new method is being used correctly, please run the following script to check for its usage across the codebase:
#!/bin/bash # Search for usage of SetFeetiersKeeper rg --type go 'SetFeetiersKeeper'Verify that this method is called appropriately during the initialization or configuration of the
Keeper
.
Line range hint
1-383
: Consider a broader review of the revenue sharing to fee tier refactoring.The changes in this file appear to be part of a larger refactoring effort, shifting from revenue sharing to fee tier management. However, the limited scope of changes in this file suggests that the refactoring might be incomplete or spread across multiple files.
- Conduct a comprehensive review of all files that might be affected by this change.
- Update any methods or functions that previously used
revShareKeeper
to now usefeetiersKeeper
appropriately.- Ensure that all revenue sharing functionality has been properly migrated to the new fee tier system.
- Update any relevant documentation or comments to reflect the new fee tier system.
- Consider adding migration scripts or functions if there's a need to convert existing data from the old revenue sharing system to the new fee tier system.
To assist in this broader review, please run the following script to identify files that might need updating:
#!/bin/bash # Search for files containing revShareKeeper or FeetiersKeeper rg --type go -l 'revShareKeeper|FeetiersKeeper'Review each of these files to ensure they've been properly updated as part of this refactoring process.
26-26
: Verify the impact of replacingrevShareKeeper
withfeetiersKeeper
.The change from
revShareKeeper
tofeetiersKeeper
suggests a shift in functionality from revenue sharing to fee tier management. This modification could have far-reaching effects on the codebase.Please run the following script to check for any remaining references to
revShareKeeper
that might need updating:Additionally, ensure that all functionalities previously handled by
revShareKeeper
are now properly managed byfeetiersKeeper
.protocol/x/revshare/keeper/revshare_test.go (4)
180-181
: LGTM: New parameters added to test casesThe addition of
lowestTakerFee
andlowestMakerFee
parameters to the test struct is appropriate. These new parameters will allow for more comprehensive testing of the revenue share safety checks.
198-200
: LGTM: Updated existing test caseThe existing test case has been updated to include the new
lowestTakerFee
andlowestMakerFee
parameters. The values chosen (350 and -110 respectively) seem reasonable for testing purposes.
202-219
: LGTM: New test case for revenue share safety violationA new test case has been added to check for revenue share safety violations. This test case covers two scenarios:
- When the sum of unconditional and market mapper revenue shares exceeds 100%.
- When there's a violation related to affiliate fees.
This addition improves the test coverage and helps ensure the safety checks are working as expected.
234-238
: LGTM: Updated ValidateRevShareSafety function callThe
ValidateRevShareSafety
function call has been updated to include the newlowestTakerFee
andlowestMakerFee
parameters. This change aligns with the modifications made to the function signature mentioned in the AI-generated summary.protocol/app/app.go (5)
952-959
: MarketMapKeeper initialization looks goodThe MarketMapKeeper is now properly initialized with the necessary parameters. This change aligns with the AI-generated summary, which mentioned the modification of the MarketMapKeeper initialization.
960-971
: FeeTiersKeeper initialization updated correctlyThe FeeTiersKeeper initialization has been updated to include references to StatsKeeper and AffiliatesKeeper. This change is in line with the AI-generated summary and improves the integration between these components.
973-973
: AffiliatesKeeper now references FeeTiersKeeperThis line sets the FeeTiersKeeper in the AffiliatesKeeper, which is consistent with the AI-generated summary mentioning tighter integration between these components.
975-985
: RevShareKeeper initialization and integrationThe RevShareKeeper is now properly initialized with references to AffiliatesKeeper and FeeTiersKeeper. This change aligns with the AI-generated summary and improves the overall integration of these components.
952-985
: Overall changes improve keeper integrationThe modifications in this section enhance the integration between MarketMapKeeper, FeeTiersKeeper, RevShareKeeper, and AffiliatesKeeper. These changes are consistent with the AI-generated summary and appear to be implemented correctly. The new structure should provide better coordination between these components, potentially improving the overall functionality of the revenue sharing and fee tier systems.
protocol/x/affiliates/keeper/msg_server.go (2)
41-44
: Ensure validations are performed withinUpdateAffiliateTiers
The removal of validation checks simplifies the method, but it's crucial to confirm that
k.Keeper.UpdateAffiliateTiers
now handles all necessary safety validations internally to maintain revenue share safety.Run the following script to verify that
UpdateAffiliateTiers
includes the required validations:
55-58
: Ensure validations are performed withinSetAffiliateWhitelist
With the removal of validation logic here, please verify that
k.Keeper.SetAffiliateWhitelist
now encompasses all necessary safety checks to ensure the whitelist is updated securely.Run the following script to verify that
SetAffiliateWhitelist
includes the required validations:
type FeetiersKeeper interface { | ||
GetAffiliateRefereeLowestTakerFee(ctx sdk.Context) int32 | ||
GetLowestMakerFee(ctx sdk.Context) int32 |
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.
Tip
Codebase Verification
Incomplete Removal of RevShareKeeper
Detected
The search results indicate that RevShareKeeper
is still present in multiple areas of the codebase, including interfaces, keeper implementations, and test files. This suggests that the replacement with FeetiersKeeper
is not fully implemented.
Action Items:
- Complete Refactoring: Ensure that all instances of
RevShareKeeper
are replaced withFeetiersKeeper
across the entire codebase. - Update Dependencies: Verify that all modules and tests are updated to use
FeetiersKeeper
instead ofRevShareKeeper
. - Review Safety Checks: Confirm that any safety checks previously handled by
RevShareKeeper
are appropriately managed by the newFeetiersKeeper
.
Analysis chain
New FeetiersKeeper
interface introduced
The new FeetiersKeeper
interface has been added, replacing the previous RevShareKeeper
. This change aligns with the PR objective of updating revenue share safety.
A few points to consider:
- The new methods focus on retrieving the lowest fees, which is a shift from the previous revenue sharing approach.
- Ensure that all places where
RevShareKeeper
was used have been updated to useFeetiersKeeper
instead. - The removal of
RevShareKeeper
methods likeValidateRevShareSafety
suggests that the safety checks might have been moved elsewhere or implemented differently.
To ensure that all usages of the old RevShareKeeper
have been properly updated, please run the following script:
This script will help identify any places where the old RevShareKeeper
might still be in use and confirm that FeetiersKeeper
is being used appropriately.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usages of RevShareKeeper in the codebase
# Search for RevShareKeeper usages
echo "Searching for RevShareKeeper usages:"
rg --type go "RevShareKeeper"
# Search for the old methods that were in RevShareKeeper
echo "Searching for old RevShareKeeper methods:"
rg --type go "GetUnconditionalRevShareConfigParams|GetMarketMapperRevenueShareParams|ValidateRevShareSafety"
# Search for new FeetiersKeeper usages
echo "Searching for FeetiersKeeper usages:"
rg --type go "FeetiersKeeper"
Length of output: 8519
MakerFeePpm: 1_000, | ||
TakerFeePpm: 2_000, |
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.
Needed to switch the order since fees should decrease with tiers increasing
@@ -125,16 +125,14 @@ func (k Keeper) SetUnconditionalRevShareConfigParams(ctx sdk.Context, config typ | |||
|
|||
// Check two conditions to ensure rev shares are safe. | |||
// 1. totalUnconditionalRevSharePpm + totalMarketMapperRevSharePpm < 100% | |||
// 2. highestAffilliateTierRevSharePpm/lib.OneMillion - lowest_taker_fee/lowest_maker_fee < 1 | |||
// 2. Highest_affiliate_rev_share[protocol constant of 50%] * lowest_taker_fee - lowest_maker_fee < lowest_taker_fee |
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.
Nit: found below to be more natural (net_fee
on the left side)
// 2. Highest_affiliate_rev_share[protocol constant of 50%] * lowest_taker_fee - lowest_maker_fee < lowest_taker_fee | |
// 2. lowest_taker_fee + lowest_maker_fee > Highest_affiliate_rev_share * lowest_taker_fee |
return false | ||
} | ||
|
||
// taker fee can never be 0. Adding a check to ensure lowest_taker_fee is not 0 |
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.
Don't love the unsafe cast from uint32
to int32
+ large num multiplication on ints (MaxInt32
is 2,147,483,647
so we are pretty close to overflow here.
Since this is not in the hot path, we can afford to use big.Int
:
bigNetFee := new(big.Int).SetUint64(
// Casting is safe since both variables are int32.
uint64(lowestTakerFeePpm) + uint64(lowestMakerFeePpm),
)
bigLowestTakerFeePpmMulRevShareRateCap := lib.BigMulPpm(
lib.BigI(lowestTakerFeePpm),
lib.BigU(affiliatetypes.AffiliatesRevSharePpmCap),
true,
)
return bigNetFee.Cmp(bigLowestTakerFeePpmMulRevShareRateCap) > 0
```
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.
good catch!
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Documentation