[OTE-687] Bump fee tiers for referees #2217
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve modifications to several keeper functions to incorporate the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
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 (
|
affanv14
force-pushed
the
affan/fee-tier
branch
2 times, most recently
from
e610afc to
44bace9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (4)
protocol/x/feetiers/types/expected_keepers.go (1)
19-22: New interfaceAffiliatesKeeperadded.The addition of the
AffiliatesKeeperinterface with theGetReferredBymethod is well-implemented. However, it would be beneficial to add documentation to explain the purpose and usage of this interface and its method to aid future developers.Consider adding comments like:
// AffiliatesKeeper defines the expected affiliates keeper. // It provides methods to manage and retrieve affiliate relationships. type AffiliatesKeeper interface { // GetReferredBy returns the referrer of the given referee. // The boolean return value indicates whether a referrer was found. GetReferredBy(ctx sdk.Context, referee string) (string, bool) }protocol/testutil/keeper/affiliates.go (1)
12-28: FunctioncreateAffiliatesKeeperimplemented correctly.The implementation of
createAffiliatesKeeperis correct and follows best practices for dependency injection and store setup. However, adding documentation would enhance understandability and maintainability.Consider adding function documentation like:
// createAffiliatesKeeper initializes a new instance of AffiliatesKeeper. // It configures the keeper with necessary dependencies and store configurations. func createAffiliatesKeeper( stateStore storetypes.CommitMultiStore, db *dbm.MemDB, cdc *codec.ProtoCodec, statsKeeper *statskeeper.Keeper, ) (*affiliateskeeper.Keeper, storetypes.StoreKey) { // function body }protocol/testutil/keeper/feetiers.go (1)
Line range hint
22-39: Integration ofaffiliatesKeeperintocreateFeeTiersKeeper.The modification to include
affiliatesKeeperincreateFeeTiersKeeperis correctly implemented. This change allows for enhanced functionality by integrating affiliate management into the fee tiers logic. It is recommended to add tests to verify this integration and ensure that it functions as expected without introducing regressions.Would you like assistance in writing the integration tests for this modification?
protocol/x/feetiers/keeper/keeper.go (1)
23-23: Add documentation foraffiliatesKeeper.The addition of
affiliatesKeeperto theKeeperstruct is correctly implemented. However, it would be beneficial to add documentation explaining its role and interactions within theKeeper.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- protocol/app/app.go (2 hunks)
- protocol/testutil/keeper/affiliates.go (1 hunks)
- protocol/testutil/keeper/clob.go (1 hunks)
- protocol/testutil/keeper/feetiers.go (3 hunks)
- protocol/testutil/keeper/listing.go (1 hunks)
- protocol/testutil/keeper/rewards.go (1 hunks)
- protocol/x/feetiers/keeper/keeper.go (2 hunks)
- protocol/x/feetiers/keeper/keeper_test.go (2 hunks)
- protocol/x/feetiers/types/constants.go (1 hunks)
- protocol/x/feetiers/types/expected_keepers.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/feetiers/types/constants.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 (4)
protocol/x/feetiers/keeper/keeper.go (1)
30-39: Constructor functionNewKeepercorrectly initializesaffiliatesKeeper.The updated
NewKeeperfunction properly initializes theKeeperstruct with the newaffiliatesKeeperparameter. The parameter ordering and initialization are correctly handled.protocol/x/feetiers/keeper/keeper_test.go (1)
136-204: Well-structured test function for perpetual fee calculation with referrals.The test function
TestGetPerpetualFeePpm_Referralis well-structured and effectively tests the perpetual fee calculation for users with and without referrals. The use of a setup function to configure the test environment is appropriate, and the test cases cover relevant scenarios.Consider adding more test cases if there are additional scenarios or edge cases that could affect the perpetual fee calculation.
protocol/testutil/keeper/clob.go (1)
142-152: Enhanced test setup with integration ofaffiliatesKeeper.The modifications to the
NewClobKeepersTestContextWithUninitializedMemStorefunction effectively integrate theaffiliatesKeeperinto the test setup. This enhancement allows the fee tiers module to potentially access affiliate-related functionalities, aligning with the PR objectives.Ensure that all dependencies and interactions with the
affiliatesKeeperare correctly handled and tested.protocol/app/app.go (1)
1026-1039: Cohesive architecture with enhanced integration ofAffiliatesKeeper.The modifications to the initialization of the
AffiliatesKeeperand the updates to theFeeTiersKeeperreflect a more cohesive architecture. The integration of theAffiliatesKeeperwith theStatsKeeperenhances the management of affiliate relationships and fee tier calculations. The updates to theFeeTiersKeepersuggest improved functionality related to affiliate management.Ensure that the new functionalities are thoroughly tested to verify correct integration and operation within the application.
protocol/x/feetiers/keeper/keeper.go
Outdated
| // Bump up to RefereeStartingFeeTier if the user is referred by an affiliate. | ||
| // We subtract 1 because the fee tiers are 1-indexed. | ||
| if idx < types.RefereeStartingFeeTier-1 { | ||
| _, hasReferree := k.affiliatesKeeper.GetReferredBy(ctx, address) | ||
| if hasReferree { | ||
| idx = types.RefereeStartingFeeTier - 1 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Review the affiliate referral logic in getUserFeeTier.
The modification to getUserFeeTier to incorporate affiliate referral logic is correctly implemented. However, consider adding error handling or logging for the call to GetReferredBy to ensure that any issues in fetching referral data are appropriately managed.
protocol/testutil/keeper/rewards.go
Outdated
| affiliatesKeeper, _ := createAffiliatesKeeper( | ||
| stateStore, | ||
| db, | ||
| cdc, | ||
| statsKeeper, | ||
| ) | ||
| feetiersKeeper, _ = createFeeTiersKeeper( | ||
| stateStore, | ||
| statsKeeper, | ||
| vaultKeeper, | ||
| affiliatesKeeper, |
There was a problem hiding this comment.
Proper integration of affiliatesKeeper in RewardsKeepers.
The creation and integration of affiliatesKeeper using createAffiliatesKeeper and its subsequent passing to createFeeTiersKeeper are correctly implemented. However, consider adding error handling in the createAffiliatesKeeper function to manage potential initialization failures effectively.
protocol/testutil/keeper/listing.go
Outdated
| affiliatesKeeper, _ := createAffiliatesKeeper( | ||
| stateStore, | ||
| db, | ||
| cdc, | ||
| statsKeeper, | ||
| ) | ||
| feeTiersKeeper, _ := createFeeTiersKeeper( | ||
| stateStore, | ||
| statsKeeper, | ||
| vaultKeeper, | ||
| affiliatesKeeper, |
There was a problem hiding this comment.
Proper integration of affiliatesKeeper in ListingKeepers.
The creation and integration of affiliatesKeeper using createAffiliatesKeeper and its subsequent passing to createFeeTiersKeeper are correctly implemented. However, consider adding error handling in the createAffiliatesKeeper function to manage potential initialization failures effectively.
affanv14
force-pushed
the
affan/fee-tier
branch
from
44bace9 to
5fa6499
Compare
protocol/x/feetiers/keeper/keeper.go
Outdated
| @@ -97,6 +100,15 @@ func (k Keeper) getUserFeeTier(ctx sdk.Context, address string) (uint32, *types. | |||
| idx = uint32(i) | |||
| } | |||
|
|
|||
| // Bump up to RefereeStartingFeeTier if the user is referred by an affiliate. | |||
| // We subtract 1 because the fee tiers are 1-indexed. | |||
There was a problem hiding this comment.
It's a bit annoying that we have to do this, thanks for being attentive to details here.
Feel like below would be cleaner:
- Rename constant to
RefereeStartingFeeTierIndex, make this2, and add a comment this is 0-indexed and refers to the index of the feeTiers array instead of fee tier name. This way we aren't making assumption that fee tier name is well aligned to the indexes.
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| ctx := tApp.InitChain() | ||
| k := tApp.App.FeeTiersKeeper | ||
| err := k.SetPerpetualFeeParams( |
There was a problem hiding this comment.
Nit: take this object to the top of function:
testPerpetualFeeParams := ....
| err := affiliatesKeeper.RegisterAffiliate(ctx, constants.AliceAccAddress.String(), constants.BobAccAddress.String()) | ||
| require.NoError(t, err) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Nit: Add two test cases that a user already at and larger than StartingFeeTier still gets correct fee tier
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- protocol/app/app.go (2 hunks)
- protocol/testutil/keeper/affiliates.go (1 hunks)
- protocol/testutil/keeper/clob.go (1 hunks)
- protocol/testutil/keeper/feetiers.go (3 hunks)
- protocol/testutil/keeper/listing.go (1 hunks)
- protocol/testutil/keeper/rewards.go (1 hunks)
- protocol/x/feetiers/keeper/keeper.go (2 hunks)
- protocol/x/feetiers/keeper/keeper_test.go (2 hunks)
- protocol/x/feetiers/types/constants.go (1 hunks)
- protocol/x/feetiers/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- protocol/testutil/keeper/affiliates.go
- protocol/testutil/keeper/clob.go
- protocol/testutil/keeper/feetiers.go
- protocol/testutil/keeper/listing.go
- protocol/testutil/keeper/rewards.go
- protocol/x/feetiers/keeper/keeper.go
- protocol/x/feetiers/keeper/keeper_test.go
- protocol/x/feetiers/types/constants.go
- protocol/x/feetiers/types/expected_keepers.go
Additional comments not posted (2)
protocol/app/app.go (2)
1026-1035: Review ofAffiliatesKeeperInitializationThe initialization of
AffiliatesKeeperhas been updated to include a dependency onStatsKeeper. This change aligns with the PR's objective to enhance the fee model by potentially using statistical data in fee calculations.
- Correctness: The initialization appears correct and follows the typical pattern for keeper initialization in the Cosmos SDK.
- Performance: No immediate concerns as the initialization happens once and the impact on runtime performance should be minimal.
- Maintainability: The addition of
StatsKeeperas a dependency is clearly stated, making the code easy to understand and maintain.Overall, the changes are aligned with the PR objectives and are implemented correctly.
Line range hint
1039-1049: Review ofFeeTiersKeeperInitializationThe
FeeTiersKeepernow includesAffiliatesKeeperas a parameter. This modification is crucial for the PR's goal to integrate affiliate data into fee tier calculations.
- Correctness: The code correctly passes the
AffiliatesKeeperinto theFeeTiersKeeper. This ensures that the fee tier logic can access affiliate-related functionalities.- Security: No security issues are apparent in this change.
- Maintainability: The change is minimal and does not complicate the existing code structure. It is also well-documented through the code itself, making it easy to understand the new dependency.
This change is essential for achieving the PR's objectives and is implemented correctly.
Changelist
Bumps addresses to a starting fee tier if the address has a referral
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
Tests
Documentation