Integrate commission and overrides to fee tier calculation #3117
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
protocol/x/affiliates/keeper/grpc_query_test.go (1)
78-83: Inconsistent 1e18 scaling in delegation amount (may cause flaky expectations later)In Success you scale by 1e18; here you don’t. It’s benign now (tier-0 is 0) but will break if requirements change.
Apply for consistency:
- err := stakingKeeper.SetDelegation(ctx, - stakingtypes.NewDelegation(constants.AliceAccAddress.String(), - constants.AliceValAddress.String(), math.LegacyNewDecFromBigInt( - big.NewInt(int64(types.DefaultAffiliateTiers.Tiers[0].ReqStakedWholeCoins)), - ), - ), - ) + err := stakingKeeper.SetDelegation(ctx, + stakingtypes.NewDelegation(constants.AliceAccAddress.String(), + constants.AliceValAddress.String(), math.LegacyNewDecFromBigInt( + new(big.Int).Mul( + big.NewInt(int64(types.DefaultAffiliateTiers.Tiers[0].ReqStakedWholeCoins)), + big.NewInt(1e18), + ), + ), + ), + )
🧹 Nitpick comments (2)
protocol/x/affiliates/keeper/keeper_test.go (1)
668-669: Consider simplifying the test structure or adding multi-affiliate test cases.The test structure now uses
expectedReferrerandexpectedAttributedVolumearrays to support multiple referrers, but all test cases only verify a single affiliate. This suggests either:
- The array structure is over-engineered for the current test cases and could be simplified to single values, or
- Additional test cases covering multiple distinct affiliates would better justify the array-based approach.
If the intent is to test scenarios where fills involve multiple different affiliates (not just multiple referees under one affiliate), consider adding such test cases to validate the per-referrer aggregation logic more thoroughly.
Also applies to: 1062-1067
protocol/x/affiliates/keeper/grpc_query_test.go (1)
107-111: Avoid hard-coding last-tier index and fee share; derive from DefaultAffiliateTiersPrevents breakage if tiers change.
- Tier: 4, - FeeSharePpm: 250_000, + Tier: uint32(len(types.DefaultAffiliateTiers.Tiers) - 1), + FeeSharePpm: types.DefaultAffiliateTiers.Tiers[len(types.DefaultAffiliateTiers.Tiers)-1].TakerFeeSharePpm,Optional: rename this case from "Whitelisted" to "Overrides" for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
protocol/x/affiliates/keeper/grpc_query_test.go(3 hunks)protocol/x/affiliates/keeper/keeper_test.go(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
protocol/x/affiliates/keeper/keeper_test.go (7)
indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/tx.ts (2)
MsgUpdateAffiliateParameters(78-84)MsgUpdateAffiliateParameters(399-445)indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/affiliates.ts (2)
AffiliateParameters(79-94)AffiliateParameters(350-405)protocol/x/affiliates/types/affiliates.pb.go (3)
AffiliateParameters(241-250)AffiliateParameters(254-254)AffiliateParameters(255-257)protocol/x/affiliates/types/expected_keepers.go (1)
StatsKeeper(10-15)indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (2)
UserStats(125-137)UserStats(510-574)protocol/x/stats/types/stats.pb.go (3)
UserStats(356-365)UserStats(369-369)UserStats(370-372)protocol/x/affiliates/keeper/keeper.go (1)
Keeper(20-27)
protocol/x/affiliates/keeper/grpc_query_test.go (2)
protocol/x/affiliates/types/constants.go (1)
DefaultAffiliateTiers(4-32)protocol/dtypes/serializable_int.go (1)
NewIntFromUint64(36-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (36)
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: check-build-auxo
- GitHub Check: check-build-bazooka
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: test / run_command
- GitHub Check: test-coverage-upload
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: liveness-test
- GitHub Check: build-and-push-mainnet
- GitHub Check: container-tests
- GitHub Check: build
- GitHub Check: golangci-lint
- GitHub Check: build-and-push-testnet
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: benchmark
- GitHub Check: run_command
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (9)
protocol/x/affiliates/keeper/keeper_test.go (7)
142-147: LGTM!The addition of
UpdateAffiliateParameterscorrectly initializes the 30-day attributable volume cap before testing affiliate behavior.
173-177: LGTM!The migration to
StatsKeeper.SetUserStatscorrectly sets up the affiliate's referred volume to exceed the tier threshold, enabling proper tier progression testing.
270-274: LGTM!Correctly uses
StatsKeeper.SetUserStatsto pre-populate affiliate referred volume at the tier 2 threshold for testing tier qualification logic.
703-709: LGTM!The consistent use of
StatsKeeper.SetUserStatsto pre-initialize affiliate volumes before testing aggregation logic correctly simulates existing 30-day rolling window state. The comments clearly explain the test scenarios involving volume caps.Also applies to: 738-745, 820-827, 869-876, 929-935, 994-999
1096-1106: LGTM!The assertions correctly validate the renamed parameter fields using the updated getter methods that align with the quote-quantum nomenclature from the protobuf definitions.
1-1108: Well-structured migration to StatsKeeper-driven state management.The test file successfully migrates from direct state mutations to
StatsKeeper-driven setup, which provides better alignment with the actual system behavior where stats drive affiliate state transitions. The field naming updates consistently reflect the shift from notional/revenue to quote-quantum nomenclature across all test cases.The enhanced test structure with per-referrer tracking arrays provides good extensibility, though most cases currently only test single-affiliate scenarios.
808-852: Cannot run test due to build errors – please verify fill totals vs. expectedVolume
Two fills of 100 000 000 000 each sum to 200 000 000 000, butexpectedVolumeis 300 000 000 000. Run the test locally (TestAggregateAffiliateReferredVolumeForFills/2_referrals,_takers_not_referred,_maker_referred) to confirm whetherexpectedVolumeshould be 200 000 000 000 or a fill notional updated.protocol/x/affiliates/keeper/grpc_query_test.go (2)
32-40: Switch to ReferredVolume_30DRolling looks correctUsing dtypes.NewIntFromUint64 and the 30D metric aligns with the keeper changes. Expected 0 baseline matches DefaultAffiliateTiers.Tiers[0].
66-74: LGTM on updated expectationsReferredVolume_30DRolling and SerializableInt usage are consistent with the new 30D window.
|
https://github.com/Mergifyio backport release/protocol/v9.x |
✅ Backports have been created
|
|
https://github.com/Mergifyio backport release/indexer/v9.x |
✅ Backports have been created
|
(cherry picked from commit 1b53602) # Conflicts: # indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/affiliates.ts # indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts # proto/dydxprotocol/affiliates/affiliates.proto # proto/dydxprotocol/stats/stats.proto # protocol/scripts/affiliates/update_affiliate_parameters.py # protocol/scripts/genesis/sample_pregenesis.json # protocol/testing/testnet/genesis.json # protocol/x/affiliates/keeper/grpc_query_test.go # protocol/x/affiliates/keeper/keeper.go # protocol/x/affiliates/keeper/keeper_test.go # protocol/x/affiliates/types/affiliates.pb.go # protocol/x/affiliates/types/constants.go # protocol/x/affiliates/types/expected_keepers.go # protocol/x/affiliates/types/keys.go # protocol/x/affiliates/types/query.pb.go # protocol/x/revshare/keeper/revshare.go # protocol/x/revshare/keeper/revshare_test.go # protocol/x/stats/keeper/keeper.go # protocol/x/stats/types/stats.pb.go
(cherry picked from commit 1b53602) # Conflicts: # indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/affiliates.ts # indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts # proto/dydxprotocol/affiliates/affiliates.proto # proto/dydxprotocol/stats/stats.proto # protocol/scripts/affiliates/update_affiliate_parameters.py # protocol/scripts/genesis/sample_pregenesis.json # protocol/testing/testnet/genesis.json # protocol/x/affiliates/keeper/grpc_query_test.go # protocol/x/affiliates/keeper/keeper.go # protocol/x/affiliates/keeper/keeper_test.go # protocol/x/affiliates/types/affiliates.pb.go # protocol/x/affiliates/types/constants.go # protocol/x/affiliates/types/expected_keepers.go # protocol/x/affiliates/types/keys.go # protocol/x/affiliates/types/query.pb.go # protocol/x/revshare/keeper/revshare.go # protocol/x/revshare/keeper/revshare_test.go # protocol/x/stats/keeper/keeper.go # protocol/x/stats/types/stats.pb.go
|
https://github.com/Mergifyio backport release/protocol/v9.x |
✅ Backports have been created
|
|
https://github.com/Mergifyio backport release/indexer/v9.x |
✅ Backports have been created
|
…#3117) (backport #3191) (#3193) Co-authored-by: Justin Barnett <[email protected]>
Changelist
Test Plan
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
Chores