Add initial impl - need to fix current test #3249
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds a minimum-deposit constraint check for subaccount updates: new constant and update result, a helper that flags USDC deposits below the minimum on new/zero-balance subaccounts, and integration of that check into the Withdrawal/Transfer validation flow to short-circuit further checks on violation. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Validator as internalCanUpdateSubaccountsWithLeverage
participant DepositCheck as checkDepositConstraints
Client->>Validator: Submit Withdrawal/Transfer updates
activate Validator
rect rgb(235,245,235)
Note over Validator,DepositCheck: Early deposit-constraint validation
Validator->>DepositCheck: Pass settled updates
activate DepositCheck
DepositCheck->>DepositCheck: Iterate updates\n(USDC delta checks on zero-balance subaccounts)
alt Violation found
DepositCheck-->>Validator: Violations present
else No violation
DepositCheck-->>Validator: No violations
end
deactivate DepositCheck
end
alt Violations present
Validator-->>Client: Reject (ViolatesDepositConstraints)
else No violations
Validator->>Validator: Continue TNC & outage checks
Validator-->>Client: Proceed with update
end
deactivate Validator
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
protocol/x/subaccounts/keeper/subaccount.go(2 hunks)protocol/x/subaccounts/types/update.go(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/subaccounts/types/update.goprotocol/x/subaccounts/keeper/subaccount.go
📚 Learning: 2024-11-15T15:59:28.095Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:833-850
Timestamp: 2024-11-15T15:59:28.095Z
Learning: The function `GetInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/subaccounts/types/update.goprotocol/x/subaccounts/keeper/subaccount.go
📚 Learning: 2024-11-15T16:17:29.092Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/subaccounts/keeper/subaccount.go
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 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/subaccount.go
🧬 Code graph analysis (1)
protocol/x/subaccounts/keeper/subaccount.go (3)
protocol/x/subaccounts/types/settled_update.go (1)
SettledUpdate(7-17)protocol/x/subaccounts/types/update.go (3)
UpdateResult(10-10)MIN_SUBACCOUNT_INITIAL_DEPOSIT_QUANTUMS(163-163)ViolatesDepositConstraints(75-75)protocol/x/assets/types/genesis.go (1)
AssetUsdc(12-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: build-and-push-testnet
- 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-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-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: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-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-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: benchmark
- GitHub Check: build-and-push-mainnet
- GitHub Check: container-tests
- GitHub Check: test-race
- GitHub Check: golangci-lint
- GitHub Check: build
- GitHub Check: test-coverage-upload
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: liveness-test
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
Comment on lines
583
to
+588
| if updateType == types.Withdrawal || updateType == types.Transfer { | ||
| success, successPerUpdate = k.checkDepositConstraints(settledUpdates) | ||
| if !success { | ||
| return success, successPerUpdate, nil | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Include deposit updates in the minimum deposit gate
Right now we only invoke checkDepositConstraints for Withdrawal and Transfer. Any Deposit update skips this guard entirely, so a user can still open a fresh subaccount with a sub-0.1 USDC deposit via the deposit flow, which defeats the whole intent of MIN_SUBACCOUNT_INITIAL_DEPOSIT_QUANTUMS. Please run the same check for types.Deposit updates as well.
- if updateType == types.Withdrawal || updateType == types.Transfer {
+ if updateType == types.Withdrawal || updateType == types.Transfer || updateType == types.Deposit {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if updateType == types.Withdrawal || updateType == types.Transfer { | |
| success, successPerUpdate = k.checkDepositConstraints(settledUpdates) | |
| if !success { | |
| return success, successPerUpdate, nil | |
| } | |
| if updateType == types.Withdrawal || updateType == types.Transfer || updateType == types.Deposit { | |
| success, successPerUpdate = k.checkDepositConstraints(settledUpdates) | |
| if !success { | |
| return success, successPerUpdate, nil | |
| } |
🤖 Prompt for AI Agents
In protocol/x/subaccounts/keeper/subaccount.go around lines 583 to 588, the
deposit minimum check is only applied for Withdrawal and Transfer updates so
Deposit updates bypass the MIN_SUBACCOUNT_INITIAL_DEPOSIT_QUANTUMS gate; include
types.Deposit in the conditional (or otherwise invoke k.checkDepositConstraints
when updateType == types.Deposit) so deposit updates run the same check, return
the same success, successPerUpdate, nil on failure, and preserve existing
control flow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
protocol/x/subaccounts/keeper/subaccount.go (1)
587-587: Deposit updates bypass the minimum deposit constraint.The
checkDepositConstraintsfunction is only invoked forWithdrawalandTransferupdate types. DirectDepositupdates to new subaccounts bypass this gate entirely, allowing users to open fresh accounts with sub-0.1 USDC deposits and defeating the purpose ofMIN_SUBACCOUNT_INITIAL_DEPOSIT_QUANTUMS.Apply this diff to include deposits in the check:
- if updateType == types.Withdrawal || updateType == types.Transfer { + if updateType == types.Withdrawal || updateType == types.Transfer || updateType == types.Deposit {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/x/subaccounts/keeper/subaccount.go(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/subaccounts/keeper/subaccount.go
📚 Learning: 2024-11-15T15:59:28.095Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:833-850
Timestamp: 2024-11-15T15:59:28.095Z
Learning: The function `GetInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/subaccounts/keeper/subaccount.go
📚 Learning: 2024-11-15T16:17:29.092Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/subaccounts/keeper/subaccount.go
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 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/subaccount.go
🧬 Code graph analysis (1)
protocol/x/subaccounts/keeper/subaccount.go (3)
protocol/x/subaccounts/types/settled_update.go (1)
SettledUpdate(7-17)protocol/x/subaccounts/types/update.go (3)
UpdateResult(10-10)MIN_SUBACCOUNT_INITIAL_DEPOSIT_QUANTUMS(163-163)ViolatesDepositConstraints(75-75)protocol/x/assets/types/genesis.go (1)
AssetUsdc(12-19)
⏰ 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). (4)
- GitHub Check: test-sim-multi-seed-short
- GitHub Check: test-race
- GitHub Check: container-tests
- GitHub Check: Summary
🔇 Additional comments (2)
protocol/x/subaccounts/keeper/subaccount.go (2)
551-551: Resolve the TODO or remove if no longer applicable.The TODO suggests uncertainty about whether this constraint breaks existing valid flows. Please verify that this implementation doesn't inadvertently block legitimate use cases, or remove the TODO if validation is complete.
593-654: Code organization looks good.The deposit constraint check is properly sequenced before the withdrawal/transfer blocking logic, maintaining a clear validation flow.
|
|
||
| for i, u := range settledUpdates { | ||
| for _, assetUpdate := range u.AssetUpdates { | ||
| // Reject deposit if the account the account is new (no perp position, no usdc position) and the deposit/transfer |
Contributor
There was a problem hiding this comment.
Fix typo in comment.
The comment contains a repeated phrase.
Apply this diff:
- // Reject deposit if the account the account is new (no perp position, no usdc position) and the deposit/transfer
+ // Reject deposit if the account is new (no perp position, no usdc position) and the deposit/transfer📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Reject deposit if the account the account is new (no perp position, no usdc position) and the deposit/transfer | |
| // Reject deposit if the account is new (no perp position, no usdc position) and the deposit/transfer |
🤖 Prompt for AI Agents
In protocol/x/subaccounts/keeper/subaccount.go around line 549, the comment
currently reads "Reject deposit if the account the account is new..." with a
duplicated phrase; remove the extra "the account" so the comment becomes "Reject
deposit if the account is new (no perp position, no usdc position) and the
deposit/transfer" to fix the typo and restore clarity.
Comment on lines
+552
to
+558
| if assetUpdate.AssetId == assettypes.AssetUsdc.Id && | ||
| assetUpdate.BigQuantumsDelta.Cmp(big.NewInt(types.MIN_SUBACCOUNT_INITIAL_DEPOSIT_QUANTUMS)) < 0 && | ||
| u.SettledSubaccount.GetUsdcPosition().Sign() == 0 && | ||
| len(u.SettledSubaccount.PerpetualPositions) == 0 { | ||
| successPerUpdate[i] = types.ViolatesDepositConstraints | ||
| success = false | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add positive delta check for clarity and efficiency.
The comparison assetUpdate.BigQuantumsDelta.Cmp(...) < 0 will evaluate true for any negative delta (e.g., withdrawals). While withdrawals from new accounts would fail elsewhere, checking negative deltas here wastes gas and reduces clarity. The constraint should only apply to incoming deposits/transfers (positive deltas).
Apply this diff:
if assetUpdate.AssetId == assettypes.AssetUsdc.Id &&
+ assetUpdate.BigQuantumsDelta.Sign() > 0 &&
assetUpdate.BigQuantumsDelta.Cmp(big.NewInt(types.MIN_SUBACCOUNT_INITIAL_DEPOSIT_QUANTUMS)) < 0 &&
u.SettledSubaccount.GetUsdcPosition().Sign() == 0 &&
len(u.SettledSubaccount.PerpetualPositions) == 0 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if assetUpdate.AssetId == assettypes.AssetUsdc.Id && | |
| assetUpdate.BigQuantumsDelta.Cmp(big.NewInt(types.MIN_SUBACCOUNT_INITIAL_DEPOSIT_QUANTUMS)) < 0 && | |
| u.SettledSubaccount.GetUsdcPosition().Sign() == 0 && | |
| len(u.SettledSubaccount.PerpetualPositions) == 0 { | |
| successPerUpdate[i] = types.ViolatesDepositConstraints | |
| success = false | |
| } | |
| if assetUpdate.AssetId == assettypes.AssetUsdc.Id && | |
| assetUpdate.BigQuantumsDelta.Sign() > 0 && | |
| assetUpdate.BigQuantumsDelta.Cmp(big.NewInt(types.MIN_SUBACCOUNT_INITIAL_DEPOSIT_QUANTUMS)) < 0 && | |
| u.SettledSubaccount.GetUsdcPosition().Sign() == 0 && | |
| len(u.SettledSubaccount.PerpetualPositions) == 0 { | |
| successPerUpdate[i] = types.ViolatesDepositConstraints | |
| success = false | |
| } |
🤖 Prompt for AI Agents
In protocol/x/subaccounts/keeper/subaccount.go around lines 552 to 558, the
current check applies the minimum-initial-deposit constraint to any negative
delta as well as positive ones; change it to only run for positive deltas to
improve clarity and gas (first check assetUpdate.BigQuantumsDelta.Sign() > 0,
then compare with types.MIN_SUBACCOUNT_INITIAL_DEPOSIT_QUANTUMS), keeping the
rest of the condition the same and short-circuiting early to avoid unnecessary
Cmp calls for withdrawals.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
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
Other