9.5 upgrade & add underflow protection for referred volume#3261
9.5 upgrade & add underflow protection for referred volume#3261
Conversation
WalkthroughAdds a v9.5 upgrade that migrates users' 30-day referred volume into epoch-level stats, wires a v9.5 upgrade handler, adds underflow protection and an address-enumeration helper to the stats keeper, updates testing version files, adds unit and container tests for the migration, and removes the v9.4 container test. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator as Upgrade Trigger
participant Handler as CreateUpgradeHandler
participant Migration as Migrate30dReferredVolumeToEpochStats
participant Stats as StatsKeeper
participant Epochs as EpochsKeeper
participant MM as ModuleManager
Operator->>Handler: start upgrade
Handler->>Migration: run migration(ctx, statsKeeper, epochsKeeper)
Migration->>Stats: GetAllAddressesWithReferredVolume()
Stats-->>Migration: addresses[]
rect rgba(150,200,255,0.14)
note right of Migration: For each address with 30d referred volume
Migration->>Epochs: GetCurrentEpoch()
Epochs-->>Migration: epoch
Migration->>Epochs: GetEpochStats(addr, epoch)
Epochs-->>Migration: epochStats (or nil)
Migration->>Migration: merge 30d referred volume -> epochStats
Migration->>Epochs: SetEpochStats(updated)
Migration->>Migration: log per-address migration
end
Handler->>MM: mm.RunMigrations(configurator)
MM-->>Handler: done
Handler-->>Operator: upgrade complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 1
🧹 Nitpick comments (2)
protocol/x/stats/keeper/keeper_test.go (1)
1070-1075: Redundant assertions after confirming value is 0.Lines 1065-1068 already assert the values equal 0. The subsequent
require.Lessassertions on lines 1072-1075 will always pass since 0 < 1000.Consider removing these redundant checks or restructuring the test to use only one assertion style.
protocol/app/upgrades/v9.5/upgrade.go (1)
54-58: Unnecessary nil check:GetUserStatsnever returns nil.Looking at
keeper.golines 149-160,GetUserStatsalways returns a pointer to a struct (either unmarshaled data or an empty&types.UserStats{}), nevernil.The nil check on line 56 will never be true and can be removed:
globalUserStats := statsKeeper.GetUserStats(ctx, address) -if globalUserStats == nil { - continue -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
protocol/app/upgrades/v9.5/constants.go(1 hunks)protocol/app/upgrades/v9.5/upgrade.go(1 hunks)protocol/app/upgrades/v9.5/upgrade_test.go(1 hunks)protocol/x/stats/keeper/keeper.go(2 hunks)protocol/x/stats/keeper/keeper_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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/stats/keeper/keeper.goprotocol/x/stats/keeper/keeper_test.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/stats/keeper/keeper.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/stats/keeper/keeper.goprotocol/x/stats/keeper/keeper_test.go
🧬 Code graph analysis (3)
protocol/x/stats/keeper/keeper.go (2)
protocol/x/stats/types/keys.go (1)
UserStatsKeyPrefix(21-21)protocol/x/stats/types/stats.pb.go (3)
UserStats(459-471)UserStats(475-475)UserStats(476-478)
protocol/app/upgrades/v9.5/upgrade.go (4)
protocol/x/stats/keeper/keeper.go (1)
Keeper(23-30)protocol/mocks/Configurator.go (1)
Configurator(15-17)protocol/lib/context.go (1)
UnwrapSDKContext(31-55)protocol/app/upgrades/v9.5/constants.go (1)
UpgradeName(9-9)
protocol/x/stats/keeper/keeper_test.go (1)
protocol/x/stats/types/stats.pb.go (15)
EpochStats(304-309)EpochStats(313-313)EpochStats(314-316)EpochStats_UserWithStats(359-362)EpochStats_UserWithStats(366-366)EpochStats_UserWithStats(367-369)UserStats(459-471)UserStats(475-475)UserStats(476-478)GlobalStats(412-415)GlobalStats(419-419)GlobalStats(420-422)StatsMetadata(257-261)StatsMetadata(265-265)StatsMetadata(266-268)
⏰ 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). (28)
- 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-vulcan / (vulcan) 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: (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-socks / (socks) 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-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) 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: (Mainnet) 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-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: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: golangci-lint
- GitHub Check: build
- GitHub Check: container-tests
- GitHub Check: build-and-push-testnet
- GitHub Check: build-and-push-mainnet
- GitHub Check: benchmark
- GitHub Check: test-coverage-upload
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (9)
protocol/x/stats/keeper/keeper_test.go (3)
1061-1063: Note:TakerNotionalandMakerNotionalstill lack underflow protection.The comment acknowledges that
TakerNotionalandMakerNotionalcan wrap around for uint64. Looking atkeeper.golines 332-334, these fields are subtracted without clamping:stats.TakerNotional -= removedStats.Stats.TakerNotional stats.MakerNotional -= removedStats.Stats.MakerNotional stats.Affiliate_30DRevenueGeneratedQuantums -= removedStats.Stats.Affiliate_30DRevenueGeneratedQuantumsIf the same data inconsistency scenario occurs for these fields, they would underflow. Consider whether consistent clamping should be applied to all uint64 stats fields.
1078-1128: LGTM! Edge case test is well-structured.The test correctly verifies exact boundary behavior where user stats equal epoch stats, ensuring the result is exactly 0 after subtraction.
1130-1221: LGTM! Multi-user test provides comprehensive coverage.The test effectively covers three scenarios in one: normal subtraction (Alice), underflow clamping (Bob), and exact boundary (Carl). This ensures the underflow protection works correctly regardless of other users' data.
protocol/x/stats/keeper/keeper.go (1)
454-473: LGTM! New helper method is well-implemented.The method correctly:
- Uses prefix store with the proper key prefix
- Properly closes the iterator with
defer- Filters for non-zero referred volume only
- Returns addresses as strings (matching the key type)
The full iteration is acceptable for a one-time migration operation.
protocol/app/upgrades/v9.5/constants.go (1)
1-15: LGTM! Standard upgrade constants structure.The upgrade constants follow the established pattern with the correct version naming and empty
StoreUpgradessince this upgrade doesn't modify store structure.protocol/app/upgrades/v9.5/upgrade_test.go (2)
13-124: LGTM! Comprehensive migration test.The test effectively covers:
- Migration preserves existing
TakerNotional/MakerNotionalvalues (Alice, Bob)- Users with referred volume but no trading activity are added (Carl)
- Users without referred volume are excluded (Dave)
- Verifies pre-migration state has zero referred volume in epoch stats
126-169: LGTM! Empty epoch stats edge case covered.Good test coverage for the scenario where no epoch stats exist initially, ensuring the migration creates new epoch stats correctly.
protocol/app/upgrades/v9.5/upgrade.go (2)
73-76: Potential issue: Addition instead of assignment could double-count referred volume.Line 75 adds to existing
Affiliate_30DReferredVolumeQuoteQuantums:epochUserStats.Stats.Affiliate_30DReferredVolumeQuoteQuantums += referredVolumeHowever, looking at the test setup on lines 28-47 of upgrade_test.go, the initial epoch stats have no referred volume (comment: "No referred volume set yet"). If epoch stats were to already contain referred volume from normal trading activity during the epoch, this addition would result in double-counting.
Consider whether this should be an assignment instead:
-epochUserStats.Stats.Affiliate_30DReferredVolumeQuoteQuantums += referredVolume +epochUserStats.Stats.Affiliate_30DReferredVolumeQuoteQuantums = referredVolumeOr clarify in the PR objectives if additive behavior is intentional.
108-125: LGTM! Upgrade handler follows established patterns.The handler correctly:
- Unwraps the SDK context with the module name
- Logs upgrade start/completion
- Runs the migration before standard module migrations
- Returns the result of
RunMigrations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
protocol/app/upgrades/v9.5/upgrade.go (3)
60-76: Defensive guard for potentially nilStatson existing epoch entriesWhen
epochUserStatsalready exists (Line 62), the code assumesepochUserStats.Statsis non‑nil before incrementingAffiliate_30DReferredVolumeQuoteQuantums(Line 75). If any historical data were written with a nilStatspointer, this would panic at upgrade time.Even if current invariants guarantee
Stats != nil, a small defensive guard keeps this migration robust against legacy/partial data:- } else { - // User already in epoch stats, add the referred volume - epochUserStats.Stats.Affiliate_30DReferredVolumeQuoteQuantums += referredVolume - } + } else { + // User already in epoch stats, add the referred volume + if epochUserStats.Stats == nil { + epochUserStats.Stats = &statstypes.UserStats{} + } + epochUserStats.Stats.Affiliate_30DReferredVolumeQuoteQuantums += referredVolume + }
80-86: Per‑address Info logs may be excessively verbose on large mainnet setsLogging an
Infoline for every migrated address (including the running count and volume) could generate a very large number of log lines during upgrade on mainnet, which may bloat node logs and slow I/O.Consider either:
- lowering this to
Debug(if available in your logger setup), or- sampling (e.g., log every Nth address), or
- aggregating into a small number of summary logs.
116-125: “Successfully completed … Upgrade” log emitted beforeRunMigrationsresultThe handler logs
"Successfully completed %s Upgrade"(Lines 123–123) before callingmm.RunMigrations(Line 125). IfRunMigrationsreturns an error, logs will still say the upgrade completed successfully, which is misleading for operators.Consider restructuring to log success only after migrations complete, and optionally log on error:
- // Migrate 30d referred volume to epoch stats - Migrate30dReferredVolumeToEpochStats(sdkCtx, statsKeeper, epochsKeeper) - - sdkCtx.Logger().Info(fmt.Sprintf("Successfully completed %s Upgrade", UpgradeName)) - - return mm.RunMigrations(ctx, configurator, vm) + // Migrate 30d referred volume to epoch stats + Migrate30dReferredVolumeToEpochStats(sdkCtx, statsKeeper, epochsKeeper) + + newVM, err := mm.RunMigrations(ctx, configurator, vm) + if err != nil { + sdkCtx.Logger().Error(fmt.Sprintf("Error during %s Upgrade: %v", UpgradeName, err)) + return nil, err + } + + sdkCtx.Logger().Info(fmt.Sprintf("Successfully completed %s Upgrade", UpgradeName)) + return newVM, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/app/upgrades/v9.5/upgrade.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🧬 Code graph analysis (1)
protocol/app/upgrades/v9.5/upgrade.go (4)
protocol/x/stats/keeper/keeper.go (1)
Keeper(23-30)protocol/mocks/Configurator.go (1)
Configurator(15-17)protocol/lib/context.go (1)
UnwrapSDKContext(31-55)protocol/app/upgrades/v9.5/constants.go (1)
UpgradeName(9-9)
⏰ 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). (28)
- 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-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-ecs-service-ender / (ender) Build and Push
- 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: (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-vulcan / (vulcan) 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-ender / (ender) 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-roundtable / (roundtable) Build and Push
- GitHub Check: build-and-push-testnet
- GitHub Check: benchmark
- GitHub Check: golangci-lint
- GitHub Check: test-coverage-upload
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: liveness-test
- GitHub Check: build
- GitHub Check: test-race
- GitHub Check: build-and-push-mainnet
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: container-tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
protocol/app/upgrades/v9.5/upgrade.go (1)
89-98: Deterministic rebuild fromuserStatsMaplooks goodSorting
keysbefore rebuildingepochStats.Stats(Lines 89–98) ensures deterministic ordering and avoids map‑iteration nondeterminism in state hashes. This is the right pattern for Cosmos SDK state migrations.
|
@mergify backport release/protocol/v9.5.x |
✅ Backports have been createdDetails
|
(cherry picked from commit 4c4f705)
…3261) (#3271) 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
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.