Fix issue with 30d volume not being removed, and commission not being capped#3250
Fix issue with 30d volume not being removed, and commission not being capped#3250
Conversation
WalkthroughThis PR refactors affiliate revenue sharing to track attributions at the fill level rather than block-level aggregation. It introduces affiliate attribution persistence in block stats, moves volume aggregation from the affiliates EndBlocker to stats processing, enforces 30-day affiliate revenue and volume caps, and deprecates staking-based affiliate tier progression. Changes
Sequence Diagram(s)sequenceDiagram
participant clob as CLOB Keeper
participant stats as Stats Keeper
participant revshare as RevShare Keeper
participant affiliates as Affiliates Keeper
clob->>clob: ProcessSingleMatch
clob->>clob: buildAttributableVolumeAttributions
clob->>clob: getAttributableVolume (per-user cap check)
clob->>stats: RecordFill(..., affiliateAttributions)
stats->>stats: Store affiliateAttributions in BlockStats_Fill
rect rgb(200, 220, 255)
Note over stats: ProcessBlockStats
stats->>stats: Iterate affiliateAttributions per fill
stats->>stats: Update referrer Affiliate_30DReferredVolumeQuoteQuantums
stats->>stats: Update referee Affiliate_30DAttributedVolumeQuoteQuantums
stats->>stats: Initialize/update per-epoch userStatsMap
end
revshare->>revshare: GetAllRevShares
revshare->>revshare: getAffiliateRevShares
revshare->>stats: getHistoricalUserStats (retrieve cap status)
revshare->>revshare: Clamp affiliate fees to remaining cap
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
♻️ Duplicate comments (6)
protocol/x/stats/keeper/grpc_query_test.go (2)
155-169: Remove redundant test case.The "Success with attributed volume" test case is functionally identical to the "Success" test case (lines 146-154). Both query the same user with the same
userStatsand expect identical responses. This duplication should be removed.
183-191: Remove redundant assertion.The explicit verification of
Affiliate_30DAttributedVolumeQuoteQuantumsis unnecessary because line 182'srequire.Equal(t, tc.res, res)already performs a complete struct comparison that validates all fields, including the attributed volume field.protocol/x/stats/keeper/keeper.go (2)
237-281: Verify upstream cap enforcement is complete.The attribution processing correctly uses
attribution.ReferredVolumeQuoteQuantumsrather thanfill.Notional, which means the capped volume must be computed and set by the caller. Based on the related changes inprocess_single_match.go, this appears to be the intended design.However, per the existing review comment, there are concerns about:
- Intra-block cap enforcement: Multiple fills within the same block may each grant volume up to the cap because prior in-block attributions are invisible (caps are only enforced across blocks).
- Taker/maker asymmetry: Taker attribution requires
AffiliateRevShare != nil, while maker attribution only requiresGetReferredBy, creating inconsistent behavior.Run the following script to verify the cap enforcement logic in the clob keeper:
#!/bin/bash # Verify how getAttributableVolume is called and whether it accounts for in-block fills rg -n "getAttributableVolume" protocol/x/clob/keeper/process_single_match.go -A5 -B5 # Check if there's any in-block accumulation logic rg -n "ProcessBlockStats\|RecordFill" protocol/x/stats/keeper/ -A3 # Verify the taker/maker gating logic rg -n "AffiliateRevShare.*nil\|GetReferredBy" protocol/x/clob/keeper/process_single_match.go -A3 -B3
335-336: Expiration logic correctly removes attribution volumes.The code properly decrements both
Affiliate_30DReferredVolumeQuoteQuantumsandAffiliate_30DAttributedVolumeQuoteQuantumswhen epoch stats expire, which addresses the PR's stated goal of fixing "30d volume not being removed."However, per the existing conversation at line 335, there's a concern about existing mainnet state corruption: referees with consistently high volume before the upgrade won't get new volume attributed, while new referees will. This creates a scenario where some affiliates will have perpetually higher affiliate tiers due to volume that should have expired but won't without a forced reset.
Based on learnings from the existing discussion, confirm whether a migration or forced reset is planned to address the corrupted mainnet data, or if the impact is deemed acceptable.
protocol/x/clob/keeper/process_single_match.go (2)
642-675: Intra-block cap enforcement reads only committed state.
getAttributableVolumecorrectly computes the remaining attributable volume by readingAffiliate_30DAttributedVolumeQuoteQuantumsfromGetUserStats. However, as noted in the existing review comment, this function reads from the committed KVStore state, which is only updated at end-of-block viaProcessBlockStats.Impact: Multiple fills for the same referee within one block will each see the identical baseline attributed volume, allowing each fill to independently consume up to the remaining cap. The per-referee cap is enforced only across blocks, not within a block.
Example: If a referee has 80k attributed and the cap is 100k, two fills of 15k each in the same block will both be granted 15k attribution (total 30k in-block), exceeding the intended 20k remaining capacity.
If strict per-fill cap enforcement is required, maintain a block-scoped accumulator (e.g., an in-memory map keyed by referee address) during matching to track in-block attributions and reduce the remaining attributable volume for subsequent fills in the same block.
677-733: Taker/maker attribution asymmetry requires clarification.The attribution logic differs between taker and maker:
- Taker (lines 688-706): Requires
revSharesForFill.AffiliateRevShare != nilAND non-empty recipient.- Maker (lines 710-730): Only requires
GetReferredByto return a non-empty referrer.Impact: If a referred taker generates zero rev-share (e.g., due to rounding or zero net fees), their volume won't be attributed even though they have an active referral. Conversely, a referred maker's volume will always be attributed whenever they trade, regardless of whether affiliate rev-share was actually paid.
The code does correctly use the capped
ReferredVolumeQuoteQuantums(lines 703, 727), which addresses the prior concern about attribution cap logic only gating presence.Clarify the intended semantics:
- If stats should track "volume that generated affiliate commission", gate both taker and maker on rev-share result.
- If stats should track "volume under an active referral", remove the rev-share requirement for taker.
Add test coverage for nil rev-share scenarios to document the intended behavior.
🧹 Nitpick comments (1)
protocol/x/clob/keeper/process_single_match_affiliate_stats_test.go (1)
126-254: Test assertions are correct; prior calculation appears to be based on incorrect quantum conversion.The prior review comment claimed the notional exceeds the cap, but recalculating:
- Base quantums: 100,000,000 (1 BTC)
- Subticks: 5,000,000,000 ($50,000 per BTC)
- For BTC-USD (clob pair 0), the quantum conversion exponent is typically -9
- Quote quantums = 100,000,000 × 5,000,000,000 × 10^(-9) = 500,000,000 (500M)
This is well below the 1,000,000,000,000 (1T) cap configured at line 219, so no capping occurs and both taker and maker attributions should receive the full notional.
The test assertions appear correct as written. However, to improve clarity and avoid confusion:
Consider adding a comment in the test documenting the expected notional calculation:
+ // Expected notional: 100M base quantums × 5B subticks × 10^-9 = 500M quote quantums + // This is below the 1T cap, so no capping occurs require.Len(t, fill.AffiliateAttributions, 2, "Should have two attributions (taker and maker)")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d85a18f50b95604aa734316da6ebb85751dad442 and bb75b96.
⛔ Files ignored due to path filters (3)
protocol/x/affiliates/types/affiliates.pb.gois excluded by!**/*.pb.goprotocol/x/affiliates/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/stats/types/stats.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (20)
indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/affiliates.ts(2 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/query.ts(6 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts(14 hunks)proto/dydxprotocol/affiliates/affiliates.proto(1 hunks)proto/dydxprotocol/affiliates/query.proto(1 hunks)proto/dydxprotocol/stats/stats.proto(3 hunks)protocol/x/affiliates/abci.go(0 hunks)protocol/x/affiliates/keeper/grpc_query.go(1 hunks)protocol/x/affiliates/keeper/grpc_query_test.go(5 hunks)protocol/x/affiliates/keeper/keeper.go(1 hunks)protocol/x/affiliates/keeper/keeper_test.go(1 hunks)protocol/x/affiliates/types/expected_keepers.go(1 hunks)protocol/x/clob/keeper/process_single_match.go(3 hunks)protocol/x/clob/keeper/process_single_match_affiliate_stats_test.go(1 hunks)protocol/x/clob/types/expected_keepers.go(2 hunks)protocol/x/revshare/keeper/revshare.go(1 hunks)protocol/x/revshare/keeper/revshare_test.go(2 hunks)protocol/x/stats/keeper/grpc_query_test.go(3 hunks)protocol/x/stats/keeper/keeper.go(4 hunks)protocol/x/stats/keeper/keeper_test.go(8 hunks)
💤 Files with no reviewable changes (1)
- protocol/x/affiliates/abci.go
🚧 Files skipped from review as they are similar to previous changes (6)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/affiliates.ts
- protocol/x/affiliates/keeper/grpc_query.go
- protocol/x/revshare/keeper/revshare_test.go
- protocol/x/revshare/keeper/revshare.go
- proto/dydxprotocol/affiliates/affiliates.proto
- proto/dydxprotocol/affiliates/query.proto
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/affiliates/keeper/keeper.goprotocol/x/clob/types/expected_keepers.goprotocol/x/stats/keeper/keeper_test.goprotocol/x/stats/keeper/keeper.goprotocol/x/affiliates/keeper/keeper_test.goprotocol/x/clob/keeper/process_single_match.goprotocol/x/clob/keeper/process_single_match_affiliate_stats_test.go
📚 Learning: 2024-11-15T16:17:29.092Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/affiliates/keeper/keeper.goprotocol/x/clob/types/expected_keepers.goprotocol/x/stats/keeper/keeper_test.goprotocol/x/stats/keeper/keeper.goprotocol/x/affiliates/keeper/keeper_test.goprotocol/x/affiliates/keeper/grpc_query_test.goprotocol/x/clob/keeper/process_single_match.goprotocol/x/clob/keeper/process_single_match_affiliate_stats_test.goprotocol/x/affiliates/types/expected_keepers.go
📚 Learning: 2024-11-15T15:59:28.095Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:833-850
Timestamp: 2024-11-15T15:59:28.095Z
Learning: The function `GetInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/affiliates/keeper/keeper.goprotocol/x/clob/types/expected_keepers.goprotocol/x/stats/keeper/keeper_test.goprotocol/x/stats/keeper/keeper.goprotocol/x/affiliates/keeper/keeper_test.goprotocol/x/clob/keeper/process_single_match.go
📚 Learning: 2025-05-24T00:45:00.519Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 2837
File: proto/dydxprotocol/clob/order.proto:4-4
Timestamp: 2025-05-24T00:45:00.519Z
Learning: In the dydxprotocol/v4-chain repository, cosmos_proto dependencies are managed through buf.yaml configuration rather than being physically present in the repository. The import "cosmos_proto/cosmos.proto" is a standard pattern used across 28+ proto files for cosmos address annotations like (cosmos_proto.scalar) = "cosmos.AddressString".
Applied to files:
protocol/x/clob/keeper/process_single_match.go
🧬 Code graph analysis (6)
protocol/x/affiliates/keeper/keeper.go (1)
protocol/x/affiliates/types/errors.go (1)
ErrInvalidAffiliateTiers(8-8)
protocol/x/clob/types/expected_keepers.go (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (2)
AffiliateAttribution(58-67)AffiliateAttribution(289-344)protocol/x/stats/types/stats.pb.go (3)
AffiliateAttribution(61-68)AffiliateAttribution(72-72)AffiliateAttribution(73-75)
protocol/x/stats/keeper/keeper_test.go (2)
protocol/x/stats/types/stats.pb.go (25)
AffiliateAttribution(61-68)AffiliateAttribution(72-72)AffiliateAttribution(73-75)BlockStats(125-128)BlockStats(132-132)BlockStats(133-135)BlockStats_Fill(171-186)BlockStats_Fill(190-190)BlockStats_Fill(191-193)UserStats(459-471)UserStats(475-475)UserStats(476-478)GlobalStats(412-415)GlobalStats(419-419)GlobalStats(420-422)AffiliateAttribution_ROLE_MAKER(37-37)EpochStats(304-309)EpochStats(313-313)EpochStats(314-316)EpochStats_UserWithStats(359-362)EpochStats_UserWithStats(366-366)EpochStats_UserWithStats(367-369)StatsMetadata(257-261)StatsMetadata(265-265)StatsMetadata(266-268)protocol/x/clob/types/expected_keepers.go (1)
StatsKeeper(185-195)
protocol/x/affiliates/keeper/grpc_query_test.go (2)
protocol/x/affiliates/types/query.pb.go (3)
AffiliateInfoResponse(82-99)AffiliateInfoResponse(103-103)AffiliateInfoResponse(104-106)protocol/x/stats/types/stats.pb.go (3)
UserStats(459-471)UserStats(475-475)UserStats(476-478)
protocol/x/affiliates/types/expected_keepers.go (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (2)
EpochStats(168-174)EpochStats(528-574)protocol/x/stats/types/stats.pb.go (3)
EpochStats(304-309)EpochStats(313-313)EpochStats(314-316)
indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (1)
protocol/x/stats/types/stats.pb.go (5)
AffiliateAttribution_Role(32-32)AffiliateAttribution_Role(56-58)AffiliateAttribution(61-68)AffiliateAttribution(72-72)AffiliateAttribution(73-75)
⏰ 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). (38)
- GitHub Check: build
- GitHub Check: test / run_command
- GitHub Check: test-coverage-upload
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: container-tests
- 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-auxo-lambda / (auxo) Build and Push Lambda
- 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-bazooka-lambda / (bazooka) Build and Push Lambda
- 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-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- 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-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) 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-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: lint
- GitHub Check: golangci-lint
- GitHub Check: build-and-push-testnet
- GitHub Check: run_command
- GitHub Check: benchmark
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (23)
protocol/x/affiliates/keeper/keeper.go (1)
236-238: LGTM: Staking requirement removed from tier validation.The validation now only enforces strictly increasing volume requirements, which aligns with the deprecation of staking-based affiliate tier progression mentioned in the AI summary. The simplified error message accurately reflects this change.
protocol/x/affiliates/keeper/grpc_query_test.go (2)
13-13: LGTM: Import added for new test functionality.The
statstypesimport is required for the new test case that validates attributed volume handling.
139-172: LGTM: Comprehensive test for attributed volume feature.The new test case properly validates the end-to-end flow of attributed volume by:
- Setting up UserStats with both referred and attributed volumes
- Configuring proper delegations
- Verifying the attributed volume appears correctly in the AffiliateInfoResponse
indexer/packages/v4-protos/src/codegen/dydxprotocol/affiliates/query.ts (1)
51-53: LGTM: Generated protobuf code for attributed volume field.The TypeScript code generation correctly implements the new
attributedVolume_30dRollingfield with:
- Proper field number (7) and wire tag (58) for protobuf encoding/decoding
- Appropriate initialization with
new Uint8Array()- Complete coverage in encode, decode, and fromPartial methods
Also applies to: 86-88, 281-282, 312-314, 352-354, 373-373
protocol/x/clob/types/expected_keepers.go (2)
186-193: LGTM: RecordFill signature extended for attribution tracking.The addition of the
affiliateAttributionsparameter enables fill-level affiliate attribution tracking, which aligns with the PR's objective to move from block-level to fill-level attribution.
233-233: LGTM: GetReferredBy method addition.The new method uses idiomatic Go patterns (value + found bool) for looking up referral relationships, supporting the expanded attribution functionality.
protocol/x/affiliates/types/expected_keepers.go (1)
15-16: LGTM: Epoch-level stats accessors added.The new methods enable reading and writing epoch-scoped statistics, supporting the per-epoch attribution tracking mentioned in the PR summary.
protocol/x/affiliates/keeper/keeper_test.go (1)
320-352: LGTM: Comprehensive test coverage for updated tier validation.The new test cases appropriately validate:
- Cap enforcement for
TakerFeeSharePpm(safety check)- Support for deprecated
req_staked_whole_coinsfield set to 0- Mixed staking values including 0 (backward compatibility)
These tests align with the deprecation of staking-based tier progression while maintaining backward compatibility.
protocol/x/stats/keeper/grpc_query_test.go (1)
132-138: LGTM: UserStats extended with affiliate attribution fields.The test data now includes the new affiliate attribution fields (
Affiliate_30DRevenueGeneratedQuantums,Affiliate_30DReferredVolumeQuoteQuantums,Affiliate_30DAttributedVolumeQuoteQuantums), enabling comprehensive testing of the attribution feature.proto/dydxprotocol/stats/stats.proto (3)
9-24: LGTM: Well-designed AffiliateAttribution message.The new message appropriately captures affiliate attribution data with:
- Clear role differentiation (TAKER/MAKER) via enum
- Referrer address for tracking the affiliate
- Referred volume in quote quantums for attribution calculations
The structure supports both per-fill attribution tracking and the cap enforcement mentioned in the PR objectives.
45-47: LGTM: Fill-level attribution tracking enabled.Adding
affiliate_attributionsas a repeated field onBlockStats.Fillenables the shift from block-level to fill-level attribution tracking, which is core to fixing the 30d volume issue mentioned in the PR objectives.
98-100: LGTM: UserStats extended with attributed volume.The new
affiliate_30d_attributed_volume_quote_quantumsfield enables tracking of volume attributed to affiliates over the 30-day window, supporting proper volume cap enforcement per the PR objectives.protocol/x/stats/keeper/keeper.go (1)
82-102: RecordFill signature change looks correct.The addition of
affiliateAttributionsparameter and its storage inBlockStats_Fillaligns with the PR's goal of tracking attributions at the fill level. The function appropriately delegates validation to the caller (clob keeper).protocol/x/clob/keeper/process_single_match.go (1)
580-594: Attribution integration with RecordFill is correct.The call to
buildAttributableVolumeAttributionsand passing of results toRecordFillis clean and properly integrates the new attribution tracking with the stats keeper.protocol/x/stats/keeper/keeper_test.go (4)
28-155: TestRecordFill expectations now match inputs correctly.The test cases properly set
ReferredVolumeQuoteQuantumsin both the inputaffiliateAttributionsand the expectedBlockStats_Fill.AffiliateAttributions, resolving the prior mismatch concern. The test correctly validates thatRecordFillstores attributions as provided.
257-477: Attribution test expectations are now correctly aligned with implementation.The tests properly pass already-capped
ReferredVolumeQuoteQuantumsvalues inAffiliateAttributionsand expectProcessBlockStatsto aggregate those values as-is. This correctly reflects the design where:
- Cap enforcement happens in clob keeper (
getAttributableVolume)- Stats keeper aggregates the capped volumes passed via
affiliateAttributionsThe test at lines 350-386 demonstrates a fill with notional 100B but attributed volume capped at 30B, which correctly validates that
ProcessBlockStatsuses the providedReferredVolumeQuoteQuantums(30B) rather than the fill'sNotional(100B).This resolves the prior concern about test expectations not matching implementation behavior.
480-607: TestExpireOldStats correctly validates attribution volume expiration.The test properly verifies that both
Affiliate_30DReferredVolumeQuoteQuantums(referrer) andAffiliate_30DAttributedVolumeQuoteQuantums(referee) are decremented when epoch stats expire, validating the fix for the PR's stated issue of "30d volume not being removed."
609-867: Excellent test coverage for cap equilibrium behavior.This comprehensive test validates that a high-volume trader at the attribution cap can continue receiving new attributions as old epoch stats expire. The test correctly interleaves
ExpireOldStatswithProcessBlockStatsto simulate realistic ongoing trading while maintaining the user at the cap equilibrium.This provides strong evidence that there is no equilibrium trap—users can maintain active attribution status even while continuously trading at high volume.
protocol/x/clob/keeper/process_single_match_affiliate_stats_test.go (1)
256-890: Comprehensive test coverage for attribution cap scenarios.The test suite provides excellent coverage of attribution cap behavior:
- No referrers (256-336): Validates empty attributions when neither party is referred
- Volume cap applied (338-457): Large trade with cap enforcement
- Already at cap (459-580): No attribution when referee is at cap
- Over cap (582-703): No attribution when referee exceeds cap
- Cap with expiration (705-890): Attribution resumes after old volume expires
These tests thoroughly validate the
getAttributableVolumelogic and its integration withbuildAttributableVolumeAttributions.indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (4)
6-79: AffiliateAttribution structure correctly matches the proto definition.Contrary to the prior review comment claiming a missing
refereeAddressfield, the TypeScript interfaces correctly implement all three fields defined in the Go protobuf (fromprotocol/x/stats/types/stats.pb.go:60-67):
role(AffiliateAttribution_Role)referrerAddress(string)referredVolumeQuoteQuantums(Long)The referee's address is implicitly determined by the attribution's role (taker or maker) and the corresponding fill's taker/maker addresses, rather than being duplicated as a separate field. This design avoids redundancy.
If proto verification is still failing, the issue lies elsewhere, not in missing fields.
281-344: AffiliateAttribution serialization is correctly implemented.The encode/decode/fromPartial methods properly handle all three fields with correct protobuf wire tags:
- Field 1 (role): tag 8 (wire type varint)
- Field 2 (referrerAddress): tag 18 (wire type string)
- Field 3 (referredVolumeQuoteQuantums): tag 24 (wire type varint for uint64)
391-473: BlockStats_Fill correctly implements affiliateAttributions field.The repeated
affiliateAttributionsfield (proto field 5) is properly implemented:
- Initialized as empty array in
createBaseBlockStats_Fill- Encoded using tag 42 (5*8 + 2 for length-delimited repeated field)
- Decoded by pushing each decoded
AffiliateAttributionto the array- Mapped in
fromPartialusingAffiliateAttribution.fromPartial
676-757: UserStats correctly implements affiliate_30dAttributedVolumeQuoteQuantums field.The new
affiliate_30dAttributedVolumeQuoteQuantumsfield (proto field 5) is properly implemented:
- Initialized as
Long.UZEROincreateBaseUserStats- Encoded using tag 40 (5*8 for uint64 varint)
- Decoded and assigned as
Longtype- Handled in
fromPartialwith null-safety
|
@Mergifyio backport release/protocol/v9.5.x |
✅ Backports have been createdDetails
|
… capped (backport #3250) (#3256) Co-authored-by: Justin Barnett <[email protected]>
3 participants
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
Release Notes
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.