Check Leverage On Order Placement#3141
Conversation
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
WalkthroughLeverage ownership moved from the CLOB keeper into the Subaccounts module; per-subaccount leverage persisted and threaded as custom_imf_ppm through risk/margin APIs, protos/TS types updated, CLOB exposes SubaccountsKeeper, mocks/CLI/GRPC/tests/e2e adapted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Ante as CLOB AnteHandler
participant CLOB as CLOB Keeper
participant SA as Subaccounts Keeper
participant Perp as Perpetuals Keeper
rect rgba(220,240,255,0.35)
Note over User,SA: MsgUpdateLeverage delegation
User->>Ante: MsgUpdateLeverage {SubaccountId, leverageMap}
Ante->>CLOB: GetSubaccountsKeeper()
CLOB-->>Ante: SubaccountsKeeper
Ante->>SA: UpdateLeverage(SubaccountId, leverageMap)
SA->>Perp: GetPerpetualAndMarketPriceAndLiquidityTier(perpId)
Perp-->>SA: {perpetual, liquidityTier}
SA-->>Ante: ok / error
end
sequenceDiagram
autonumber
participant Keeper as Subaccounts Keeper
participant Lib as Subaccounts Lib
participant PerpLib as Perpetuals Lib
rect rgba(235,255,235,0.35)
Note over Keeper,PerpLib: Leverage-aware risk calculation
Keeper->>Keeper: getSettledUpdates(... fetch LeverageMap)
Keeper->>Lib: GetRiskForSettledUpdate(settledUpdate, perpInfos)
Lib->>PerpLib: GetNetCollateralAndMarginRequirements(..., custom_imf_ppm)
PerpLib-->>Lib: Margin & risk
Lib-->>Keeper: Risk
Keeper-->>Keeper: accept/reject updates
end
sequenceDiagram
autonumber
participant Liquid as Liquidations
participant PerpLib as Perpetuals Lib
rect rgba(255,245,220,0.35)
Note over Liquid,PerpLib: Liquidation pricing using default IMF
Liquid->>PerpLib: GetPositionNetNotionalValueAndMarginRequirements(..., custom_imf_ppm=0)
PerpLib-->>Liquid: Risk (default IMR/MMR)
Liquid-->>Liquid: Compute bankruptcy / fillable price
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 (3)
protocol/x/clob/keeper/leverage.go (1)
8-30: Consider removing deprecated delegation wrappers.Since all leverage operations now delegate directly to
SubaccountsKeeper, these wrapper methods inClobKeeperadd unnecessary indirection. The deprecation comments and TODOs suggest they should be unused or migrated.Consider:
- Audit all call sites in the clob module to use
GetSubaccountsKeeper()directly- Remove these wrapper methods to eliminate the technical debt
Example migration:
-k.UpdateLeverage(ctx, subaccountId, leverageMap) +k.GetSubaccountsKeeper().UpdateLeverage(ctx, subaccountId, leverageMap)protocol/x/subaccounts/keeper/subaccount.go (1)
308-385: Verify collateral sufficiency before allocation.The
applyLeverageAwareCollateralAllocationmethod moves collateral from the main USDC balance to perpetual positions based on leverage-scaled IMR. However, there's no explicit check that the subaccount has sufficient USDC balance to satisfy the allocation before modifying the updates.Scenario: If a subaccount opens a leveraged position but has insufficient USDC, the allocation could drive the USDC balance negative (lines 365-367 or 373), which may or may not be caught by downstream collateral checks depending on the overall risk calculation.
Question: Is the collateral sufficiency guaranteed by the risk checks in
internalCanUpdateSubaccountsWithLeverage(line 766), which callsGetRiskForSettledUpdateon the modified settled update? If so, a comment explaining this would be helpful.Consider adding a comment like:
// Note: Collateral sufficiency is validated by downstream risk checks in // internalCanUpdateSubaccountsWithLeverage, which will reject the update if // the allocated collateral exceeds available USDC balance.Or, if validation should occur here, add an explicit check before allocation.
protocol/x/subaccounts/keeper/leverage.go (1)
37-52: Document panic behavior in GetLeverage/SetLeverage. Add doc comments noting these methods intentionally panic on JSON (un)marshal errors to clarify crash-on-corruption behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 8eb3563 and 7d6eac2cb4082d00f169dae293e8ce06348667d1.
📒 Files selected for processing (20)
protocol/daemons/liquidation/client/sub_task_runner.go(1 hunks)protocol/mocks/ClobKeeper.go(1 hunks)protocol/testutil/app/app.go(2 hunks)protocol/testutil/keeper/listing.go(2 hunks)protocol/x/clob/ante/clob.go(1 hunks)protocol/x/clob/keeper/keeper.go(1 hunks)protocol/x/clob/keeper/leverage.go(1 hunks)protocol/x/clob/keeper/leverage_e2e_test.go(1 hunks)protocol/x/clob/keeper/liquidations.go(2 hunks)protocol/x/clob/types/clob_keeper.go(1 hunks)protocol/x/clob/types/expected_keepers.go(1 hunks)protocol/x/perpetuals/lib/lib.go(6 hunks)protocol/x/perpetuals/lib/lib_test.go(4 hunks)protocol/x/subaccounts/keeper/leverage.go(1 hunks)protocol/x/subaccounts/keeper/subaccount.go(8 hunks)protocol/x/subaccounts/lib/updates.go(2 hunks)protocol/x/subaccounts/lib/updates_test.go(2 hunks)protocol/x/subaccounts/types/errors.go(1 hunks)protocol/x/subaccounts/types/keys.go(1 hunks)protocol/x/subaccounts/types/settled_update.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#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/mocks/ClobKeeper.goprotocol/x/subaccounts/lib/updates_test.goprotocol/x/clob/types/clob_keeper.goprotocol/daemons/liquidation/client/sub_task_runner.goprotocol/x/subaccounts/keeper/subaccount.goprotocol/testutil/keeper/listing.goprotocol/x/subaccounts/keeper/leverage.goprotocol/x/clob/keeper/keeper.go
📚 Learning: 2024-11-15T15:59:28.095Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#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/subaccounts/lib/updates_test.goprotocol/x/clob/types/clob_keeper.goprotocol/daemons/liquidation/client/sub_task_runner.goprotocol/testutil/keeper/listing.goprotocol/x/clob/keeper/keeper.go
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.
Applied to files:
protocol/x/clob/keeper/leverage.goprotocol/x/perpetuals/lib/lib.goprotocol/x/clob/keeper/leverage_e2e_test.goprotocol/x/clob/types/expected_keepers.goprotocol/testutil/app/app.goprotocol/x/subaccounts/keeper/subaccount.goprotocol/x/clob/ante/clob.goprotocol/x/subaccounts/keeper/leverage.go
🧬 Code graph analysis (14)
protocol/mocks/ClobKeeper.go (3)
protocol/x/clob/types/clob_keeper.go (1)
ClobKeeper(13-165)protocol/x/subaccounts/types/types.go (1)
SubaccountsKeeper(11-80)protocol/mocks/SubaccountsKeeper.go (1)
SubaccountsKeeper(19-21)
protocol/x/perpetuals/lib/lib_test.go (1)
protocol/x/perpetuals/lib/lib.go (1)
GetNetCollateralAndMarginRequirements(71-90)
protocol/x/subaccounts/lib/updates_test.go (1)
protocol/x/subaccounts/lib/updates.go (1)
GetRiskForSubaccount(343-388)
protocol/x/subaccounts/types/errors.go (1)
protocol/x/subaccounts/types/keys.go (1)
ModuleName(6-6)
protocol/x/clob/types/clob_keeper.go (2)
protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/subaccounts/types/types.go (1)
SubaccountsKeeper(11-80)
protocol/x/clob/keeper/leverage.go (1)
protocol/x/clob/keeper/keeper.go (1)
Keeper(29-71)
protocol/x/clob/keeper/leverage_e2e_test.go (9)
protocol/testutil/app/app.go (2)
TestApp(600-622)MustMakeCheckTxsWithClobMsg(1352-1393)protocol/x/clob/types/clob_keeper.go (1)
ClobKeeper(13-165)protocol/x/assets/types/genesis.go (1)
AssetUsdc(12-19)protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/subaccounts/types/update.go (3)
Update(79-86)AssetUpdate(88-93)PerpetualUpdate(102-110)protocol/x/clob/types/order_id.go (1)
OrderIdFlags_LongTerm(15-15)protocol/x/clob/types/order.pb.go (1)
Order_SIDE_BUY(38-38)protocol/x/clob/types/orderbook.go (1)
Subticks(11-11)protocol/x/clob/types/message_place_order.go (1)
NewMsgPlaceOrder(33-37)
protocol/testutil/app/app.go (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts (2)
MsgUpdateLeverage(408-414)MsgUpdateLeverage(1450-1496)protocol/x/clob/types/tx.pb.go (3)
MsgUpdateLeverage(1151-1156)MsgUpdateLeverage(1160-1160)MsgUpdateLeverage(1161-1163)
protocol/x/subaccounts/keeper/subaccount.go (4)
protocol/x/subaccounts/types/settled_update.go (1)
SettledUpdate(7-17)protocol/x/subaccounts/lib/updates.go (4)
GetSettledSubaccountWithPerpetuals(20-82)CalculateLeverageAwareCollateralRequirement(408-428)GetRiskForSettledUpdate(393-402)GetRiskForSubaccount(343-388)protocol/x/subaccounts/types/update.go (2)
AssetUpdate(88-93)PerpetualUpdate(102-110)protocol/x/assets/types/genesis.go (1)
AssetUsdc(12-19)
protocol/x/clob/ante/clob.go (1)
protocol/x/clob/types/leverage.go (1)
ValidateAndConstructPerpetualLeverageMap(58-75)
protocol/x/subaccounts/lib/updates.go (5)
protocol/x/perpetuals/lib/lib.go (2)
GetNetCollateralAndMarginRequirements(71-90)GetMarginRequirementsInQuoteQuantums(117-181)protocol/x/subaccounts/types/settled_update.go (1)
SettledUpdate(7-17)protocol/x/perpetuals/types/perpinfo.go (2)
PerpInfos(16-16)PerpInfo(9-13)protocol/lib/margin/risk.go (1)
Risk(9-13)protocol/x/subaccounts/types/update.go (1)
PerpetualUpdate(102-110)
protocol/x/clob/keeper/liquidations.go (1)
protocol/x/perpetuals/lib/lib.go (1)
GetPositionNetNotionalValueAndMarginRequirements(41-67)
protocol/x/subaccounts/keeper/leverage.go (3)
protocol/x/clob/keeper/keeper.go (1)
Keeper(29-71)protocol/x/subaccounts/types/keys.go (1)
LeverageKeyPrefix(30-30)protocol/x/subaccounts/types/errors.go (3)
ErrInvalidLeverage(83-83)ErrLeverageExceedsMaximum(84-84)ErrInitialMarginPpmIsZero(85-85)
protocol/x/clob/keeper/keeper.go (2)
protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/subaccounts/types/types.go (1)
SubaccountsKeeper(11-80)
⏰ 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). (29)
- GitHub Check: liveness-test
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-coverage-upload
- GitHub Check: test-race
- 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-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-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-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-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: check-sample-pregenesis-up-to-date
- GitHub Check: exchange-tests
- GitHub Check: benchmark
- GitHub Check: build
- GitHub Check: build-and-push-testnet
- GitHub Check: golangci-lint
- GitHub Check: container-tests
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (33)
protocol/daemons/liquidation/client/sub_task_runner.go (1)
341-345: LGTM!Correctly passes
nilfor the leverage parameter in liquidation calculations. The comment clearly explains the rationale: leverage only affects initial margin requirements (IMR) during order placement, while liquidation checks use maintenance margin requirements (MMR) which remain unchanged by leverage.protocol/x/clob/types/clob_keeper.go (1)
164-164: LGTM!Clean interface addition that exposes the SubaccountsKeeper to enable delegation of leverage-related operations. The method signature follows existing conventions.
protocol/x/clob/keeper/keeper.go (1)
167-169: LGTM!Straightforward implementation of the GetSubaccountsKeeper accessor declared in the ClobKeeper interface.
protocol/x/subaccounts/types/keys.go (1)
29-30: LGTM!Clean addition of a store key prefix constant for leverage data storage. The constant naming and placement follow existing conventions in the file.
protocol/testutil/keeper/listing.go (1)
144-177: LGTM!Clean refactoring that reflects the architectural shift where leverage is now managed directly by the subaccounts keeper rather than a separate leverage keeper. The initialization order change (creating subaccounts keeper before clob keeper) is appropriate, and the comments clearly document the reasoning.
protocol/x/clob/ante/clob.go (1)
196-216: LGTM!Clean delegation pattern where the clob ante handler validates the UpdateLeverage message and constructs the perpetual leverage map, then delegates the actual storage and validation to the SubaccountsKeeper. This improves separation of concerns while maintaining consistent error handling.
protocol/x/perpetuals/lib/lib_test.go (5)
207-213: LGTM!Tests correctly updated to pass
0for the new leverage parameter, indicating default margin requirements should be used. This is appropriate for tests that aren't specifically testing leverage functionality.
214-221: LGTM!Consistent with the pattern above - passing
0for leverage to use default margin requirements.
314-321: LGTM!Benchmark updated to pass
0for the leverage parameter, maintaining default behavior.
400-409: LGTM!Test correctly updated with the
0leverage argument.
683-689: LGTM!Final test case updated consistently with the new function signature.
protocol/x/subaccounts/types/settled_update.go (1)
14-16: LGTM!Clean addition of the LeverageMap field to carry per-perpetual leverage configuration through the settled update flow. The documentation clearly explains the nil semantics (use default margin requirements when not configured).
protocol/x/clob/keeper/liquidations.go (2)
454-467: LGTM: Correct leverage handling for liquidations.Passing
0for leverage in liquidation calculations is correct, as liquidations should always use the default margin requirements from the liquidity tier rather than user-configured leverage. The inline comments clearly document this intent.
546-552: LGTM: Consistent leverage handling in fillable price calculation.The change correctly passes
0for leverage in the fillable price calculation, maintaining consistency with the bankruptcy price calculation and ensuring liquidations use default margin requirements.protocol/x/subaccounts/lib/updates_test.go (2)
163-163: LGTM: Correct adaptation to new function signature.The test correctly passes
nilfor the newleverageMapparameter, which means "use default margin requirements". This is appropriate for tests that are not specifically testing leverage functionality.
186-186: LGTM: Consistent test adaptation.The panic test case correctly includes the
nilleverage parameter, maintaining consistency with the other test cases.protocol/mocks/ClobKeeper.go (1)
509-527: LGTM: Standard mock implementation.The
GetSubaccountsKeepermock method follows the established mockery pattern used throughout this file, with proper return value validation and type handling.protocol/testutil/app/app.go (1)
1352-1377: LGTM: Consistent extension for new message type.The addition of
clobtypes.MsgUpdateLeveragesupport follows the established pattern used for other CLOB message types, with correct signer address extraction and message handling.protocol/x/subaccounts/types/errors.go (1)
82-85: LGTM: Well-defined leverage errors.The new leverage-related error variables follow the established patterns in this file, with appropriate error codes (700-799 range) and clear, concise error messages.
protocol/x/clob/keeper/leverage_e2e_test.go (2)
194-202: Verify leverage=1 configuration is intentional.The test configures Alice with
Leverage: 1(1x leverage), which represents the maximum restriction on leverage. With this configuration, Alice should indeed have more stringent collateral requirements than Bob (who uses default margin requirements).Please confirm this is the intended test behavior. If testing maximum restriction is the goal, consider adding a comment explaining that leverage=1 means 1x (no leverage), to make the test's intent clearer.
67-123: LGTM: Comprehensive keeper wiring test.The test effectively verifies that the SubaccountsKeeper leverage subsystem is properly wired by:
- Configuring leverage
- Verifying storage
- Testing leverage-aware operations (
CanUpdateSubaccounts)This provides good coverage of the integration points.
protocol/x/subaccounts/lib/updates.go (3)
343-388: LGTM: Robust leverage integration in risk calculation.The updated
GetRiskForSubaccountfunction correctly:
- Accepts an optional
leverageMapparameter- Defaults to
leverage = 0(use default margins) when not configured- Passes per-perpetual leverage to margin calculations
The nil-check on
leverageMap(line 372) prevents panics and ensures safe operation when leverage is not configured.
390-402: LGTM: Convenient helper for settled update risk calculation.The
GetRiskForSettledUpdatehelper appropriately:
- Calculates the updated subaccount state
- Extracts leverage from the
SettledUpdate- Delegates to
GetRiskForSubaccountThis provides a clean abstraction for the common pattern of calculating risk for settled updates with embedded leverage data.
404-428: LGTM: Clear collateral requirement calculation.The
CalculateLeverageAwareCollateralRequirementhelper correctly:
- Returns zero when leverage is not configured or position size is zero
- Uses absolute value for margin calculation (line 423)
- Calculates IMR based on the configured leverage
The early returns (lines 414-416) optimize for common cases and improve readability.
protocol/x/perpetuals/lib/lib.go (2)
41-67: LGTM!The leverage parameter is cleanly threaded through to the margin calculation functions.
71-90: LGTM!The comment clarifies the default behavior when leverage is 0.
protocol/x/clob/types/expected_keepers.go (1)
108-128: LGTM!The leverage management methods are cleanly added to the SubaccountsKeeper interface. The method signatures align with the implementations in
protocol/x/subaccounts/keeper/leverage.go.protocol/x/subaccounts/keeper/leverage.go (3)
54-97: LGTM!The validation logic correctly checks leverage against the maximum allowed value before merging and storing. The error wrapping provides clear context for debugging.
99-116: Good: Returns error instead of panicking.Unlike the panic in
protocol/x/perpetuals/lib/lib.goline 166, this function correctly returns an error whenInitialMarginPpm == 0. This is the safer approach for handling invalid configurations.
24-34: Keep panic here — consistent with repository conventionsThe codebase intentionally panics on marshal/unmarshal/invariant failures (see protocol/x/subaccounts/keeper/leverage.go and many other keeper/test files); returning an error would be inconsistent with existing patterns.
Likely an incorrect or invalid review comment.
protocol/x/subaccounts/keeper/subaccount.go (3)
253-302: Leverage caching and propagation looks correct.The implementation efficiently caches leverage data per subaccount (line 254, 279) and propagates it through
SettledUpdate.LeverageMap(line 290). The conditional application of leverage-aware collateral allocation (lines 294-296) ensures it only runs when leverage is configured.
468-473: LGTM: Leverage-aware validation path integrated.The changes correctly route both
UpdateSubaccountsandCanUpdateSubaccountsthrough the leverage-aware validation path (internalCanUpdateSubaccountsWithLeverage). The settled updates already contain the leverage data (viaLeverageMap), which is propagated to risk calculations.Also applies to: 616-617
765-789: Leverage correctly propagated to risk calculations.The leverage map is correctly fetched and passed to risk calculation functions (
GetRiskForSettledUpdateat line 766 andGetRiskForSubaccountat lines 785-788, 851-854). This ensures that margin requirements are computed with leverage awareness.Also applies to: 845-856
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
force-pushed
the
anmol/leverage-collat
branch
from
06d2c3f to
74f5e4f
Compare
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
1-1: Critical: Regenerate protobuf code to fix pipeline failures.The pipeline failures indicate that
pnpm run build:verify-protois detecting diffs between the generated code and what's committed. Since this is an auto-generated file, it should be regenerated from the proto definitions rather than manually edited.Run the following command to regenerate the protobuf code:
#!/bin/bash # Verify the proto generation command in the indexer package cd indexer/packages/v4-protos cat package.json | jq -r '.scripts | to_entries[] | select(.key | contains("proto") or contains("generate") or contains("codegen")) | "\(.key): \(.value)"'
♻️ Duplicate comments (6)
protocol/x/perpetuals/lib/lib.go (1)
116-126: Update the function comment to reflect imf_ppm semantics.The comment on line 116 still references the old "leverage" and "maxLeverage/userLeverage ratio" approach, but the implementation now uses
custom_imf_ppmto override initial margin factors.Apply this diff:
-// GetMarginRequirementsInQuoteQuantums returns initial and maintenance margin requirements -// in quote quantums, given the position size in base quantums. -// If leverage > 0, scales the margin requirements based on maxLeverage/userLeverage ratio. +// GetMarginRequirementsInQuoteQuantums returns initial and maintenance margin requirements +// in quote quantums, given the position size in base quantums. +// If custom_imf_ppm > 0, the initial margin requirement uses the custom IMF (in parts per million) +// to override the default liquidity tier margin. MMR is always derived from the base IMR.indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts (1)
390-402: Same protobuf regeneration issue affects this file.The
LeverageEntryfield rename fromleveragetoimfPpmis correct, but the pipeline failures indicate the generated code needs to be regenerated and committed.See the comment on
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.tsfor the resolution steps.protocol/x/clob/client/cli/tx_update_leverage.go (1)
42-48: Deterministic order added — prefer numeric sort of IDsGreat fix for determinism. Since keys are numeric IDs encoded as strings, consider numeric sort to avoid “10” < “2”.
- // Sort the keys to ensure deterministic ordering - keys := make([]string, 0, len(leverageMap)) - for clobPairIdStr := range leverageMap { - keys = append(keys, clobPairIdStr) - } - sort.Strings(keys) + // Sort by numeric clob pair ID for deterministic, intuitive ordering. + ids := make([]uint32, 0, len(leverageMap)) + for clobPairIdStr := range leverageMap { + id, err := strconv.ParseUint(clobPairIdStr, 10, 32) + if err != nil { + return fmt.Errorf("invalid clob pair ID %s: %w", clobPairIdStr, err) + } + ids = append(ids, uint32(id)) + } + sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] }) @@ - for _, clobPairIdStr := range keys { - clobPairId, err := strconv.ParseUint(clobPairIdStr, 10, 32) - if err != nil { - return fmt.Errorf("invalid clob pair ID %s: %w", clobPairIdStr, err) - } - - clobPairLeverage = append(clobPairLeverage, &types.LeverageEntry{ - ClobPairId: uint32(clobPairId), - CustomImfPpm: leverageMap[clobPairIdStr], - }) - } + for _, clobPairId := range ids { + clobPairLeverage = append(clobPairLeverage, &types.LeverageEntry{ + ClobPairId: clobPairId, + CustomImfPpm: leverageMap[strconv.FormatUint(uint64(clobPairId), 10)], + }) + }Also applies to: 50-59
protocol/x/clob/keeper/grpc_query_leverage.go (1)
35-43: Deterministic, numeric ordering and field rename handled correctlySorting by uint32 perpetualId and emitting CustomImfPpm with ClobPairId resolution is correct. Error wrapping on pair lookup is appropriate.
Also applies to: 44-56
protocol/x/clob/keeper/leverage_e2e_test.go (2)
143-146: Fix leverage logging: printing ppm as multiplier.Line 143 logs
custom_imf_ppm(100,000) as if it's the leverage multiplier, but 10x leverage = 1,000,000 / 100,000. The log will incorrectly show "100000x leverage" instead of "10x leverage".Apply this diff to compute and log the actual leverage multiplier:
- t.Logf("✅ Successfully configured and verified %dx leverage for subaccount", custom_imf_ppm) + leverageX := float64(1_000_000) / float64(custom_imf_ppm) + t.Logf("✅ Successfully configured and verified %.1fx leverage (imf_ppm=%d) for subaccount", leverageX, custom_imf_ppm) t.Logf(" Subaccount: %v", subaccountId) t.Logf(" Perpetual ID: %d", perpetualId) t.Logf(" Initial balance: $%s", new(big.Int).Div(initialBalance, big.NewInt(1_000_000)))
175-175: Fix leverage logging: printing ppm as multiplier.Line 175 has the same issue as TestLeverageBasicOrderPlacement - it logs the ppm value instead of computing the actual leverage multiplier.
Apply this diff:
- t.Logf("✅ Successfully configured %dx leverage", tc.custom_imf_ppm) + leverageX := float64(1_000_000) / float64(tc.custom_imf_ppm) + t.Logf("✅ Successfully configured %.1fx leverage (imf_ppm=%d)", leverageX, tc.custom_imf_ppm)
🧹 Nitpick comments (3)
protocol/x/subaccounts/types/settled_update.go (1)
14-16: Consider clarifying the comment for precision.The comment "perpetualId -> custom imf" could be more precise. Consider updating it to "perpetualId -> custom_imf_ppm" to match the field naming used elsewhere (e.g.,
custom_imf_ppmparameter inlib.go).Apply this diff:
- // Leverage configuration for this subaccount (perpetualId -> custom imf). + // Leverage configuration for this subaccount (perpetualId -> custom_imf_ppm in parts per million).protocol/x/subaccounts/lib/updates.go (1)
370-375: Optionally distinguish “unset” vs “explicit 0” IMFIf you ever need to treat an explicit 0 override differently from absence, guard with ok. Current behavior is fine if both mean “use default IMF.”
- custom_imf_ppm := uint32(0) - if leverageMap != nil { - custom_imf_ppm = leverageMap[pos.PerpetualId] - } + custom_imf_ppm := uint32(0) + if leverageMap != nil { + if v, ok := leverageMap[pos.PerpetualId]; ok { + custom_imf_ppm = v + } + }protocol/x/subaccounts/keeper/leverage.go (1)
69-69: Optional: Use Go naming conventions (camelCase over snake_case).Go style prefers
camelCasefor local variables. Consider renamingcustom_imf_ppmtocustomImfPpmthroughout this function for consistency with Go conventions.Also applies to: 80-80, 84-84, 98-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 06d2c3f91e36cba225e32769455c9f0f4f7bb516 and 74f5e4f.
⛔ Files ignored due to path filters (3)
protocol/x/clob/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/clob/types/tx.pb.gois excluded by!**/*.pb.goprotocol/x/subaccounts/types/leverage.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (36)
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts(4 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts(5 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts(5 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/leverage.ts(1 hunks)indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts(1 hunks)indexer/packages/v4-protos/src/codegen/google/bundle.ts(1 hunks)proto/dydxprotocol/clob/query.proto(1 hunks)proto/dydxprotocol/clob/tx.proto(1 hunks)proto/dydxprotocol/subaccounts/leverage.proto(1 hunks)protocol/daemons/liquidation/client/sub_task_runner.go(1 hunks)protocol/mocks/ClobKeeper.go(4 hunks)protocol/testutil/app/app.go(2 hunks)protocol/testutil/keeper/listing.go(2 hunks)protocol/x/clob/ante/clob.go(1 hunks)protocol/x/clob/client/cli/tx_update_leverage.go(2 hunks)protocol/x/clob/e2e/app_test.go(1 hunks)protocol/x/clob/keeper/grpc_query_leverage.go(2 hunks)protocol/x/clob/keeper/keeper.go(1 hunks)protocol/x/clob/keeper/leverage.go(1 hunks)protocol/x/clob/keeper/leverage_e2e_test.go(1 hunks)protocol/x/clob/keeper/liquidations.go(2 hunks)protocol/x/clob/types/clob_keeper.go(1 hunks)protocol/x/clob/types/expected_keepers.go(1 hunks)protocol/x/clob/types/leverage.go(2 hunks)protocol/x/perpetuals/lib/lib.go(7 hunks)protocol/x/perpetuals/lib/lib_test.go(4 hunks)protocol/x/perpetuals/types/liquidity_tier.go(1 hunks)protocol/x/perpetuals/types/liquidity_tier_test.go(2 hunks)protocol/x/subaccounts/keeper/leverage.go(1 hunks)protocol/x/subaccounts/keeper/margining.go(2 hunks)protocol/x/subaccounts/keeper/subaccount.go(7 hunks)protocol/x/subaccounts/lib/updates.go(2 hunks)protocol/x/subaccounts/lib/updates_test.go(2 hunks)protocol/x/subaccounts/types/errors.go(1 hunks)protocol/x/subaccounts/types/keys.go(1 hunks)protocol/x/subaccounts/types/settled_update.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- protocol/x/subaccounts/types/keys.go
- proto/dydxprotocol/clob/tx.proto
- protocol/testutil/app/app.go
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts
- proto/dydxprotocol/subaccounts/leverage.proto
- protocol/x/perpetuals/lib/lib_test.go
- protocol/x/subaccounts/lib/updates_test.go
- protocol/testutil/keeper/listing.go
- protocol/x/perpetuals/types/liquidity_tier_test.go
- proto/dydxprotocol/clob/query.proto
- protocol/x/clob/ante/clob.go
- protocol/x/subaccounts/keeper/margining.go
- protocol/x/subaccounts/types/errors.go
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.
Applied to files:
protocol/x/clob/keeper/grpc_query_leverage.goprotocol/x/clob/keeper/leverage.goprotocol/x/clob/client/cli/tx_update_leverage.goprotocol/x/perpetuals/lib/lib.goprotocol/x/clob/e2e/app_test.goprotocol/x/clob/types/expected_keepers.goprotocol/mocks/ClobKeeper.goprotocol/x/clob/types/leverage.goprotocol/x/subaccounts/keeper/leverage.goprotocol/x/clob/keeper/leverage_e2e_test.goprotocol/x/subaccounts/keeper/subaccount.go
📚 Learning: 2024-11-15T16:17:29.092Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#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/clob/keeper/liquidations.goprotocol/x/clob/types/expected_keepers.go
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#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/clob/types/expected_keepers.goprotocol/x/clob/types/clob_keeper.goprotocol/x/clob/keeper/keeper.goprotocol/x/subaccounts/keeper/subaccount.go
📚 Learning: 2024-11-15T15:59:28.095Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#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/clob/keeper/keeper.go
🧬 Code graph analysis (16)
protocol/x/subaccounts/lib/updates.go (5)
protocol/x/perpetuals/lib/lib.go (1)
GetNetCollateralAndMarginRequirements(71-90)protocol/x/assets/lib/lib.go (1)
GetNetCollateralAndMarginRequirements(12-41)protocol/x/subaccounts/types/settled_update.go (1)
SettledUpdate(7-17)protocol/x/perpetuals/types/perpinfo.go (1)
PerpInfos(16-16)protocol/lib/margin/risk.go (1)
Risk(9-13)
protocol/x/clob/keeper/grpc_query_leverage.go (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (4)
ClobPairLeverageInfo(299-305)ClobPairLeverageInfo(1466-1512)QueryLeverageResponse(287-290)QueryLeverageResponse(1420-1457)protocol/x/clob/types/query.pb.go (6)
ClobPairLeverageInfo(954-959)ClobPairLeverageInfo(963-963)ClobPairLeverageInfo(964-966)QueryLeverageResponse(908-911)QueryLeverageResponse(915-915)QueryLeverageResponse(916-918)
protocol/x/clob/keeper/liquidations.go (1)
protocol/x/perpetuals/lib/lib.go (1)
GetPositionNetNotionalValueAndMarginRequirements(41-67)
indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/leverage.ts (1)
protocol/x/subaccounts/types/leverage.pb.go (6)
PerpetualLeverageEntry(28-33)PerpetualLeverageEntry(37-37)PerpetualLeverageEntry(38-40)LeverageData(83-86)LeverageData(90-90)LeverageData(91-93)
protocol/x/clob/client/cli/tx_update_leverage.go (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts (2)
LeverageEntry(387-393)LeverageEntry(1395-1441)protocol/x/clob/types/tx.pb.go (3)
LeverageEntry(1095-1100)LeverageEntry(1104-1104)LeverageEntry(1105-1107)
protocol/x/perpetuals/types/liquidity_tier.go (1)
protocol/lib/big_math.go (2)
BigMax(105-115)BigMulPpm(20-28)
protocol/x/perpetuals/lib/lib.go (1)
protocol/lib/big_math.go (1)
BigU(10-12)
protocol/x/clob/e2e/app_test.go (3)
protocol/x/clob/types/orderbook.go (1)
ClobPairId(29-29)indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts (4)
MsgUpdateLeverage(408-414)MsgUpdateLeverage(1450-1496)LeverageEntry(387-393)LeverageEntry(1395-1441)protocol/x/clob/types/tx.pb.go (6)
MsgUpdateLeverage(1151-1156)MsgUpdateLeverage(1160-1160)MsgUpdateLeverage(1161-1163)LeverageEntry(1095-1100)LeverageEntry(1104-1104)LeverageEntry(1105-1107)
protocol/mocks/ClobKeeper.go (2)
protocol/x/clob/types/clob_keeper.go (1)
ClobKeeper(13-163)protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)
protocol/x/clob/types/leverage.go (1)
protocol/x/clob/types/errors.go (1)
ErrInvalidLeverage(564-568)
protocol/x/clob/types/clob_keeper.go (2)
protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/subaccounts/types/types.go (1)
SubaccountsKeeper(11-80)
protocol/x/subaccounts/keeper/leverage.go (4)
protocol/x/clob/keeper/keeper.go (1)
Keeper(29-71)protocol/x/subaccounts/types/keys.go (1)
LeverageKeyPrefix(30-30)protocol/x/subaccounts/types/leverage.pb.go (6)
PerpetualLeverageEntry(28-33)PerpetualLeverageEntry(37-37)PerpetualLeverageEntry(38-40)LeverageData(83-86)LeverageData(90-90)LeverageData(91-93)protocol/x/subaccounts/types/errors.go (3)
ErrInvalidLeverage(83-83)ErrLeverageExceedsMaximum(84-84)ErrInitialMarginPpmIsZero(85-85)
protocol/x/clob/keeper/leverage_e2e_test.go (10)
protocol/testutil/app/app.go (3)
TestApp(600-622)NewTestAppBuilder(335-350)MustMakeCheckTxsWithClobMsg(1352-1393)protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/assets/types/genesis.go (1)
AssetUsdc(12-19)protocol/dtypes/serializable_int.go (1)
NewIntFromBigInt(43-49)protocol/x/subaccounts/types/update.go (4)
Update(79-86)AssetUpdate(88-93)PerpetualUpdate(102-110)CollatCheck(133-133)protocol/x/clob/types/tx.pb.go (6)
MsgUpdateLeverage(1151-1156)MsgUpdateLeverage(1160-1160)MsgUpdateLeverage(1161-1163)LeverageEntry(1095-1100)LeverageEntry(1104-1104)LeverageEntry(1105-1107)protocol/x/clob/types/order_id.go (1)
OrderIdFlags_LongTerm(15-15)protocol/x/clob/types/order.pb.go (1)
Order_SIDE_BUY(38-38)protocol/x/clob/types/orderbook.go (1)
Subticks(11-11)protocol/x/clob/types/message_place_order.go (1)
NewMsgPlaceOrder(33-37)
protocol/x/clob/keeper/keeper.go (2)
protocol/x/clob/types/expected_keepers.go (1)
SubaccountsKeeper(20-128)protocol/x/subaccounts/types/types.go (1)
SubaccountsKeeper(11-80)
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (1)
vault(31-34)indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (2)
vault(53-57)vault(108-110)
protocol/x/subaccounts/keeper/subaccount.go (2)
protocol/x/subaccounts/types/settled_update.go (1)
SettledUpdate(7-17)protocol/x/subaccounts/lib/updates.go (3)
GetSettledSubaccountWithPerpetuals(20-82)GetRiskForSettledUpdate(393-402)GetRiskForSubaccount(343-388)
🪛 GitHub Actions: Indexer API Documentation Check
indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/leverage.ts
[error] 1-1: ELIFECYCLE: Command failed with exit code 1. pnpm run build:verify-proto failed (ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL).
indexer/packages/v4-protos/src/codegen/google/bundle.ts
[error] 1-1: ELIFECYCLE: Command failed with exit code 1. pnpm run build:verify-proto failed (ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL).
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts
[error] 1-1: ELIFECYCLE: Command failed with exit code 1. pnpm run build:verify-proto failed (ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL).
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts
[error] 1-1: ELIFECYCLE: Command failed with exit code 1. pnpm run build:verify-proto failed (ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL).
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts
[error] 1-1: ELIFECYCLE: Command failed with exit code 1. pnpm run build:verify-proto failed (ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL).
🪛 GitHub Actions: Indexer Build, Test & Coverage
indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/leverage.ts
[error] 1-1: pnpm run build:verify-proto failed. Exit code 1. The build step reported diffs between generated code and committed code (patches applied), causing the recursive build to fail.
indexer/packages/v4-protos/src/codegen/google/bundle.ts
[error] 1-1: pnpm run build:verify-proto failed. Exit code 1. The build step reported diffs between generated code and committed code (patches applied), causing the recursive build to fail.
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/tx.ts
[error] 1-1: pnpm run build:verify-proto failed. Exit code 1. The build step reported diffs between generated code and committed code (patches applied), causing the recursive build to fail.
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts
[error] 1-1: pnpm run build:verify-proto failed. Exit code 1. The build step reported diffs between generated code and committed code (patches applied), causing the recursive build to fail.
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts
[error] 1-1: pnpm run build:verify-proto failed. Exit code 1. The build step reported diffs between generated code and committed code (patches applied), causing the recursive build to fail.
⏰ 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). (37)
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) 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-vulcan / (vulcan) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: check-build-auxo
- 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-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: (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-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-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-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: build-and-push-mainnet
- GitHub Check: build-and-push-testnet
- GitHub Check: benchmark
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: lint
- GitHub Check: golangci-lint
- GitHub Check: exchange-tests
- GitHub Check: test-coverage-upload
- GitHub Check: liveness-test
- GitHub Check: container-tests
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (25)
protocol/x/clob/types/leverage.go (2)
31-36: Validation logic correctly updated for IMF PPM.The validation now checks that
CustomImfPpmis in the range (0, 1,000,000], which correctly represents an Initial Margin Fraction in parts per million (0% to 100%). The error message clearly communicates the valid range.
68-72: Field rename applied correctly in map construction.The perpetual leverage map now correctly uses
entry.CustomImfPpminstead of the oldentry.Leveragefield.protocol/daemons/liquidation/client/sub_task_runner.go (1)
341-345: Correct: Liquidations should use default margin requirements.Passing
nilfor the leverage map ensures that liquidation calculations use standard margin requirements rather than leverage-adjusted ones. This is the correct behavior for determining liquidation eligibility.protocol/x/clob/keeper/liquidations.go (2)
454-467: Default margins correctly applied for bankruptcy price calculation.Both
riskPosOldandriskPosNewcalculations pass0forcustom_imf_ppm, ensuring that bankruptcy prices are computed using standard margin requirements rather than leverage-adjusted ones. This is correct for liquidation mechanics.
546-552: Default margins correctly applied for fillable price calculation.Passing
0forcustom_imf_ppmensures that liquidation fillable prices use standard margin requirements. This is consistent with the bankruptcy price calculation and the liquidation daemon's use ofnilleverage.protocol/x/clob/keeper/keeper.go (1)
167-169: LGTM: Standard accessor for SubaccountsKeeper.This simple getter exposes the
subaccountsKeeperfield, enabling external components to access subaccount-related operations (including the new leverage methods). The pattern is consistent with other keeper accessors.protocol/x/perpetuals/lib/lib.go (3)
41-67: Parameter threading looks correct.The
custom_imf_ppmparameter is properly threaded fromGetPositionNetNotionalValueAndMarginRequirementsthrough toGetMarginRequirementsInQuoteQuantums. The parameter passing is clean and consistent.
71-90: LGTM: custom_imf_ppm parameter added with clear documentation.The parameter is well-documented with the comment "0 means use default liquidity tier margins" and correctly passed through to the position calculation function.
145-164: Implementation correctly delegates IMF override to GetInitialMarginQuoteQuantums.The code:
- Computes base IMR (line 146-150) by passing 0 for both open interest and custom IMF
- Derives MMR from base IMR (line 152-156)
- Computes final IMR (line 158-162) with actual open interest and custom_imf_ppm
This delegates the custom IMF logic to
GetInitialMarginQuoteQuantums, which is cleaner than inline scaling.indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/query.ts (1)
302-314: ****The original review comment incorrectly assumes protobuf code needs regeneration due to pipeline failures. However, verification shows:
- The generated code at lines 302-314 is already correct with the field renames (
imfPpm/imf_ppm)git statusconfirms the working tree is clean—no uncommitted diffs insrc/codegen/- The GitHub workflow (
proto.yml) runsbuild:verify-proto, which regenerates the code and fails if any diffs exist; the absence of pipeline failures indicates the code is already properly committedThe code changes are already present and verified. No regeneration or additional commits are needed.
Likely an incorrect or invalid review comment.
protocol/x/subaccounts/lib/updates.go (2)
390-402: Convenience wrapper LGTMWrapper cleanly threads leverage from SettledUpdate; keeps call sites tidy.
346-347: All call sites properly updated; nil and non-nil paths covered by testsVerification confirms all 6 invocations of
GetRiskForSubaccounthave been updated to pass 3 arguments:
- Updates test calls (2 sites): pass
nil— tested explicitly in unit tests- Keeper call sites (2 sites): pass
u.LeverageMapor result fromGetLeverage— tested via integration tests- Library internal call (1 site): passes
settledUpdate.LeverageMap— covered by flow- Liquidation call (1 site): passes
nil— exercised via integrationNil-path coverage is explicitly tested in
TestGetRiskForSubaccount(lines 160–167). Non-nil path is validated through integration tests inleverage_e2e_test.gothat set and verify actual leverage values, ensuring the nil-check atupdates.go:372–373works correctly for both branches. No gaps detected; signature migration is complete.protocol/x/clob/e2e/app_test.go (1)
319-356: Leverage payloads updated to CustomImfPpm — values/names consistentConstants reflect expected IMF ppm (e.g., Lev5→200_000, Lev10→100_000, Lev4→250_000). Looks good.
Also applies to: 338-346, 347-356
protocol/x/clob/types/clob_keeper.go (1)
162-162: Interface addition properly integrated; mocks and usages confirmed updatedThe
GetSubaccountsKeeper()method has been correctly added to the interface with corresponding updates: the mock inprotocol/mocks/ClobKeeper.gohas been regenerated, the concrete implementation inprotocol/x/clob/keeper/keeper.gois present, and the method is actively used inprotocol/x/clob/ante/clob.go. No missing implementations or stale mocks detected.protocol/x/clob/keeper/leverage.go (1)
14-14: ID-space translation is correctly implemented — review comment resolvedThe code properly converts
ClobPairId→perpetualIdbefore calling the keeper. TheValidateAndConstructPerpetualLeverageMapfunction (protocol/x/clob/types/leverage.go:58–73) iterates through eachLeverageEntry(keyed byClobPairId), retrieves the correspondingClobPairviaclobKeeper.GetClobPair(), extracts theperpetualIdusingclob.MustGetPerpetualId(), and builds amap[uint32]uint32keyed byperpetualId. This map is then passed tokeeper.UpdateLeverage(), which delegates tosubaccountsKeeper.UpdateLeverage().protocol/x/clob/types/expected_keepers.go (1)
109-127: SubaccountsKeeper leverage API implementation verified — signatures match interface.All four methods are implemented in
protocol/x/subaccounts/keeper/leverage.gowith signatures matching the interface declarations:
SetLeverage(line 21)GetLeverage(line 42)UpdateLeverage(line 63)GetMinImfForPerpetual(line 109)Type alias
satypes.SubaccountIdresolves to the sametypes.SubaccountIdused in the implementation. No issues found.protocol/x/perpetuals/types/liquidity_tier.go (1)
76-88: <!-- [scratchpad_continue]
[task verify_review_comment_1/1 nil_dereference_risk CONTINUE][observations] UPDATED
- Go best practice: "It is common in Go that if you are receiving an argument as a pointer, you check to see if it was nil or not before performing any operations" (DigitalOcean)
- Go conventions explicitly recommend nil checks for pointer arguments
- Codebase evidence: all existing callers pass non-nil values (big.NewInt(0) or lib.BigU(uint32))
- Review comment claims parameter is "documented as optional" - need to verify this claim
[analysis] UPDATED
- The nil-dereference risk is TECHNICALLY VALID per Go best practices
- Go explicitly recommends nil guards on pointer parameters
- However, empirically: no callers pass nil in current codebase
- The review comment's characterization of the parameter as "documented as optional" is NOT SUBSTANTIATED
- Parameter has no documentation saying it's optional
- Higher-level API (lib.go) has a uint32 parameter that is documented as "0 means use default"
- But that's a different function/type than GetInitialMarginQuoteQuantums
- GetInitialMarginQuoteQuantums's parameter comment doesn't say it's optional
[pending]
- Is the parameter actually documented as optional? (check parameter documentation)
- Does oiQuoteQuantums get guarded for nil? (check other *big.Int params in method)
[actions]
Search for GetInitialMarginQuoteQuantums parameter documentation and compare treatment of other *big.Int params
[scratchpad_end] --><!-- [scratchpad_final]
[task verify_review_comment_1/1 nil_dereference_risk FINAL][observations] FINAL
- Go stdlib big.Int documentation (pkg.go.dev): "Operations always take pointer arguments (*Int)" - no mention of optional nil handling
- Go best practice consensus: Always check for nil on pointer parameters before dereferencing
- Codebase evidence: ALL callers pass non-nil values (big.NewInt(0) or lib.BigU(uint32))
- The function has NO documentation saying the parameter is optional
- The higher-level API in lib.go documents "0 means use default" on a uint32 parameter, not on *big.Int parameter
[analysis] FINAL
ISSUE: Review comment claims parameter is "documented as optional" - THIS IS FALSE
- GetInitialMarginQuoteQuantums has no documentation saying custom_imf_ppm is optional
- All callers in codebase pass non-nil values
- The parameter type is *big.Int (pointer), so it COULD theoretically be nil
- But there is no evidence anyone intends to pass nil
REALITY:
- The nil dereference is a THEORETICAL risk per Go best practices (always check pointer params)
- But it's not a PRACTICAL risk in this codebase
- No documentation indicates the parameter is optional
- All call sites pass valid values
VERDICT:
- The review comment's core claim (parameter is documented as optional) is INCORRECT
- However, the suggested nil check is defensively sound and not harmful
- The review comment OVERSTATES the severity by treating a defensive suggestion as a bug
RECOMMENDATION:
The nil check suggestion is valid defensive programming but the framing is incorrect.
Should rewrite to:
- NOT claim the parameter is documented as optional (it's not)
- Frame as defensive programming suggestion
- Note that all current callers pass non-nil (so not a breaking bug fix)
[scratchpad_end] -->Add nil check for defensive robustness, not to fix an active bug
Go recommends checking for nil before dereferencing pointer parameters to prevent panics. While all current callers pass non-nil values (
big.NewInt(0)orlib.BigU(custom_imf_ppm)), adding a nil guard is defensive:func (liquidityTier LiquidityTier) GetInitialMarginQuoteQuantums( quoteQuantums *big.Int, oiQuoteQuantums *big.Int, custom_imf_ppm *big.Int, ) *big.Int { totalImfPpm := liquidityTier.GetAdjustedInitialMarginPpm(oiQuoteQuantums) - if custom_imf_ppm.Sign() > 0 { + if custom_imf_ppm != nil && custom_imf_ppm.Sign() > 0 { // use the configured IMF if it is greater than the OI scaled IMF totalImfPpm = lib.BigMax(totalImfPpm, custom_imf_ppm) } return lib.BigMulPpm(quoteQuantums, totalImfPpm, true) // Round up initial margin. }Note: The parameter is not documented as optional in the function signature—this is a defensive hardening suggestion, not fixing an observed defect.
protocol/mocks/ClobKeeper.go (1)
1-1: LGTM: Auto-generated mock updates align with interface changes.The mock correctly adds
GetSubaccountsKeeper()and updates the affiliate types import alias, matching the broader architectural shift to route leverage operations through SubaccountsKeeper.Also applies to: 8-8, 627-645
protocol/x/subaccounts/keeper/leverage.go (2)
41-60: LGTM: Deterministic deserialization.The unmarshal and map reconstruction logic is correct. Reading from storage is deterministic, and the resulting map is only used in-memory for lookups.
107-122: LGTM: Clean IMF retrieval with proper error handling.The function correctly retrieves the minimum IMF from the liquidity tier and validates that
InitialMarginPpmis non-zero before returning.protocol/x/clob/keeper/leverage_e2e_test.go (3)
17-60: LGTM: Clean test helpers.The helper functions provide clear, reusable scaffolding for leverage tests. The setup, configuration, and subaccount creation logic is straightforward and correct.
62-119: LGTM: Comprehensive wiring validation.This test properly verifies that the leverage keeper is integrated into the subaccounts module and that
CanUpdateSubaccountsfunctions correctly with configured leverage.
180-266: LGTM: Correctly validates leverage-constrained order placement.This test properly demonstrates the leverage constraint mechanism - Alice's 1x leverage (IMF = 100% of position value) causes her order to fail the collateral check, while Bob's identical order succeeds with default margin requirements.
protocol/x/subaccounts/keeper/subaccount.go (2)
254-296: LGTM: Correctly propagates leverage for all cases.The leverage fetching logic (lines 273-279, 287-289) now correctly handles both:
- Updates that modify perpetual positions (
len(u.PerpetualUpdates) > 0)- Asset-only updates on subaccounts with existing perpetual positions (
len(settledSubaccount.PerpetualPositions) > 0)This properly addresses the past review concern about leverage being ignored for asset-only updates on subaccounts with perpetual exposure.
386-386: LGTM: Complete leverage integration into risk calculations.The changes properly thread leverage configuration through all risk validation paths:
getSettledUpdatesattachesLeverageMapto eachSettledUpdateinternalCanUpdateSubaccountsWithLeverageuses leverage from settled updatesGetRiskForSettledUpdateandGetRiskForSubaccountapply leverage-aware margin requirementsGetNetCollateralAndMarginRequirementsfetches and propagates leverage for speculative risk queriesThe architecture cleanly separates leverage retrieval (in
getSettledUpdates) from usage (in risk calculations).Also applies to: 499-535, 538-729, 763-773
| import * as _m0 from "protobufjs/minimal"; | ||
| import { DeepPartial } from "../../helpers"; | ||
| /** PerpetualLeverageEntry represents a single perpetual leverage setting for internal storage */ | ||
|
|
||
| export interface PerpetualLeverageEntry { | ||
| /** The perpetual ID (internal storage format) */ | ||
| perpetualId: number; | ||
| /** The user selected IMF in parts per million */ | ||
|
|
||
| imfPpm: number; | ||
| } | ||
| /** PerpetualLeverageEntry represents a single perpetual leverage setting for internal storage */ | ||
|
|
||
| export interface PerpetualLeverageEntrySDKType { | ||
| /** The perpetual ID (internal storage format) */ | ||
| perpetual_id: number; | ||
| /** The user selected IMF in parts per million */ | ||
|
|
||
| imf_ppm: number; | ||
| } | ||
| /** LeverageData represents the leverage settings for a subaccount */ | ||
|
|
||
| export interface LeverageData { | ||
| /** List of leverage entries for this subaccount */ | ||
| entries: PerpetualLeverageEntry[]; | ||
| } | ||
| /** LeverageData represents the leverage settings for a subaccount */ | ||
|
|
||
| export interface LeverageDataSDKType { | ||
| /** List of leverage entries for this subaccount */ | ||
| entries: PerpetualLeverageEntrySDKType[]; | ||
| } | ||
|
|
||
| function createBasePerpetualLeverageEntry(): PerpetualLeverageEntry { | ||
| return { | ||
| perpetualId: 0, | ||
| imfPpm: 0 | ||
| }; | ||
| } | ||
|
|
||
| export const PerpetualLeverageEntry = { | ||
| encode(message: PerpetualLeverageEntry, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer { | ||
| if (message.perpetualId !== 0) { | ||
| writer.uint32(8).uint32(message.perpetualId); | ||
| } | ||
|
|
||
| if (message.imfPpm !== 0) { | ||
| writer.uint32(16).uint32(message.imfPpm); | ||
| } | ||
|
|
||
| return writer; | ||
| }, | ||
|
|
||
| decode(input: _m0.Reader | Uint8Array, length?: number): PerpetualLeverageEntry { | ||
| const reader = input instanceof _m0.Reader ? input : new _m0.Reader(input); | ||
| let end = length === undefined ? reader.len : reader.pos + length; | ||
| const message = createBasePerpetualLeverageEntry(); | ||
|
|
||
| while (reader.pos < end) { | ||
| const tag = reader.uint32(); | ||
|
|
||
| switch (tag >>> 3) { | ||
| case 1: | ||
| message.perpetualId = reader.uint32(); | ||
| break; | ||
|
|
||
| case 2: | ||
| message.imfPpm = reader.uint32(); | ||
| break; | ||
|
|
||
| default: | ||
| reader.skipType(tag & 7); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return message; | ||
| }, | ||
|
|
||
| fromPartial(object: DeepPartial<PerpetualLeverageEntry>): PerpetualLeverageEntry { | ||
| const message = createBasePerpetualLeverageEntry(); | ||
| message.perpetualId = object.perpetualId ?? 0; | ||
| message.imfPpm = object.imfPpm ?? 0; | ||
| return message; | ||
| } | ||
|
|
||
| }; | ||
|
|
||
| function createBaseLeverageData(): LeverageData { | ||
| return { | ||
| entries: [] | ||
| }; | ||
| } | ||
|
|
||
| export const LeverageData = { | ||
| encode(message: LeverageData, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer { | ||
| for (const v of message.entries) { | ||
| PerpetualLeverageEntry.encode(v!, writer.uint32(10).fork()).ldelim(); | ||
| } | ||
|
|
||
| return writer; | ||
| }, | ||
|
|
||
| decode(input: _m0.Reader | Uint8Array, length?: number): LeverageData { | ||
| const reader = input instanceof _m0.Reader ? input : new _m0.Reader(input); | ||
| let end = length === undefined ? reader.len : reader.pos + length; | ||
| const message = createBaseLeverageData(); | ||
|
|
||
| while (reader.pos < end) { | ||
| const tag = reader.uint32(); | ||
|
|
||
| switch (tag >>> 3) { | ||
| case 1: | ||
| message.entries.push(PerpetualLeverageEntry.decode(reader, reader.uint32())); | ||
| break; | ||
|
|
||
| default: | ||
| reader.skipType(tag & 7); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return message; | ||
| }, | ||
|
|
||
| fromPartial(object: DeepPartial<LeverageData>): LeverageData { | ||
| const message = createBaseLeverageData(); | ||
| message.entries = object.entries?.map(e => PerpetualLeverageEntry.fromPartial(e)) || []; | ||
| return message; | ||
| } | ||
|
|
||
| }; No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
CRITICAL: Regenerate protobuf files to fix pipeline failure.
The pipeline is failing because this auto-generated file doesn't match the expected output from the protobuf code generator. The build verification step detected diffs between the generated and committed code.
To fix this issue, regenerate the protobuf TypeScript files and commit the updated output:
After running the generation command, commit any changes to ensure the generated code matches the proto definitions.
🏁 Script executed:
#!/bin/bash
# Regenerate protobuf files in the indexer package
cd indexer
pnpm run build:proto
# Check if there are any differences after regeneration
git diff --exit-code packages/v4-protos/src/codegen/Length of output: 122
🏁 Script executed:
#!/bin/bash
# Find the indexer package.json and check available scripts
cat indexer/package.json | grep -A 50 '"scripts"'Length of output: 809
🏁 Script executed:
#!/bin/bash
# Search for protobuf generation configuration or tooling
fd -e proto -e buf.yaml -e proto.lock 2>/dev/null | head -20Length of output: 841
🏁 Script executed:
#!/bin/bash
# Look for any Makefile or build configuration
fd -e Makefile -e makefile -e buf.gen.yaml -e "*.mk" 2>/dev/null | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for references to protobuf or code generation in package.json files
rg -t json "proto\|codegen" --type-list | head -20Length of output: 389
🏁 Script executed:
#!/bin/bash
# Check if there's a root-level package.json with build scripts that might handle proto generation
cat package.json | grep -A 100 '"scripts"' | head -50Length of output: 44
🏁 Script executed:
#!/bin/bash
# Look for buf configuration or protoc usage
rg -i "buf\|protoc" --type yaml --type toml --type json -n | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the v4-protos package scripts
cat indexer/packages/v4-protos/package.json | grep -A 50 '"scripts"'Length of output: 1602
🏁 Script executed:
#!/bin/bash
# Find CI configuration files
fd -e yml -e yaml | grep -E "(github|gitlab|circle|ci)" | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for verify-proto or code generation references
rg "verify-proto|codegen" -n -C 3 | head -40Length of output: 3098
🏁 Script executed:
#!/bin/bash
# Look for build scripts in all package.json files
find . -name "package.json" -exec grep -l "proto\|codegen" {} \; | head -10Length of output: 808
🏁 Script executed:
#!/bin/bash
# Check if proto/Makefile exists and has proto-export target
if [ -f proto/Makefile ]; then
grep -A 5 "proto-export:" proto/Makefile | head -10
else
echo "No Makefile found in proto/"
ls -la proto/ | head -20
fiLength of output: 0
🏁 Script executed:
#!/bin/bash
# Verify the directory structure for telescope and proto-export
ls -la indexer/packages/v4-protos/src/ | head -20Length of output: 283
🏁 Script executed:
#!/bin/bash
# Check proto directory and Makefile
if [ -d proto ]; then
echo "=== proto directory exists ==="
ls -la proto/ | head -15
if [ -f proto/Makefile ]; then
echo "=== Makefile found, proto-export target ==="
grep "proto-export:" proto/Makefile
fi
else
echo "proto directory not found at root"
fiLength of output: 695
🏁 Script executed:
#!/bin/bash
# Display the actual Makefile content
cat proto/MakefileLength of output: 593
Regenerate protobuf files — but first, the build script is broken and must be fixed.
The pipeline fails because indexer/packages/v4-protos/package.json defines build:proto with a call to make proto-export, but this target does not exist in proto/Makefile. Only proto-export-v4-proto-js exists (marked as unused with a TODO).
Fix required: Either restore the proto-export Makefile target or update the build:proto script to call the correct target. Once corrected, regenerate the protobuf files by running:
cd indexer/packages/v4-protos
pnpm run build:protoCommit any changes to src/codegen/ to resolve the pipeline failure.
🧰 Tools
🪛 GitHub Actions: Indexer API Documentation Check
[error] 1-1: ELIFECYCLE: Command failed with exit code 1. pnpm run build:verify-proto failed (ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL).
🪛 GitHub Actions: Indexer Build, Test & Coverage
[error] 1-1: pnpm run build:verify-proto failed. Exit code 1. The build step reported diffs between generated code and committed code (patches applied), causing the recursive build to fail.
🤖 Prompt for AI Agents
indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/leverage.ts
lines 1-132: the CI fails because package.json's build:proto invokes make
proto-export which doesn't exist in proto/Makefile; update package.json to call
the existing proto-export-v4-proto-js target (or alternatively add a
proto-export target to proto/Makefile that forwards to
proto-export-v4-proto-js), then run cd indexer/packages/v4-protos && pnpm run
build:proto to regenerate the protobuf output, and commit any updated files
under src/codegen/.
There was a problem hiding this comment.
regenerated proto files
There was a problem hiding this comment.
@anmolagrawal345, thank you for regenerating the protobuf files! The CI pipeline will verify that the generated code now matches the expected output. Once the checks pass, this should resolve the build failure. 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| import * as _134 from "./api/annotations"; | ||
| import * as _135 from "./api/http"; | ||
| import * as _136 from "./protobuf/descriptor"; | ||
| import * as _137 from "./protobuf/duration"; | ||
| import * as _138 from "./protobuf/timestamp"; | ||
| import * as _139 from "./protobuf/any"; | ||
| export namespace google { | ||
| export const api = { ..._133, | ||
| ..._134 | ||
| export const api = { ..._134, | ||
| ..._135 | ||
| }; | ||
| export const protobuf = { ..._135, | ||
| ..._136, | ||
| export const protobuf = { ..._136, | ||
| ..._137, | ||
| ..._138 | ||
| ..._138, | ||
| ..._139 | ||
| }; | ||
| } |
There was a problem hiding this comment.
Critical: Pipeline failure due to generated code mismatch.
The build is failing because the committed code doesn't match what the protobuf code generator produces. This is a blocker that must be resolved before merging.
To fix this issue, regenerate the protobuf code by running:
cd indexer
pnpm run build:protoThen commit the newly generated files. This will ensure the committed code matches the expected generated output.
If you need help with the proto generation command or encounter any issues during regeneration, let me know.
🧰 Tools
🪛 GitHub Actions: Indexer API Documentation Check
[error] 1-1: ELIFECYCLE: Command failed with exit code 1. pnpm run build:verify-proto failed (ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL).
🪛 GitHub Actions: Indexer Build, Test & Coverage
[error] 1-1: pnpm run build:verify-proto failed. Exit code 1. The build step reported diffs between generated code and committed code (patches applied), causing the recursive build to fail.
🤖 Prompt for AI Agents
In indexer/packages/v4-protos/src/codegen/google/bundle.ts lines 1-16 the
committed generated protobuf code does not match the output from the proto
generator, causing pipeline failures; to fix, regenerate the protobuf outputs by
running from the repository root: cd indexer && pnpm run build:proto, verify the
changed files (including this bundle.ts) match the generator output, and commit
the updated generated files so the repo and CI use the correct generated code.
There was a problem hiding this comment.
regenerated proto files
There was a problem hiding this comment.
@anmolagrawal345, thank you for regenerating the proto files! The changes look good—the import aliases have been systematically renumbered, which is exactly what we'd expect from a fresh proto generation. This should resolve the pipeline failures.
Once the CI passes, this blocker will be cleared. 🎉
🐰✨
/_/
( o.o )
^ < "All fixed! Great work!"
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| // UpdateLeverage updates leverage for specific perpetuals for a subaccount. | ||
| func (k Keeper) UpdateLeverage( | ||
| ctx sdk.Context, | ||
| subaccountId *types.SubaccountId, | ||
| perpetualLeverage map[uint32]uint32, | ||
| ) error { | ||
| // Validate leverage against maximum allowed for each perpetual | ||
| for perpetualId, custom_imf_ppm := range perpetualLeverage { | ||
| minImfPpm, err := k.GetMinImfForPerpetual(ctx, perpetualId) | ||
| if err != nil { | ||
| return errorsmod.Wrapf( | ||
| types.ErrInvalidLeverage, | ||
| "failed to get max leverage for perpetual %d: %v", | ||
| perpetualId, | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| if custom_imf_ppm < minImfPpm { | ||
| return errorsmod.Wrapf( | ||
| types.ErrLeverageExceedsMaximum, | ||
| "%d is less than minimum allowed imf (%d) for perpetual %d resulting in higher than allowed leverage", | ||
| custom_imf_ppm, | ||
| minImfPpm, | ||
| perpetualId, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Get existing leverage data | ||
| existingLeverage, exists := k.GetLeverage(ctx, subaccountId) | ||
| if !exists { | ||
| existingLeverage = make(map[uint32]uint32) | ||
| } | ||
|
|
||
| // Update with new leverage values | ||
| for perpetualId, custom_imf_ppm := range perpetualLeverage { | ||
| existingLeverage[perpetualId] = custom_imf_ppm | ||
| } | ||
|
|
||
| // Store updated leverage | ||
| k.SetLeverage(ctx, subaccountId, existingLeverage) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Critical: Non-deterministic validation order could cause inconsistent errors.
Lines 69 and 98 iterate over maps without sorting. While the final state writes are deterministic (via SetLeverage), the validation loop (lines 69-89) returns on the first error encountered. Non-deterministic iteration means different nodes may fail on different perpetual IDs first, potentially producing different error messages or failure states.
Apply this diff to sort map keys before iteration:
+import (
+ "sort"
+ errorsmod "cosmossdk.io/errors"
+ "cosmossdk.io/store/prefix"
+ sdk "github.com/cosmos/cosmos-sdk/types"
+ "github.com/dydxprotocol/v4-chain/protocol/lib"
+ "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
+)
func (k Keeper) UpdateLeverage(
ctx sdk.Context,
subaccountId *types.SubaccountId,
perpetualLeverage map[uint32]uint32,
) error {
- // Validate leverage against maximum allowed for each perpetual
- for perpetualId, custom_imf_ppm := range perpetualLeverage {
+ // Sort perpetual IDs for deterministic validation
+ sortedPerpIds := lib.GetSortedKeys[lib.Sortable[uint32]](perpetualLeverage)
+
+ // Validate leverage against maximum allowed for each perpetual
+ for _, perpetualId := range sortedPerpIds {
+ custom_imf_ppm := perpetualLeverage[perpetualId]
minImfPpm, err := k.GetMinImfForPerpetual(ctx, perpetualId)
if err != nil {
return errorsmod.Wrapf(
types.ErrInvalidLeverage,
"failed to get max leverage for perpetual %d: %v",
perpetualId,
err,
)
}
if custom_imf_ppm < minImfPpm {
return errorsmod.Wrapf(
types.ErrLeverageExceedsMaximum,
"%d is less than minimum allowed imf (%d) for perpetual %d resulting in higher than allowed leverage",
custom_imf_ppm,
minImfPpm,
perpetualId,
)
}
}
// Get existing leverage data
existingLeverage, exists := k.GetLeverage(ctx, subaccountId)
if !exists {
existingLeverage = make(map[uint32]uint32)
}
// Update with new leverage values
for perpetualId, custom_imf_ppm := range perpetualLeverage {
existingLeverage[perpetualId] = custom_imf_ppm
}
// Store updated leverage
k.SetLeverage(ctx, subaccountId, existingLeverage)
return nil
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In protocol/x/subaccounts/keeper/leverage.go around lines 62 to 105, the code
iterates over the perpetualLeverage map in both the validation loop (lines
~69-89) and the update loop (lines ~98-105) which is non-deterministic; to fix,
collect the map keys into a slice, sort them (e.g., ascending uint32), and then
iterate over the sorted slice in both places so validation and updates occur in
a deterministic, reproducible order.
There was a problem hiding this comment.
@anmolagrawal345, excellent! Thank you for implementing the deterministic sorting fix. This ensures that validation errors will be consistent across all nodes in the network, which is crucial for maintaining consensus. The sorted iteration prevents scenarios where different validators might reject transactions with different error messages due to map iteration order differences.
🧠 Learnings used
Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.
Learnt from: hwray
PR: dydxprotocol/v4-chain#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.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| } | ||
|
|
||
| // UpdateLeverage updates leverage for specific perpetuals for a subaccount. | ||
| func (k Keeper) UpdateLeverage( |
There was a problem hiding this comment.
In general should we rename these methods [Update|Set|Get]CustomIMF?
There was a problem hiding this comment.
I think Leverage is cleaner - the IMF is more of an implementation detail?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
protocol/x/subaccounts/keeper/leverage.go (1)
120-135: Clarify the comment on line 129.The comment states "Calculate max leverage" but the function doesn't perform a calculation—it returns
InitialMarginPpmdirectly. While the comment explains the relationship, it may mislead readers.Consider revising the comment for clarity:
- // Calculate max leverage: 1,000,000 / initial_margin_ppm + // The InitialMarginPpm represents the minimum IMF; max leverage = 1,000,000 / InitialMarginPpm if liquidityTier.InitialMarginPpm == 0 { return 0, types.ErrInitialMarginPpmIsZero }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/x/subaccounts/keeper/leverage.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.
Applied to files:
protocol/x/subaccounts/keeper/leverage.go
🧬 Code graph analysis (1)
protocol/x/subaccounts/keeper/leverage.go (5)
protocol/x/subaccounts/types/keys.go (1)
LeverageKeyPrefix(30-30)protocol/x/subaccounts/types/leverage.pb.go (6)
PerpetualLeverageEntry(27-32)PerpetualLeverageEntry(36-36)PerpetualLeverageEntry(37-39)LeverageData(82-85)LeverageData(89-89)LeverageData(90-92)protocol/lib/collections.go (1)
GetSortedKeys(57-67)protocol/lib/sortable.go (1)
Sortable(9-9)protocol/x/subaccounts/types/errors.go (3)
ErrInvalidLeverage(83-83)ErrLeverageExceedsMaximum(84-84)ErrInitialMarginPpmIsZero(85-85)
⏰ 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). (39)
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- 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: check-build-auxo
- 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-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: check-build-bazooka
- 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-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-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-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: golangci-lint
- GitHub Check: liveness-test
- GitHub Check: test / run_command
- GitHub Check: test-coverage-upload
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: build-and-push-mainnet
- GitHub Check: exchange-tests
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: lint
- GitHub Check: container-tests
- GitHub Check: build-and-push-testnet
- GitHub Check: build
- GitHub Check: run_command
- GitHub Check: benchmark
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (2)
protocol/x/subaccounts/keeper/leverage.go (2)
50-69: LGTM!The function correctly retrieves and deserializes leverage data, reconstructing the map from the protobuf entries. The use of
MustUnmarshalis appropriate for consensus-critical storage.
71-118: Well-implemented validation with deterministic ordering.The function correctly validates leverage against minimum IMF requirements using sorted iteration (line 78), preventing the consensus/apphash divergence issues flagged in previous reviews. The in-memory map update (lines 110-113) is acceptable since
SetLeverageperforms deterministic serialization.Based on learnings.
| <<<<<<< Updated upstream | ||
| for perpetualId, CustomImfPpm := range leverageMap { | ||
| ======= | ||
| sortedPerpIds := lib.GetSortedKeys[lib.Sortable[uint32]](leverageMap) | ||
| for _, perpetualId := range sortedPerpIds { | ||
| customImfPpm := leverageMap[perpetualId] | ||
| >>>>>>> Stashed changes | ||
| entries = append(entries, &types.PerpetualLeverageEntry{ | ||
| PerpetualId: perpetualId, | ||
| CustomImfPpm: CustomImfPpm, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Critical: Unresolved merge conflict prevents compilation.
The file contains unresolved merge conflict markers. Additionally, if the "Stashed changes" version (lines 32-34) is kept, line 38 references CustomImfPpm but line 34 defines customImfPpm (lowercase), causing a compilation error.
Resolve the merge conflict by keeping the deterministic sorted iteration (lines 32-34) and fix the variable name consistency:
var entries []*types.PerpetualLeverageEntry
-<<<<<<< Updated upstream
- for perpetualId, CustomImfPpm := range leverageMap {
-=======
sortedPerpIds := lib.GetSortedKeys[lib.Sortable[uint32]](leverageMap)
for _, perpetualId := range sortedPerpIds {
customImfPpm := leverageMap[perpetualId]
->>>>>>> Stashed changes
entries = append(entries, &types.PerpetualLeverageEntry{
PerpetualId: perpetualId,
- CustomImfPpm: CustomImfPpm,
+ CustomImfPpm: customImfPpm,
})
}🤖 Prompt for AI Agents
In protocol/x/subaccounts/keeper/leverage.go around lines 29 to 40 there are
unresolved merge conflict markers and a variable-name mismatch; remove the
conflict markers, keep the deterministic sorted iteration using sortedPerpIds :=
lib.GetSortedKeys[lib.Sortable[uint32]](leverageMap) and the for _, perpetualId
:= range sortedPerpIds loop, read the value as customImfPpm :=
leverageMap[perpetualId], and then use that same identifier when constructing
the PerpetualLeverageEntry (i.e. pass customImfPpm to CustomImfPpm) or
alternatively rename the loop variable to CustomImfPpm consistently; ensure no
leftover conflict markers remain so the file compiles.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
protocol/x/subaccounts/keeper/leverage.go (2)
67-98: Good deterministic validation, minor naming suggestions.The sorted iteration (line 74) ensures deterministic validation order, which is critical for consensus.
Optional: Consider naming consistency and error message clarity.
Variable naming:
custom_imf_ppm(lines 78, 107) uses snake_case, which is inconsistent with Go conventions. ConsidercustomImfPpmto match line 31 and Go style.Error message (line 83): Says "failed to get max leverage" but the function is
GetMinImfForPerpetual. Consider updating to "failed to get min IMF for perpetual".Apply this diff for consistency:
for _, perpetualId := range sortedPerpIds { - custom_imf_ppm := perpetualLeverage[perpetualId] + customImfPpm := perpetualLeverage[perpetualId] minImfPpm, err := k.GetMinImfForPerpetual(ctx, perpetualId) if err != nil { return errorsmod.Wrapf( types.ErrInvalidLeverage, - "failed to get max leverage for perpetual %d: %v", + "failed to get min IMF for perpetual %d: %v", perpetualId, err, ) } - if custom_imf_ppm < minImfPpm { + if customImfPpm < minImfPpm { return errorsmod.Wrapf( types.ErrLeverageExceedsMaximum, "%d is less than minimum allowed imf (%d) for perpetual %d resulting in higher than allowed leverage", - custom_imf_ppm, + customImfPpm, minImfPpm, perpetualId, ) } }
116-131: Misleading comment should be updated.The logic is correct, but the comment on line 125 is misleading.
The comment states "Calculate max leverage: 1,000,000 / initial_margin_ppm" but the code doesn't perform this calculation—it simply returns
InitialMarginPpm. This could confuse future maintainers about what the function actually does.Apply this diff to clarify:
- // Calculate max leverage: 1,000,000 / initial_margin_ppm + // Return the minimum IMF PPM from the liquidity tier if liquidityTier.InitialMarginPpm == 0 { return 0, types.ErrInitialMarginPpmIsZero }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/x/subaccounts/keeper/leverage.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.
Applied to files:
protocol/x/subaccounts/keeper/leverage.go
🧬 Code graph analysis (1)
protocol/x/subaccounts/keeper/leverage.go (5)
protocol/x/subaccounts/types/keys.go (1)
LeverageKeyPrefix(30-30)protocol/x/subaccounts/types/leverage.pb.go (6)
PerpetualLeverageEntry(27-32)PerpetualLeverageEntry(36-36)PerpetualLeverageEntry(37-39)LeverageData(82-85)LeverageData(89-89)LeverageData(90-92)protocol/lib/collections.go (1)
GetSortedKeys(57-67)protocol/lib/sortable.go (1)
Sortable(9-9)protocol/x/subaccounts/types/errors.go (3)
ErrInvalidLeverage(83-83)ErrLeverageExceedsMaximum(84-84)ErrInitialMarginPpmIsZero(85-85)
⏰ 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). (39)
- GitHub Check: test / run_command
- GitHub Check: run_command
- GitHub Check: lint
- 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-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-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: container-tests
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- 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-comlink / (comlink) Check docker image build
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: check-build-auxo
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: build-and-push-testnet
- GitHub Check: build-and-push-mainnet
- GitHub Check: benchmark
- GitHub Check: liveness-test
- GitHub Check: golangci-lint
- GitHub Check: test-coverage-upload
- GitHub Check: exchange-tests
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (4)
protocol/x/subaccounts/keeper/leverage.go (4)
1-21: LGTM! Clean setup for leverage storage.The imports, prefix store initialization, and key generation are well-structured. Using
ToStateKey()ensures deterministic key generation.
23-44: Excellent! Deterministic serialization properly implemented.The use of sorted keys (line 29) and protobuf encoding (line 42) ensures deterministic state storage, which is critical for consensus. The past review concerns about non-deterministic JSON encoding and map iteration have been properly addressed.
46-65: LGTM! Proper deserialization and error handling.The method correctly handles missing leverage data and properly deserializes protobuf entries back into a map.
100-114: LGTM! Proper merge and storage logic.The method correctly merges new leverage values with existing data. The unsorted iteration (lines 107-109) is acceptable here since it's only updating an in-memory map, and
SetLeveragehandles deterministic storage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
protocol/x/subaccounts/keeper/leverage.go (2)
65-96: Validation loop correctly ensures deterministic error handling.The sorted iteration (line 72) ensures all nodes validate perpetuals in the same order and return consistent errors. The validation logic correctly enforces that
custom_imf_ppm >= minImfPpm.Minor: The error message at line 81 says "failed to get max leverage" but the function is
GetMinImfForPerpetual. Consider: "failed to get min IMF for perpetual %d: %v".
98-112: Consider sorting the update loop for code consistency.While the unsorted iteration (lines 105-107) doesn't affect consensus safety (since
SetLeveragehandles deterministic storage), sorting here would maintain consistency with the validation loop above and improve code readability.Apply this diff for consistency:
// Update with new leverage values - for perpetualId, custom_imf_ppm := range perpetualLeverage { + for _, perpetualId := range sortedPerpIds { + custom_imf_ppm := perpetualLeverage[perpetualId] existingLeverage[perpetualId] = custom_imf_ppm }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/x/subaccounts/keeper/leverage.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.
Applied to files:
protocol/x/subaccounts/keeper/leverage.go
🧬 Code graph analysis (1)
protocol/x/subaccounts/keeper/leverage.go (6)
protocol/x/clob/keeper/keeper.go (1)
Keeper(29-71)protocol/x/subaccounts/types/keys.go (1)
LeverageKeyPrefix(30-30)protocol/x/subaccounts/types/leverage.pb.go (6)
PerpetualLeverageEntry(27-32)PerpetualLeverageEntry(36-36)PerpetualLeverageEntry(37-39)LeverageData(82-85)LeverageData(89-89)LeverageData(90-92)protocol/lib/collections.go (1)
GetSortedKeys(57-67)protocol/lib/sortable.go (1)
Sortable(9-9)protocol/x/subaccounts/types/errors.go (3)
ErrInvalidLeverage(83-83)ErrLeverageExceedsMaximum(84-84)ErrInitialMarginPpmIsZero(85-85)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Summary
🔇 Additional comments (3)
protocol/x/subaccounts/keeper/leverage.go (3)
11-19: LGTM: Standard storage helper functions.The prefix store pattern and key generation using
ToStateKey()follow Cosmos SDK conventions correctly.
21-42: LGTM: Deterministic storage implementation.The function correctly sorts perpetual IDs before building the entry slice, ensuring deterministic protobuf serialization across all nodes.
44-63: LGTM: Clean retrieval logic.The conversion from the deterministic stored slice back to a map is appropriate for read operations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
protocol/x/subaccounts/keeper/leverage.go (4)
16-19: Document/guard nil subaccountId.Add a brief precondition comment (or a nil check) to avoid a less-informative NPE if callers pass nil.
// leverageKey returns the store key to retrieve leverage data for a subaccount. func leverageKey(subaccountId *types.SubaccountId) []byte { + // Precondition: subaccountId must be non-nil. + // (Callers should validate upstream; this helps future-proof against panics.) + if subaccountId == nil { + panic("leverageKey: subaccountId is nil") + } return subaccountId.ToStateKey() }
26-34: Deterministic build is good; preallocate and (optionally) drop empty sets.
- Preallocate entries capacity to reduce allocs.
- If leverageMap is empty, delete the key to save storage and make GetLeverage() return exists=false.
func (k Keeper) SetLeverage(ctx sdk.Context, subaccountId *types.SubaccountId, leverageMap map[uint32]uint32) { store := k.getLeverageStore(ctx) key := leverageKey(subaccountId) - var entries []*types.PerpetualLeverageEntry + if len(leverageMap) == 0 { + store.Delete(key) + return + } + entries := make([]*types.PerpetualLeverageEntry, 0, len(leverageMap)) sortedPerpIds := lib.GetSortedKeys[lib.Sortable[uint32]](leverageMap) for _, perpetualId := range sortedPerpIds { customImfPpm := leverageMap[perpetualId] entries = append(entries, &types.PerpetualLeverageEntry{ PerpetualId: perpetualId, CustomImfPpm: customImfPpm, }) }
104-107: Optional: reuse the sorted keys for consistency.Not required for determinism (SetLeverage sorts), but iterating the same sorted slice improves readability and consistency.
-// Update with new leverage values -for perpetualId, custom_imf_ppm := range perpetualLeverage { - existingLeverage[perpetualId] = custom_imf_ppm -} +// Update with new leverage values (reuse deterministic order) +for _, perpetualId := range sortedPerpIds { + existingLeverage[perpetualId] = perpetualLeverage[perpetualId] +}
114-128: LGTM; consider a more explicit name.Implementation and docs are clear. If you want extra precision: GetMinImfPpmForPerpetual.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/x/subaccounts/keeper/leverage.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
PR: dydxprotocol/v4-chain#3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.
Applied to files:
protocol/x/subaccounts/keeper/leverage.go
🧬 Code graph analysis (1)
protocol/x/subaccounts/keeper/leverage.go (5)
protocol/x/subaccounts/types/keys.go (1)
LeverageKeyPrefix(30-30)protocol/x/subaccounts/types/leverage.pb.go (6)
PerpetualLeverageEntry(27-32)PerpetualLeverageEntry(36-36)PerpetualLeverageEntry(37-39)LeverageData(82-85)LeverageData(89-89)LeverageData(90-92)protocol/lib/collections.go (1)
GetSortedKeys(57-67)protocol/lib/sortable.go (1)
Sortable(9-9)protocol/x/subaccounts/types/errors.go (3)
ErrInvalidLeverage(83-83)ErrLeverageExceedsMaximum(84-84)ErrInitialMarginPpmIsZero(85-85)
⏰ 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). (39)
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-coverage-upload
- GitHub Check: container-tests
- GitHub Check: build
- 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-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-ender / (ender) 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: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- 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-vulcan / (vulcan) Build and Push
- GitHub Check: check-build-auxo
- GitHub Check: test / run_command
- GitHub Check: golangci-lint
- GitHub Check: exchange-tests
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build-and-push-testnet
- GitHub Check: benchmark
- GitHub Check: build-and-push-mainnet
- GitHub Check: run_command
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
protocol/x/subaccounts/keeper/leverage.go (3)
11-14: LGTM on namespaced store.Prefix store + LeverageKeyPrefix is correct and consistent with module patterns.
40-42: LGTM on protobuf marshal/store.Correct codec usage and write path.
49-63: LGTM on decode and map reconstruction.Clear exists=false contract when no key; map reconstruction is straightforward.
| // Sort the perpetual IDs to ensure deterministic ordering | ||
| sortedPerpIds := lib.GetSortedKeys[lib.Sortable[uint32]](perpetualLeverage) | ||
|
|
||
| // Validate leverage against maximum allowed leverage for each perpetual | ||
| for _, perpetualId := range sortedPerpIds { | ||
| custom_imf_ppm := perpetualLeverage[perpetualId] | ||
| minImfPpm, err := k.GetMinImfForPerpetual(ctx, perpetualId) | ||
| if err != nil { | ||
| return errorsmod.Wrapf( | ||
| types.ErrInvalidLeverage, | ||
| "failed to get max leverage for perpetual %d: %v", | ||
| perpetualId, | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| if custom_imf_ppm < minImfPpm { | ||
| return errorsmod.Wrapf( | ||
| types.ErrLeverageExceedsMaximum, | ||
| "%d is less than minimum allowed imf (%d) for perpetual %d resulting in higher than allowed leverage", | ||
| custom_imf_ppm, | ||
| minImfPpm, | ||
| perpetualId, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix wording + unify var naming.
- Comment and error message mention “max leverage” but you’re querying minimum IMF; adjust for clarity.
- Use Go‑idiomatic variable naming (customImfPpm).
-// Validate leverage against maximum allowed leverage for each perpetual
+// Validate custom IMF against the minimum allowed IMF (lower IMF => higher leverage) for each perpetual
for _, perpetualId := range sortedPerpIds {
- custom_imf_ppm := perpetualLeverage[perpetualId]
+ customImfPpm := perpetualLeverage[perpetualId]
minImfPpm, err := k.GetMinImfForPerpetual(ctx, perpetualId)
if err != nil {
return errorsmod.Wrapf(
types.ErrInvalidLeverage,
- "failed to get max leverage for perpetual %d: %v",
+ "failed to get minimum IMF for perpetual %d: %v",
perpetualId,
err,
)
}
- if custom_imf_ppm < minImfPpm {
+ if customImfPpm < minImfPpm {
return errorsmod.Wrapf(
types.ErrLeverageExceedsMaximum,
"%d is less than minimum allowed imf (%d) for perpetual %d resulting in higher than allowed leverage",
- custom_imf_ppm,
+ customImfPpm,
minImfPpm,
perpetualId,
)
}
}🤖 Prompt for AI Agents
In protocol/x/subaccounts/keeper/leverage.go around lines 71 to 96, the comment
and one error message refer to “max leverage” while the code fetches a minimum
IMF; rename the comment to state “minimum IMF” and update the error text to say
“failed to get minimum IMF” (or similar accurate wording). Also rename the Go
variable custom_imf_ppm to idiomatic customImfPpm (update all uses) to follow Go
naming conventions, and ensure the wrapped error messages reference the correct
variables/phrasing (minimum IMF) for clarity.
|
@Mergifyio backport release/protocol/v9.x |
✅ Backports have been createdDetails
|
4 participants
Changelist
Using the configured leverage for a subaccount, validate that the user has enough collateral on order placement to satisfy the scaled IMR requirements. Remember that the MMR, or liquidation level, does not change with leverage, so all that matters here is scaling the IMR to ensure that the net collateral is sufficient.
Test Plan
Added leverage_e2e_test to check a case where Alice and Bob place the exact same trade, but Alice has a lower leverage configured and thus the order placement fails at the collateral check phase.
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
Improvements
Tests