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 (2)
protocol/x/stats/keeper/keeper.go (2)
334-352: Affiliate underflow guards are correct; consider optional minor polish.The comparison-before-subtract pattern on the three affiliate-related fields correctly prevents uint64 underflow, including the equality case. If you expect these clamps to ever trigger due to state inconsistencies, you might optionally add a debug log in the
elsebranches or extract a tinysafeSubUint64helper to de-duplicate the pattern; not required, current implementation is fine as-is.
460-478:GetAllAddressesWithReferredVolumeis correct; keep scalability in mind for very large state.Iterating the
UserStatsprefix store and filtering onAffiliate_30DReferredVolumeQuoteQuantums > 0is a clear, correct way to surface addresses for the migration, and returning raw address strings matches the existingGetUserStats/SetUserStatsAPI. If this ever needs to run over millions of users, consider a streaming/chunked migration that processes entries during iteration instead of accumulating all addresses into a single slice, to cap peak memory usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
protocol/app/upgrades/v9.4/upgrade_container_test.go(0 hunks)protocol/scripts/genesis/sample_pregenesis.json(1 hunks)protocol/x/stats/keeper/keeper.go(2 hunks)
💤 Files with no reviewable changes (1)
- protocol/app/upgrades/v9.4/upgrade_container_test.go
✅ Files skipped from review due to trivial changes (1)
- protocol/scripts/genesis/sample_pregenesis.json
🧰 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.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.go
🧬 Code graph analysis (1)
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)
⏰ 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-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-vulcan / (vulcan) 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-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: (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-bazooka-lambda / (bazooka) 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-vulcan / (vulcan) 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-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: container-tests
- GitHub Check: build
- GitHub Check: golangci-lint
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build-and-push-mainnet
- GitHub Check: benchmark
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-coverage-upload
- GitHub Check: test-race
- GitHub Check: liveness-test
- GitHub Check: build-and-push-testnet
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
protocol/app/upgrades/v9.5/upgrade_container_test.go (1)
81-124: Consider verifying that global user stats remain unchanged after migration.The test thoroughly verifies the epoch stats after migration, but it doesn't check whether the global user stats (set via
SetUserStats) remain unchanged. If the migration is intended to copy data rather than move it, adding assertions to verify that global stats are preserved would strengthen the test coverage.Example verification you could add after line 124:
+ // Verify global user stats remain unchanged after migration + for _, user := range testUsers { + globalStats := statsKeeper.GetUserStats(ctx, user.address) + require.Equal(t, user.referredVolume, globalStats.Affiliate_30DReferredVolumeQuoteQuantums, + "Global referred volume should remain unchanged for user %s", user.address) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/app/upgrades/v9.5/upgrade_container_test.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_container_test.go (3)
protocol/testutil/app/app.go (2)
NewTestAppBuilder(335-350)AdvanceToBlockOptions(125-152)protocol/app/app.go (1)
App(271-376)protocol/testutil/constants/addresses.go (4)
AliceAccAddress(10-10)BobAccAddress(11-11)CarlAccAddress(12-12)DaveAccAddress(13-13)
⏰ 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). (26)
- GitHub Check: test-coverage-upload
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: benchmark
- GitHub Check: golangci-lint
- GitHub Check: container-tests
- GitHub Check: check-sample-pregenesis-up-to-date
- 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-ender / (ender) 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-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-comlink / (comlink) 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-roundtable / (roundtable) 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-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-vulcan / (vulcan) 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: build-and-push-testnet
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (4)
protocol/app/upgrades/v9.5/upgrade_container_test.go (4)
1-11: LGTM! Clean imports and package structure.The package declaration and imports are appropriate for the test file.
13-68: Test setup is comprehensive and well-organized.The test setup covers multiple scenarios with clear test data structure. The inclusion of Dave with zero referred volume is a good edge case to verify that users without referred volume are not migrated.
126-169: LGTM! Excellent edge case coverage.The empty epoch stats test effectively validates the migration behavior when no epoch stats exist initially. The assertions confirm that users with referred volume are properly added with correct values and zero trading notional.
79-79: The functionmigrate30dReferredVolumeToEpochStatsexists and is properly accessible.The function is defined in
protocol/app/upgrades/v9.5/upgrade.goat line 19 with the exact signature used in the test:func migrate30dReferredVolumeToEpochStats(ctx sdk.Context, statsKeeper statskeeper.Keeper, epochsKeeper epochskeeper.Keeper). Since both the test file and the function definition are in the same package, the function is fully accessible with no visibility issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
protocol/app/upgrades/v9.5/upgrade_container_test.go (1)
126-153: LeverageTestStateUpgradehooks to assert referred-volume migration end-to-end
TestStateUpgradesuccessfully wires up a pre‑upgrade testnet and runs the v9.5 software upgrade, butpreUpgradeSetups,preUpgradeChecks, andpostUpgradeChecksare currently no‑ops. That means this test only verifies that the chain upgrades without error, not that 30‑day referred volume is actually migrated as expected in a real node.Non‑blocking for this PR, but it would be valuable to:
- In
preUpgradeSetups, create users with 30d referred volume in global stats (via txs or appropriate module messages).- In
preUpgradeChecks, assert the pre‑upgrade shape (e.g., global stats populated, epoch stats empty).- In
postUpgradeChecks, query epoch stats and verify that the referred volume has been migrated according to the same expectations as the unit tests.That will give you an end‑to‑end, “real node” safeguard around the v9.5 migration in addition to the in‑process keeper tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
protocol/app/upgrades/v9.5/upgrade.go(1 hunks)protocol/app/upgrades/v9.5/upgrade_container_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/app/upgrades/v9.5/upgrade.go
🧰 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_container_test.go (5)
protocol/testutil/app/app.go (1)
NewTestAppBuilder(335-350)protocol/testutil/constants/addresses.go (4)
AliceAccAddress(10-10)BobAccAddress(11-11)CarlAccAddress(12-12)DaveAccAddress(13-13)protocol/testing/containertest/testnet.go (1)
NewTestnetWithPreupgradeGenesis(79-86)protocol/testing/containertest/testnet_utils.go (1)
UpgradeTestnet(24-66)protocol/testing/containertest/node.go (1)
Node(35-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). (26)
- 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: (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-roundtable / (roundtable) 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: (Mainnet) 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-roundtable / (roundtable) 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-ender / (ender) 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-comlink / (comlink) 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-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: liveness-test
- GitHub Check: build
- GitHub Check: build-and-push-testnet
- GitHub Check: test-coverage-upload
- GitHub Check: golangci-lint
- GitHub Check: benchmark
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: container-tests
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
| func TestMigrate30dReferredVolumeToEpochStats(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| tApp.InitChain() | ||
|
|
||
| statsKeeper := tApp.App.StatsKeeper | ||
| epochsKeeper := tApp.App.EpochsKeeper | ||
|
|
||
| // Advance to the next epoch so we have a current epoch with some activity | ||
| ctx := tApp.AdvanceToBlock(10, testapp.AdvanceToBlockOptions{}) | ||
|
|
||
| // Get current epoch directly from keeper instead of using containertest.Query | ||
| statsEpochInfo := epochsKeeper.MustGetStatsEpochInfo(ctx) | ||
| currentEpoch := statsEpochInfo.CurrentEpoch | ||
|
|
||
| // Create some initial epoch stats (simulating existing trading activity) | ||
| initialEpochStats := &statstypes.EpochStats{ | ||
| Stats: []*statstypes.EpochStats_UserWithStats{ | ||
| { | ||
| User: constants.AliceAccAddress.String(), | ||
| Stats: &statstypes.UserStats{ | ||
| TakerNotional: 50, | ||
| MakerNotional: 75, | ||
| // No referred volume set yet | ||
| }, | ||
| }, | ||
| { | ||
| User: constants.BobAccAddress.String(), | ||
| Stats: &statstypes.UserStats{ | ||
| TakerNotional: 100, | ||
| MakerNotional: 150, | ||
| // No referred volume set yet | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| statsKeeper.SetEpochStats(ctx, currentEpoch, initialEpochStats) | ||
|
|
||
| // Set up global user stats with referred volume (this represents the 30d cumulative volume) | ||
| testUsers := []struct { | ||
| address string | ||
| referredVolume uint64 | ||
| }{ | ||
| {constants.AliceAccAddress.String(), 1_000_000_000}, // 1k volume | ||
| {constants.BobAccAddress.String(), 5_000_000_000}, // 5k volume | ||
| {constants.CarlAccAddress.String(), 10_000_000_000}, // 10k volume | ||
| {constants.DaveAccAddress.String(), 0}, // no referred volume | ||
| } | ||
|
|
||
| for _, user := range testUsers { | ||
| userStats := &statstypes.UserStats{ | ||
| TakerNotional: 100, | ||
| MakerNotional: 200, | ||
| Affiliate_30DReferredVolumeQuoteQuantums: user.referredVolume, | ||
| } | ||
| statsKeeper.SetUserStats(ctx, user.address, userStats) | ||
| } | ||
|
|
||
| // Verify initial state - epoch stats should not have referred volume | ||
| preUpgradeEpochStats := statsKeeper.GetEpochStatsOrNil(ctx, currentEpoch) | ||
| require.NotNil(t, preUpgradeEpochStats) | ||
| for _, userStats := range preUpgradeEpochStats.Stats { | ||
| require.Equal(t, uint64(0), userStats.Stats.Affiliate_30DReferredVolumeQuoteQuantums, | ||
| "Referred volume should be 0 before migration for user %s", userStats.User) | ||
| } | ||
|
|
||
| // Run the migration function directly | ||
| v_9_5.Migrate30dReferredVolumeToEpochStats(ctx, statsKeeper, epochsKeeper) | ||
|
|
||
| // Verify migration results | ||
| postUpgradeEpochStats := statsKeeper.GetEpochStatsOrNil(ctx, currentEpoch) | ||
| require.NotNil(t, postUpgradeEpochStats) | ||
|
|
||
| // Create a map for easier verification |
There was a problem hiding this comment.
Complete TestMigrate30dReferredVolumeToEpochStats: missing closing brace and core assertions
This test currently never closes the function (no terminating }) and stops right after checking that postUpgradeEpochStats is non‑nil. It doesn’t actually assert anything about which users ended up in epoch stats or what their referred volumes are, so it won’t guard the migration logic.
Consider:
- Adding a map from user address → stats to validate the migration for each of
testUsers. - Verifying that users with non‑zero referred volume have their epoch stats populated with the expected
Affiliate_30DReferredVolumeQuoteQuantums. - Verifying that users with zero referred volume are handled as intended (either absent from epoch stats or present with zero, depending on your migration contract).
- Closing the function with a
}.
For example, something along these lines would both fix the syntax and exercise the migration:
// Verify migration results
postUpgradeEpochStats := statsKeeper.GetEpochStatsOrNil(ctx, currentEpoch)
require.NotNil(t, postUpgradeEpochStats)
- // Create a map for easier verification
-
+ // Create a map for easier verification
+ statsByUser := make(map[string]*statstypes.UserStats)
+ for _, u := range postUpgradeEpochStats.Stats {
+ statsByUser[u.User] = u.Stats
+ }
+
+ for _, user := range testUsers {
+ stats, ok := statsByUser[user.address]
+
+ if user.referredVolume == 0 {
+ // Adjust this expectation if zero-volume users should still appear in epoch stats.
+ require.False(t, ok, "expected no epoch stats entry for user %s", user.address)
+ continue
+ }
+
+ require.True(t, ok, "expected epoch stats entry for user %s", user.address)
+ require.Equal(
+ t,
+ user.referredVolume,
+ stats.Affiliate_30DReferredVolumeQuoteQuantums,
+ "unexpected referred volume for user %s",
+ user.address,
+ )
+ }
+}Adjust the zero‑volume behavior (Dave) to match how Migrate30dReferredVolumeToEpochStats is intended to behave, but you’ll at least have a concrete check that the per‑user migration is correct.
| func TestMigrate30dReferredVolumeToEpochStats_EmptyEpochStats(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| tApp.InitChain() | ||
|
|
||
| statsKeeper := tApp.App.StatsKeeper | ||
| epochsKeeper := tApp.App.EpochsKeeper | ||
|
|
||
| // Advance to next epoch | ||
| ctx := tApp.AdvanceToBlock(10, testapp.AdvanceToBlockOptions{}) | ||
|
|
||
| // Get current epoch directly from keeper instead of using containertest.Query | ||
| statsEpochInfo := epochsKeeper.MustGetStatsEpochInfo(ctx) | ||
| currentEpoch := statsEpochInfo.CurrentEpoch | ||
|
|
||
| // Setup: Create user stats with referred volume but no epoch stats | ||
| userStats := &statstypes.UserStats{ | ||
| TakerNotional: 100, | ||
| MakerNotional: 200, | ||
| Affiliate_30DReferredVolumeQuoteQuantums: 1_000_000_000, | ||
| } | ||
| statsKeeper.SetUserStats(ctx, constants.AliceAccAddress.String(), userStats) | ||
|
|
||
| // Verify no epoch stats exist initially | ||
| preUpgradeEpochStats := statsKeeper.GetEpochStatsOrNil(ctx, currentEpoch) | ||
| require.Nil(t, preUpgradeEpochStats) | ||
|
|
||
| // Run the migration function directly | ||
| v_9_5.Migrate30dReferredVolumeToEpochStats(ctx, statsKeeper, epochsKeeper) | ||
|
|
||
| // Verify Alice was added to epoch stats even though she wasn't trading | ||
| postUpgradeEpochStats := statsKeeper.GetEpochStatsOrNil(ctx, currentEpoch) | ||
| require.NotNil(t, postUpgradeEpochStats) | ||
| require.Len(t, postUpgradeEpochStats.Stats, 1, | ||
| "Alice should be added to epoch stats because she has referred volume") | ||
|
|
||
| aliceStats := postUpgradeEpochStats.Stats[0] |
There was a problem hiding this comment.
Finish TestMigrate30dReferredVolumeToEpochStats_EmptyEpochStats: use aliceStats and close the function
This test also lacks a terminating } and declares aliceStats without ever asserting on it, which will fail to compile and leaves the main behavior (epoch stats created for non‑trading referrer) untested.
You likely want to:
- Assert that the sole stats entry is for Alice.
- Assert that Alice’s referred volume in epoch stats matches her global
UserStats. - Optionally assert your chosen behavior for taker/maker notionals when a user only has referred volume but no prior epoch stats.
For example:
// Verify Alice was added to epoch stats even though she wasn't trading
postUpgradeEpochStats := statsKeeper.GetEpochStatsOrNil(ctx, currentEpoch)
require.NotNil(t, postUpgradeEpochStats)
require.Len(t, postUpgradeEpochStats.Stats, 1,
"Alice should be added to epoch stats because she has referred volume")
- aliceStats := postUpgradeEpochStats.Stats[0]
+ aliceStats := postUpgradeEpochStats.Stats[0]
+ require.Equal(t, constants.AliceAccAddress.String(), aliceStats.User)
+ require.Equal(
+ t,
+ uint64(1_000_000_000),
+ aliceStats.Stats.Affiliate_30DReferredVolumeQuoteQuantums,
+ "expected referred volume to be migrated into epoch stats for Alice",
+ )
+
+ // If the migration is supposed to keep epoch notionals at zero for non-trading users,
+ // add these checks (otherwise adjust to the intended behavior):
+ // require.Equal(t, uint64(0), aliceStats.Stats.TakerNotional)
+ // require.Equal(t, uint64(0), aliceStats.Stats.MakerNotional)
+}This will both fix the compilation issue and make the “no prior epoch stats” scenario a meaningful regression test for the migration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestMigrate30dReferredVolumeToEpochStats_EmptyEpochStats(t *testing.T) { | |
| tApp := testapp.NewTestAppBuilder(t).Build() | |
| tApp.InitChain() | |
| statsKeeper := tApp.App.StatsKeeper | |
| epochsKeeper := tApp.App.EpochsKeeper | |
| // Advance to next epoch | |
| ctx := tApp.AdvanceToBlock(10, testapp.AdvanceToBlockOptions{}) | |
| // Get current epoch directly from keeper instead of using containertest.Query | |
| statsEpochInfo := epochsKeeper.MustGetStatsEpochInfo(ctx) | |
| currentEpoch := statsEpochInfo.CurrentEpoch | |
| // Setup: Create user stats with referred volume but no epoch stats | |
| userStats := &statstypes.UserStats{ | |
| TakerNotional: 100, | |
| MakerNotional: 200, | |
| Affiliate_30DReferredVolumeQuoteQuantums: 1_000_000_000, | |
| } | |
| statsKeeper.SetUserStats(ctx, constants.AliceAccAddress.String(), userStats) | |
| // Verify no epoch stats exist initially | |
| preUpgradeEpochStats := statsKeeper.GetEpochStatsOrNil(ctx, currentEpoch) | |
| require.Nil(t, preUpgradeEpochStats) | |
| // Run the migration function directly | |
| v_9_5.Migrate30dReferredVolumeToEpochStats(ctx, statsKeeper, epochsKeeper) | |
| // Verify Alice was added to epoch stats even though she wasn't trading | |
| postUpgradeEpochStats := statsKeeper.GetEpochStatsOrNil(ctx, currentEpoch) | |
| require.NotNil(t, postUpgradeEpochStats) | |
| require.Len(t, postUpgradeEpochStats.Stats, 1, | |
| "Alice should be added to epoch stats because she has referred volume") | |
| aliceStats := postUpgradeEpochStats.Stats[0] | |
| func TestMigrate30dReferredVolumeToEpochStats_EmptyEpochStats(t *testing.T) { | |
| tApp := testapp.NewTestAppBuilder(t).Build() | |
| tApp.InitChain() | |
| statsKeeper := tApp.App.StatsKeeper | |
| epochsKeeper := tApp.App.EpochsKeeper | |
| // Advance to next epoch | |
| ctx := tApp.AdvanceToBlock(10, testapp.AdvanceToBlockOptions{}) | |
| // Get current epoch directly from keeper instead of using containertest.Query | |
| statsEpochInfo := epochsKeeper.MustGetStatsEpochInfo(ctx) | |
| currentEpoch := statsEpochInfo.CurrentEpoch | |
| // Setup: Create user stats with referred volume but no epoch stats | |
| userStats := &statstypes.UserStats{ | |
| TakerNotional: 100, | |
| MakerNotional: 200, | |
| Affiliate_30DReferredVolumeQuoteQuantums: 1_000_000_000, | |
| } | |
| statsKeeper.SetUserStats(ctx, constants.AliceAccAddress.String(), userStats) | |
| // Verify no epoch stats exist initially | |
| preUpgradeEpochStats := statsKeeper.GetEpochStatsOrNil(ctx, currentEpoch) | |
| require.Nil(t, preUpgradeEpochStats) | |
| // Run the migration function directly | |
| v_9_5.Migrate30dReferredVolumeToEpochStats(ctx, statsKeeper, epochsKeeper) | |
| // Verify Alice was added to epoch stats even though she wasn't trading | |
| postUpgradeEpochStats := statsKeeper.GetEpochStatsOrNil(ctx, currentEpoch) | |
| require.NotNil(t, postUpgradeEpochStats) | |
| require.Len(t, postUpgradeEpochStats.Stats, 1, | |
| "Alice should be added to epoch stats because she has referred volume") | |
| aliceStats := postUpgradeEpochStats.Stats[0] | |
| require.Equal(t, constants.AliceAccAddress.String(), aliceStats.User) | |
| require.Equal( | |
| t, | |
| uint64(1_000_000_000), | |
| aliceStats.Stats.Affiliate_30DReferredVolumeQuoteQuantums, | |
| "expected referred volume to be migrated into epoch stats for Alice", | |
| ) | |
| // If the migration is supposed to keep epoch notionals at zero for non-trading users, | |
| // add these checks (otherwise adjust to the intended behavior): | |
| // require.Equal(t, uint64(0), aliceStats.Stats.TakerNotional) | |
| // require.Equal(t, uint64(0), aliceStats.Stats.MakerNotional) | |
| } |
🤖 Prompt for AI Agents
In protocol/app/upgrades/v9.5/upgrade_container_test.go around lines 90–125, the
test TestMigrate30dReferredVolumeToEpochStats_EmptyEpochStats is missing the
closing brace and never asserts on the declared aliceStats; fix it by closing
the function and adding assertions that the single epoch stat entry belongs to
Alice (compare account/address), that Alice’s referred-volume field in the epoch
stat equals the 1_000_000_000 value set on her global UserStats, and assert
expected taker/maker notionals (e.g., zero) to fully validate the migration
behavior for a non‑trading referrer. Ensure you use
constants.AliceAccAddress.String() for the identity check and then close the
function with a trailing }.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
protocol/app/upgrades/v9.5/upgrade_container_test.go (1)
164-165: Critical: Missing closing brace and assertions forTestMigrate30dReferredVolumeToEpochStats_EmptyEpochStats.The function at lines 129-164 is incomplete. Line 164 declares
aliceStatsbut never uses it, and there's no closing}beforeTestStateUpgradebegins on line 165. This will not compile.aliceStats := postUpgradeEpochStats.Stats[0] + require.Equal(t, constants.AliceAccAddress.String(), aliceStats.User) + require.Equal(t, uint64(1_000_000_000), aliceStats.Stats.Affiliate_30DReferredVolumeQuoteQuantums, + "Alice's referred volume should be migrated") + require.Equal(t, uint64(0), aliceStats.Stats.TakerNotional, + "Alice's taker notional should be 0 (epoch stats were empty)") + require.Equal(t, uint64(0), aliceStats.Stats.MakerNotional, + "Alice's maker notional should be 0 (epoch stats were empty)") +} + func TestStateUpgrade(t *testing.T) {
🧹 Nitpick comments (1)
protocol/app/upgrades/v9.5/upgrade_container_test.go (1)
183-192: Empty helper functions leave container test without assertions.The helper functions have comments indicating intent but no implementation. For a meaningful container test of the v9.5 upgrade, these should:
preUpgradeSetups: Set up user stats with referred volume via gRPC/transactionspreUpgradeChecks: Verify pre-upgrade state (epoch stats without referred volume)postUpgradeChecks: Query epoch stats and verify the migration populated referred volume correctlyWould you like me to help implement these helper functions based on the migration logic in
Migrate30dReferredVolumeToEpochStats?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/app/upgrades/v9.5/upgrade_container_test.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_container_test.go (4)
protocol/testutil/constants/addresses.go (4)
AliceAccAddress(10-10)BobAccAddress(11-11)CarlAccAddress(12-12)DaveAccAddress(13-13)protocol/app/upgrades/v9.5/upgrade.go (1)
Migrate30dReferredVolumeToEpochStats(19-100)protocol/testing/containertest/testnet_utils.go (1)
UpgradeTestnet(24-66)protocol/testing/containertest/node.go (1)
Node(35-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). (25)
- GitHub Check: build
- GitHub Check: golangci-lint
- GitHub Check: liveness-test
- GitHub Check: test-coverage-upload
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: benchmark
- GitHub Check: container-tests
- 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-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: (Mainnet) 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-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: (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: (Public Testnet) 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-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-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: build-and-push-testnet
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (2)
protocol/app/upgrades/v9.5/upgrade_container_test.go (2)
16-127: LGTM!This test is comprehensive and well-structured. It properly validates:
- Migration of referred volume for existing epoch stats users (Alice, Bob)
- Preservation of existing taker/maker notional values
- Addition of users with referred volume but no prior epoch stats (Carl)
- Exclusion of users with zero referred volume (Dave)
165-181: Container test scaffolding is in place but lacks assertions.The
TestStateUpgradefunction correctly orchestrates the upgrade flow usingcontainertest.UpgradeTestnet, but the helper functions (preUpgradeSetups,preUpgradeChecks,postUpgradeChecks) are empty. This means the test will pass but won't verify any upgrade behavior.Is this intentional as a placeholder for future implementation, or should assertions be added for the referred volume migration?
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/gogo/protobuf/proto" |
There was a problem hiding this comment.
Remove unused import proto.
The proto package is imported but never used in this file, which will cause a compilation error.
- "github.com/gogo/protobuf/proto"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "github.com/gogo/protobuf/proto" |
🤖 Prompt for AI Agents
In protocol/app/upgrades/v9.5/upgrade_container_test.go around line 6, the
import "github.com/gogo/protobuf/proto" is unused and causes a compilation
error; remove that import from the file's import block (or if it was intended
for side-effects, replace with the appropriate usage or a blank identifier
import), then run go vet/build to confirm the unused import is gone.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
protocol/app/upgrades/v9.5/upgrade_container_test.go (2)
15-125: Well‑scoped coverage of migration behavior; consider a couple of small refinementsThis test does a good job of exercising the main migration paths: existing epoch stats (Alice/Bob), a non‑trading user with referred volume (Carl), and a user with zero referred volume (Dave). It also correctly asserts preservation of taker/maker notionals for users that already have epoch stats, and zero notionals for newly created epoch stats.
Two small, optional refinements you might consider:
- Clarify the inline comments for the
referredVolumevalues intestUsers(Lines 57–60): the literals (e.g.,1_000_000_000) don’t really correspond to “1k volume” / “5k volume” in human units, so the comments could just say “sample volume” or reflect the actual scale.- If you care about the post‑migration state of global
UserStats, you could add an assertion (or a follow‑up test) that documents whetherAffiliate_30DReferredVolumeQuoteQuantumsis expected to remain unchanged or be cleared after migration; right now the test is intentionally silent on that contract.These are non‑blocking; the current test meaningfully guards the migration logic.
173-200:TestStateUpgradecurrently only checks upgrade success; consider fleshing out pre/post checksRight now
TestStateUpgradepluspreUpgradeSetups/preUpgradeChecks/postUpgradeCheckseffectively assert only that the v9.5 upgrade proposal can be submitted, executed, and that the node stays healthy. That’s still useful as a smoke test, but it doesn’t validate the referred‑volume migration in a full containerized network.If you want this to be a stronger regression test for v9.5 later, consider:
- In
preUpgradeSetups, seeding on‑chain state so some users have non‑zero 30d referred volume (and some don’t).- In
preUpgradeChecks, asserting pre‑upgrade epoch stats don’t yet contain that referred volume.- In
postUpgradeChecks, querying epoch stats via RPC and asserting that referred volume has been migrated as in the in‑process tests (including behavior for non‑trading users and zero‑volume users).This can be deferred; the current skeleton is acceptable as a basic upgrade flow test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/app/upgrades/v9.5/upgrade_container_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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: 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/app/upgrades/v9.5/upgrade_container_test.go
🧬 Code graph analysis (1)
protocol/app/upgrades/v9.5/upgrade_container_test.go (3)
protocol/testutil/constants/addresses.go (4)
AliceAccAddress(10-10)BobAccAddress(11-11)CarlAccAddress(12-12)DaveAccAddress(13-13)protocol/app/upgrades/v9.5/upgrade.go (1)
Migrate30dReferredVolumeToEpochStats(19-100)protocol/testing/containertest/testnet_utils.go (1)
UpgradeTestnet(24-66)
⏰ 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: build
- 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-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-vulcan / (vulcan) 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-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-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: (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: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: test-coverage-upload
- GitHub Check: test-race
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: liveness-test
- GitHub Check: golangci-lint
- GitHub Check: benchmark
- GitHub Check: container-tests
- GitHub Check: build-and-push-mainnet
- GitHub Check: build-and-push-testnet
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
protocol/app/upgrades/v9.5/upgrade_container_test.go (1)
128-171: Empty‑epoch scenario test looks solid and matches migration contractThis test cleanly captures the “no existing epoch stats” case: it ensures that a user with only global referred volume (Alice) gets an epoch‑level entry created with the migrated referred volume and zero taker/maker notionals. That matches the described behavior of
Migrate30dReferredVolumeToEpochStatsfor non‑trading referrers.No issues from my side here.
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
|
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.