[Performance] Optimize state-reads of perpetual information when doing collateral checks #1691
Conversation
|
Warning Rate limit exceeded@BrendanChou has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 30 minutes and 25 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe refactoring consolidates individual maps for Changes
Sequence DiagramsequenceDiagram
participant Client as Liquidation Client
participant Keeper as Subaccount Keeper
participant Store as Perpetual Store
Note over Client: Initiate Functions
Client->>Client: FetchApplicationStateAtBlockHeight(perpInfos)
Client->>Client: GetLiquidatableSubaccountIds(perpInfos)
Client->>Client: CheckSubaccountCollateralization(perpInfos)
Note over Keeper: Handle Isolated Perpetuals
Keeper->>Keeper: checkIsolatedSubaccountConstraints(perpInfos)
Keeper->>Keeper: isValidIsolatedPerpetualUpdates(perpInfos)
Keeper->>Keeper: GetIsolatedPerpetualStateTransition(perpInfos)
Note over Subaccounts: Manage Subaccounts
Keeper->>Store: UpdateSubaccounts(ctx, updates, perpInfos, updateType)
Store-->>Keeper: Updated Subaccounts
Keeper->>Store: CanUpdateSubaccounts(ctx, updates, perpInfos, updateType)
Store-->>Keeper: Validated Updates
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
BrendanChou
force-pushed
the
bc/collat-relevant-perpetuals
branch
from
b536cd0 to
52c130f
Compare
BrendanChou
force-pushed
the
bc/perf/get-perpetuals-twice
branch
from
4dce300 to
9065511
Compare
BrendanChou
force-pushed
the
bc/collat-relevant-perpetuals
branch
from
52c130f to
c14b151
Compare
BrendanChou
force-pushed
the
bc/collat-relevant-perpetuals
branch
from
c14b151 to
1c0cd93
Compare
| pricestypes "github.com/dydxprotocol/v4-chain/protocol/x/prices/types" | ||
| ) | ||
|
|
||
| type PerpInfo struct { |
There was a problem hiding this comment.
arguably this could also be a type in x/perpetuals instead
There was a problem hiding this comment.
Agree that'd be better for modularity. Also can we add some documentation on on the struct, e.g. "information needed to calculate collateral/margin requirement for a perpetual" (?)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
protocol/x/subaccounts/keeper/subaccount_helper.go (1)
Line range hint
107-144: The refactoring ofUpdatePerpetualPositionsto use theperpInfosmap aligns with the PR's objectives. The implementation efficiently updates perpetual positions based on the provided map. However, consider handling the error without panicking, which could be disruptive in a production environment.- panic(errorsmod.Wrapf(types.ErrPerpetualInfoDoesNotExist, "%d", pu.PerpetualId)) + return errorsmod.Wrapf(types.ErrPerpetualInfoDoesNotExist, "%d", pu.PerpetualId)protocol/x/subaccounts/keeper/isolated_subaccount.go (1)
Line range hint
75-100: Improve error handling inisValidIsolatedPerpetualUpdates.The function currently returns immediately on the first error encountered when checking for perpetual info existence. This could be improved by implementing a more comprehensive error handling strategy, such as logging additional context or using a multi-error pattern to capture more detailed error information.
protocol/x/subaccounts/keeper/subaccount.go (1)
Line range hint
446-461: Handle missing perpetual information gracefully inGetSettledSubaccountWithPerpetuals.The function now throws an error if perpetual information is missing in the
perpInfosmap. This is crucial for preventing runtime panics due to nil pointer dereferences but ensure that all calling functions properly handle this error scenario.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- protocol/daemons/liquidation/client/sub_task_runner.go (8 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (9 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (18 hunks)
- protocol/x/subaccounts/keeper/subaccount_helper.go (3 hunks)
- protocol/x/subaccounts/types/errors.go (1 hunks)
- protocol/x/subaccounts/types/perpinfo.go (1 hunks)
Files not reviewed due to errors (1)
- protocol/daemons/liquidation/client/sub_task_runner.go (no review received)
Additional context used
golangci-lint
protocol/daemons/liquidation/client/sub_task_runner.go
28-28: undefined: Client (typecheck)
42-42: undefined: Client (typecheck)
104-104: undefined: Client (typecheck)
121-121: undefined: newContextWithQueryBlockHeight (typecheck)
protocol/x/subaccounts/keeper/isolated_subaccount.go
24-24: undefined: Keeper (typecheck)
26-26: undefined: SettledUpdate (typecheck)
62-62: undefined: SettledUpdate (typecheck)
144-144: undefined: SettledUpdate (typecheck)
235-235: undefined: Keeper (typecheck)
318-318: undefined: Keeper (typecheck)
Additional comments not posted (9)
protocol/x/subaccounts/types/perpinfo.go (1)
8-12: The definition ofPerpInfostruct is well-structured and correctly encapsulates the necessary fields for perpetual information.protocol/x/subaccounts/types/errors.go (1)
68-72: The new errorErrPerpetualInfoDoesNotExistis appropriately defined with a clear message and unique error code. It enhances error handling by providing specific feedback when perpetual information is missing.protocol/x/subaccounts/keeper/isolated_subaccount_test.go (1)
322-331: The updates to the test functionTestGetIsolatedPerpetualStateTransitioncorrectly incorporate the newperpInfosmap, ensuring that the tests align with the changes made in the main codebase.protocol/x/subaccounts/keeper/isolated_subaccount.go (3)
27-27: Refactor to incorporateperpInfosmap in isolated subaccount operations.The integration of the
perpInfosmap into functions dealing with isolated subaccount constraints is a positive change, enhancing the clarity and efficiency of data handling in these operations.Also applies to: 63-63, 145-145, 321-321
321-327: Verify correct implementation of collateral transfer incomputeAndExecuteCollateralTransfer.Ensure that the logic for computing and executing collateral transfers based on isolated perpetual state transitions is correctly implemented, particularly in how it interacts with the
perpInfosmap.Verification successful
To ensure the correctness of the collateral transfer logic in
transferCollateralForIsolatedPerpetual, we need to review its implementation and how it is invoked. Let's extract the relevant code snippets for detailed analysis.
The implementation of
transferCollateralForIsolatedPerpetualappears to handle the collateral transfer logic correctly. It checks for valid state transitions, ensures there are positive quantums to transfer, identifies the correct source and destination addresses for the transfer, and finally, executes the transfer via thebankKeeper.The function is invoked within the same file, ensuring the correct application of the state transitions derived from
perpInfos.
- : The collateral transfer logic in
transferCollateralForIsolatedPerpetualis correctly implemented and invoked. The function handles state transitions, validates transfer conditions, and performs the necessary asset transfer operations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that collateral transfers are executed correctly based on state transitions derived from `perpInfos`. # Test: Search for the function usage to ensure correct implementation. rg --type go $'transferCollateralForIsolatedPerpetual'Length of output: 421
Script:
#!/bin/bash # Extract the implementation of the `transferCollateralForIsolatedPerpetual` function. ast-grep --lang go --pattern $'func (k *Keeper) transferCollateralForIsolatedPerpetual($_, $_) error { $$$ }' protocol/x/subaccounts/keeper/isolated_subaccount.go # Extract the lines of code where `transferCollateralForIsolatedPerpetual` is invoked. rg --type go --context 5 'transferCollateralForIsolatedPerpetual' protocol/x/subaccounts/keeper/isolated_subaccount.goLength of output: 7872
37-37: Ensure accurate validation logic incheckIsolatedSubaccountConstraints.This function's reliance on the
isValidIsolatedPerpetualUpdatesfunction for validation suggests a need to verify that all updates are being correctly checked against the constraints defined in theperpInfosmap.Verification successful
The
isValidIsolatedPerpetualUpdatesfunction is designed to validate updates against specific constraints for isolated perpetuals. The function ensures that:
- Subaccounts with positions in isolated perpetuals do not receive updates for other perpetuals.
- Subaccounts with positions in non-isolated perpetuals do not receive updates for isolated perpetuals.
- Subaccounts with no positions are not updated to have positions in multiple isolated perpetuals or a mix of isolated and non-isolated perpetuals.
Given the detailed checks implemented in this function, it appears the validation logic is correctly enforced.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all perpetual updates are validated correctly against the constraints defined in `perpInfos`. # Test: Search for validation logic across the codebase. rg --type go $'isValidIsolatedPerpetualUpdates'Length of output: 253
Script:
#!/bin/bash # Description: Retrieve the implementation of isValidIsolatedPerpetualUpdates function for analysis. # Search for the function definition in the specified file. ast-grep --lang go --pattern $'func isValidIsolatedPerpetualUpdates($_, $_) { $$$ }' protocol/x/subaccounts/keeper/isolated_subaccount.goLength of output: 140
Script:
#!/bin/bash # Description: Retrieve the implementation of isValidIsolatedPerpetualUpdates function for analysis. # Search for the function definition in the specified file. rg --type go --pcre2 --context 10 'func isValidIsolatedPerpetualUpdates\(' protocol/x/subaccounts/keeper/isolated_subaccount.goLength of output: 1168
protocol/x/subaccounts/keeper/subaccount.go (3)
Line range hint
214-238: RefactorgetSettledUpdatesto useperpInfosmap for enhanced efficiency and reduced complexity.This change aligns with the PR's objective to optimize state reads by centralizing perpetual information into a single map, reducing the need for multiple parameters and simplifying the function signature.
Line range hint
553-565: Validate updates against isolated perpetual constraints ininternalCanUpdateSubaccounts.This addition enhances the robustness of the update process by ensuring that updates meet certain constraints before being applied, which is vital for maintaining system integrity.
Line range hint
289-346: Ensure the newperpInfosmap is populated correctly before usage inUpdateSubaccounts.Verification successful
The
GetAllRelevantPerpetualsfunction correctly populates theperpInfosmap with all necessary perpetual information before it is used inUpdateSubaccounts. The function collects all relevant perpetual IDs from the updates and existing subaccounts, retrieves their information, and stores it in theperpInfosmap, with comprehensive error handling in place.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `GetAllRelevantPerpetuals` properly populates `perpInfos` map with all necessary perpetual information. # Test: Search for the function usage and check for proper initialization and error handling. rg --type go 'GetAllRelevantPerpetuals'Length of output: 595
Script:
#!/bin/bash # Extract the implementation of the GetAllRelevantPerpetuals function from the subaccount.go file. ast-grep --lang go --pattern $'func (k Keeper) GetAllRelevantPerpetuals($_, $_) ($_, $_) { $$$ }' --output FullLength of output: 314
Script:
#!/bin/bash # Extract the implementation of the GetAllRelevantPerpetuals function from the subaccount.go file. ast-grep --lang go --pattern $'func (k Keeper) GetAllRelevantPerpetuals($_, $_) ($_, $_) { $$$ }'Length of output: 3733
| @@ -146,9 +142,8 @@ func isValidIsolatedPerpetualUpdates( | |||
| // so all the updates must have been applied already to the subaccount. | |||
| func GetIsolatedPerpetualStateTransition( | |||
| settledUpdateWithUpdatedSubaccount SettledUpdate, | |||
| perpetuals []perptypes.Perpetual, | |||
| perpInfos map[uint32]types.PerpInfo, | |||
There was a problem hiding this comment.
Optimize the retrieval of perpetual information in state transition calculations.
Consider optimizing the retrieval and processing of perpetual information in this function to reduce the complexity and potential overhead associated with multiple map lookups.
Also applies to: 166-172
BrendanChou
force-pushed
the
bc/collat-relevant-perpetuals
branch
from
edb240e to
519f396
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
protocol/x/subaccounts/keeper/subaccount.go (1)
Line range hint
446-461: Handle missingPerpInfomore gracefully inGetSettledSubaccountWithPerpetuals.- return types.Subaccount{}, nil, errorsmod.Wrapf(types.ErrPerpetualInfoDoesNotExist, "%d", p.PerpetualId) + if !found { + return types.Subaccount{}, nil, errorsmod.NewNotFoundError("perpetual information", fmt.Sprintf("Perpetual ID: %d", p.PerpetualId)) + }Refactor error handling in
GetSettledSubaccountWithPerpetualsto use a more specific error type whenPerpInfois missing, improving clarity and error management.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/subaccounts/keeper/subaccount.go (19 hunks)
Additional context used
golangci-lint
protocol/x/subaccounts/keeper/subaccount.go
33-33: undefined: Keeper (typecheck)
61-61: undefined: Keeper (typecheck)
69-69: undefined: Keeper (typecheck)
217-217: undefined: SettledUpdate (typecheck)
551-551: undefined: SettledUpdate (typecheck)
880-880: undefined: SettledUpdate (typecheck)
311-311: undefined: GetDeltaOpenInterestFromUpdates (typecheck)
330-330: undefined: UpdatePerpetualPositions (typecheck)
336-336: undefined: UpdateAssetPositions (typecheck)
366-366: undefined: getUpdatedPerpetualPositions (typecheck)
Additional comments not posted (2)
protocol/x/subaccounts/keeper/subaccount.go (2)
Line range hint
214-238: OptimizegetSettledUpdatesfunction by integratingperpInfos.The integration of
perpInfosintogetSettledUpdatesstreamlines the retrieval of perpetual information and reduces state reads, aligning with the PR's objective to enhance performance.Tools
golangci-lint
217-217: undefined: SettledUpdate (typecheck)
Line range hint
346-357: Check for potential race conditions in collateral transfer computations.Potential race conditions could occur during collateral transfers. Consider implementing locking mechanisms or ensuring atomic operations when accessing shared resources like
perpInfos.
| perpInfos, err := k.GetAllRelevantPerpetuals(ctx, updates) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } | ||
|
|
||
| settledUpdates, subaccountIdToFundingPayments, err := k.getSettledUpdates(ctx, updates, perpInfos, true) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } | ||
|
|
||
| allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx) | ||
| success, successPerUpdate, err = k.internalCanUpdateSubaccounts( | ||
| ctx, | ||
| settledUpdates, | ||
| updateType, | ||
| allPerps, | ||
| perpInfos, |
There was a problem hiding this comment.
Tip
Codebase Verification
The UpdateSubaccounts function contains multiple error handling patterns where it returns early in case of errors. However, there is no evidence of rollback mechanisms being implemented within the function.
- File:
./protocol/x/subaccounts/keeper/subaccount.go - Lines with error handling patterns:
- 290-292
- 295-297
- 425-427
- 430-432
- 567-569
- 583-585
- 648-650
- 676-678
- 684-686
- 700-702
- 715-717
- 730-732
Recommendation:
Consider implementing a robust rollback mechanism to ensure consistency in the event of errors.
Analysis chain
Ensure error handling and rollback mechanisms are robust in UpdateSubaccounts.
Consider implementing a robust rollback mechanism for transactions within UpdateSubaccounts to ensure consistency in the event of errors.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all error paths in `UpdateSubaccounts` properly rollback changes.
# Test: Search for transaction rollback patterns in the function.
ast-grep --lang go --pattern $'func (k Keeper) UpdateSubaccounts($_) {
$$$
if $_ {
return false, nil, $_
}
$$$
}'
Length of output: 126
Script:
#!/bin/bash
# Locate the file containing the `UpdateSubaccounts` function
file_path=$(ast-grep --lang go --pattern 'func (k Keeper) UpdateSubaccounts($_) {' --json | jq -r '.[0].path')
# If the file is found, inspect it for error handling and rollback mechanisms
if [ -n "$file_path" ]; then
echo "File containing UpdateSubaccounts function: $file_path"
echo "Searching for error handling patterns..."
ast-grep --lang go --pattern 'if $_ { return false, nil, $_ }' "$file_path"
echo "Searching for rollback patterns..."
ast-grep --lang go --pattern 'k.Rollback$_' "$file_path"
else
echo "UpdateSubaccounts function not found."
fi
Length of output: 677
Script:
#!/bin/bash
# Locate the file containing the `UpdateSubaccounts` function using a corrected pattern
file_path=$(ast-grep --lang go --pattern 'func (k *Keeper) UpdateSubaccounts($_) {' --json | jq -r '.[0].path')
# If the file is found, inspect it for error handling and rollback mechanisms
if [ -n "$file_path" ]; then
echo "File containing UpdateSubaccounts function: $file_path"
echo "Searching for error handling patterns..."
ast-grep --lang go --pattern 'if $_ { return false, nil, $_ }' "$file_path"
echo "Searching for rollback patterns..."
ast-grep --lang go --pattern 'k.Rollback$_' "$file_path"
else
echo "UpdateSubaccounts function not found."
fi
Length of output: 679
Script:
#!/bin/bash
# Locate the file containing the `UpdateSubaccounts` function using a simplified pattern
file_path=$(ast-grep --lang go --pattern 'func UpdateSubaccounts($_) {' --json | jq -r '.[0].path')
# If the file is found, inspect it for error handling and rollback mechanisms
if [ -n "$file_path" ]; then
echo "File containing UpdateSubaccounts function: $file_path"
echo "Searching for error handling patterns..."
ast-grep --lang go --pattern 'if $_ { return false, nil, $_ }' "$file_path"
echo "Searching for rollback patterns..."
ast-grep --lang go --pattern 'k.Rollback$_' "$file_path"
else
echo "UpdateSubaccounts function not found."
fi
Length of output: 655
Script:
#!/bin/bash
# Locate the file containing the `UpdateSubaccounts` function using ripgrep
file_path=$(rg -l 'func (k *Keeper) UpdateSubaccounts' .)
# If the file is found, inspect it for error handling and rollback mechanisms
if [ -n "$file_path" ]; then
echo "File containing UpdateSubaccounts function: $file_path"
echo "Searching for error handling patterns..."
ast-grep --lang go --pattern 'if $_ { return false, nil, $_ }' "$file_path"
echo "Searching for rollback patterns..."
ast-grep --lang go --pattern 'k.Rollback$_' "$file_path"
else
echo "UpdateSubaccounts function not found."
fi
Length of output: 161
Script:
#!/bin/bash
# Locate the file containing the `UpdateSubaccounts` function using a refined search pattern with ripgrep
file_path=$(rg -l 'func (k Keeper) UpdateSubaccounts' .)
# If the file is found, inspect it for error handling and rollback mechanisms
if [ -n "$file_path" ]; then
echo "File containing UpdateSubaccounts function: $file_path"
echo "Searching for error handling patterns..."
ast-grep --lang go --pattern 'if $_ { return false, nil, $_ }' "$file_path"
echo "Searching for rollback patterns..."
ast-grep --lang go --pattern 'k.Rollback$_' "$file_path"
else
echo "UpdateSubaccounts function not found."
fi
Length of output: 160
Script:
#!/bin/bash
# Locate the file containing any mention of `UpdateSubaccounts` using ripgrep
file_path=$(rg -l 'UpdateSubaccounts' .)
# If the file is found, inspect it for error handling and rollback mechanisms
if [ -n "$file_path" ]; then
echo "File containing UpdateSubaccounts function: $file_path"
echo "Searching for error handling patterns..."
ast-grep --lang go --pattern 'if $_ { return false, nil, $_ }' "$file_path"
echo "Searching for rollback patterns..."
ast-grep --lang go --pattern 'k.Rollback$_' "$file_path"
else
echo "UpdateSubaccounts function not found."
fi
Length of output: 9837
Script:
#!/bin/bash
# Inspect the file `./protocol/x/subaccounts/keeper/subaccount.go` for the `UpdateSubaccounts` function
file_path="./protocol/x/subaccounts/keeper/subaccount.go"
# Search for the `UpdateSubaccounts` function definition
echo "Searching for UpdateSubaccounts function definition in $file_path..."
rg 'func (k Keeper) UpdateSubaccounts' "$file_path"
# Search for error handling patterns within the file
echo "Searching for error handling patterns in $file_path..."
ast-grep --lang go --pattern 'if $_ { return false, nil, $_ }' "$file_path"
# Search for rollback patterns within the file
echo "Searching for rollback patterns in $file_path..."
ast-grep --lang go --pattern 'k.Rollback$_' "$file_path"
Length of output: 3388
| pricestypes "github.com/dydxprotocol/v4-chain/protocol/x/prices/types" | ||
| ) | ||
|
|
||
| type PerpInfo struct { |
There was a problem hiding this comment.
Agree that'd be better for modularity. Also can we add some documentation on on the struct, e.g. "information needed to calculate collateral/margin requirement for a perpetual" (?)
| return l.Id | ||
| }) | ||
|
|
||
| return subaccounts, marketPricesMap, perpetualsMap, liquidityTiersMap, nil | ||
| perpInfos = make(map[uint32]satypes.PerpInfo, len(perpetuals)) |
There was a problem hiding this comment.
Nit: may be cleaner to make a helper function
func CreatePerpInfos(
perpetuals []perptypes.Perpetual,
marketPricesMap map[uint32]pricestypes.MarketPrice,
liquidityTiersMap map[uint32]perptypes.LiquidityTier,
) map[uint32]satypes.PerpInfo
| @@ -56,9 +56,7 @@ func (s *SubTaskRunnerImpl) RunLiquidationDaemonTaskLoop( | |||
|
|
|||
| // 1. Fetch all information needed to calculate total net collateral and margin requirements. | |||
There was a problem hiding this comment.
To confirm, there's no state read optimization in this file, just code cleanup right?
There was a problem hiding this comment.
Yes I suppose that is correct
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (7)
protocol/x/subaccounts/keeper/subaccount_helper.go (2)
Line range hint
108-140: Refactor to handle missing perpetual information more gracefully.- panic(errorsmod.Wrapf(types.ErrPerpetualInfoDoesNotExist, "%d", pu.PerpetualId)) + return errorsmod.Wrapf(types.ErrPerpetualInfoDoesNotExist, "Perpetual ID: %d", pu.PerpetualId)Using
paniccan abruptly terminate the program which isn't ideal for production environments. Consider returning an error instead, which allows the caller to handle it appropriately.
Line range hint
17-17: Address undefined type issue.+ type SettledUpdate struct { + // define necessary fields + }The type
SettledUpdateis referenced but not defined within this file or imported packages. Define this struct or ensure it's properly imported to resolve the typecheck errors.Also applies to: 58-58, 107-107
protocol/daemons/liquidation/client/sub_task_runner.go (2)
Line range hint
59-74: Ensure proper error handling and refactor for clarity.- if !ok { - return nil, nil, errorsmod.Wrapf(pricestypes.ErrMarketPriceDoesNotExist, "%d", perp.Params.MarketId) - } + if !ok { + return nil, nil, errorsmod.Wrapf(pricestypes.ErrMarketPriceDoesNotExist, "Market ID: %d", perp.Params.MarketId) + }Refactor the error messages to include more descriptive text. This enhances the readability and debuggability of the code.
Also applies to: 110-178, 185-209, 293-352
Line range hint
28-28: Resolve undefined references and functions.+ import "path/to/client" // Adjust the path as necessary + func newContextWithQueryBlockHeight(ctx context.Context, blockHeight uint32) context.Context { + // Implement the function + }The
Clienttype andnewContextWithQueryBlockHeightfunction are used but not defined or imported. Ensure these are correctly defined or imported to resolve the typecheck errors.Also applies to: 42-42, 104-104, 121-121
protocol/x/subaccounts/keeper/subaccount.go (3)
Line range hint
214-238: Consider adding error handling for the case whereperpInfosis nil to prevent potential panics.+ if perpInfos == nil { + return nil, nil, errors.New("perpInfos map cannot be nil") + }Tools
golangci-lint
217-217: undefined: SettledUpdate (typecheck)
Line range hint
289-346: Ensure proper rollback mechanisms are in place for transactions withinUpdateSubaccountsto maintain data consistency in case of errors.Consider implementing a robust rollback mechanism for transactions within
UpdateSubaccountsto ensure consistency in the event of errors.Tools
golangci-lint
311-311: undefined: GetDeltaOpenInterestFromUpdates (typecheck)
Line range hint
446-461: Add detailed logging for the scenario whenperpInfois not found inperpInfoswithinGetSettledSubaccountWithPerpetuals.Consider adding logging here to help trace which perpetual ID was not found in the map.
+ log.Printf("Perpetual ID not found: %d", p.PerpetualId)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/daemons/liquidation/client/sub_task_runner.go (8 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (9 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (19 hunks)
- protocol/x/subaccounts/keeper/subaccount_helper.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/isolated_subaccount_test.go
Additional context used
golangci-lint
protocol/x/subaccounts/keeper/subaccount_helper.go
17-17: undefined: SettledUpdate (typecheck)
58-58: undefined: SettledUpdate (typecheck)
107-107: undefined: SettledUpdate (typecheck)
protocol/daemons/liquidation/client/sub_task_runner.go
28-28: undefined: Client (typecheck)
42-42: undefined: Client (typecheck)
104-104: undefined: Client (typecheck)
121-121: undefined: newContextWithQueryBlockHeight (typecheck)
protocol/x/subaccounts/keeper/isolated_subaccount.go
24-24: undefined: Keeper (typecheck)
26-26: undefined: SettledUpdate (typecheck)
62-62: undefined: SettledUpdate (typecheck)
144-144: undefined: SettledUpdate (typecheck)
235-235: undefined: Keeper (typecheck)
318-318: undefined: Keeper (typecheck)
protocol/x/subaccounts/keeper/subaccount.go
33-33: undefined: Keeper (typecheck)
61-61: undefined: Keeper (typecheck)
69-69: undefined: Keeper (typecheck)
217-217: undefined: SettledUpdate (typecheck)
551-551: undefined: SettledUpdate (typecheck)
879-879: undefined: SettledUpdate (typecheck)
311-311: undefined: GetDeltaOpenInterestFromUpdates (typecheck)
330-330: undefined: UpdatePerpetualPositions (typecheck)
336-336: undefined: UpdateAssetPositions (typecheck)
366-366: undefined: getUpdatedPerpetualPositions (typecheck)
| // GetAllRelevantPerpetuals returns all relevant perpetual information for a given set of updates. | ||
| // This includes all perpetuals that exist on the accounts already and all perpetuals that are | ||
| // being updated in the input updates. | ||
| func (k Keeper) GetAllRelevantPerpetuals( | ||
| ctx sdk.Context, | ||
| updates []types.Update, | ||
| ) ( | ||
| map[uint32]perptypes.PerpInfo, | ||
| error, | ||
| ) { | ||
| subaccountIds := make(map[types.SubaccountId]struct{}) | ||
| perpIds := make(map[uint32]struct{}) | ||
|
|
||
| // Add all relevant perpetuals in every update. | ||
| for _, update := range updates { | ||
| // If this subaccount has not been processed already, get all of its existing perpetuals. | ||
| if _, exists := subaccountIds[update.SubaccountId]; !exists { | ||
| sa := k.GetSubaccount(ctx, update.SubaccountId) | ||
| for _, postition := range sa.PerpetualPositions { | ||
| perpIds[postition.PerpetualId] = struct{}{} | ||
| } | ||
| subaccountIds[update.SubaccountId] = struct{}{} | ||
| } | ||
|
|
||
| // Add all perpetuals in the update. | ||
| for _, perpUpdate := range update.PerpetualUpdates { | ||
| perpIds[perpUpdate.GetId()] = struct{}{} | ||
| } | ||
| } | ||
|
|
||
| // Get all perpetual information from state. | ||
| perpetuals := make(map[uint32]perptypes.PerpInfo, len(perpIds)) | ||
| for perpId := range perpIds { | ||
| perpetual, | ||
| price, | ||
| liquidityTier, | ||
| err := k.perpetualsKeeper.GetPerpetualAndMarketPriceAndLiquidityTier(ctx, perpId) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| perpetuals[perpId] = perptypes.PerpInfo{ | ||
| Perpetual: perpetual, | ||
| Price: price, | ||
| LiquidityTier: liquidityTier, | ||
| } | ||
| } | ||
|
|
||
| return perpetuals, nil | ||
| } |
There was a problem hiding this comment.
Optimize the retrieval of all relevant perpetual information in GetAllRelevantPerpetuals.
Consider caching the results of GetAllRelevantPerpetuals if it is called multiple times within the same transaction or query to avoid redundant state reads.
| for _, perp := range allPerps { | ||
| perpIdToFundingIndex[perp.Params.Id] = perp.FundingIndex | ||
| } | ||
|
|
||
| // Get OpenInterestDelta from the updates, and persist the OI change if any. | ||
| perpOpenInterestDelta := GetDeltaOpenInterestFromUpdates(settledUpdates, updateType) |
There was a problem hiding this comment.
Undefined functions are referenced in this file, which could lead to runtime errors.
Ensure that all functions like GetDeltaOpenInterestFromUpdates, UpdatePerpetualPositions, UpdateAssetPositions, and getUpdatedPerpetualPositions are defined or imported correctly.
Also applies to: 330-330, 336-336, 366-366
Tools
golangci-lint
311-311: undefined: GetDeltaOpenInterestFromUpdates (typecheck)
| perpInfos, err := k.GetAllRelevantPerpetuals(ctx, updates) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } | ||
|
|
||
| allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx) | ||
| success, successPerUpdate, err = k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType, allPerps) | ||
| return success, successPerUpdate, err | ||
| } | ||
|
|
||
| // getSettledSubaccount returns 1. a new settled subaccount given an unsettled subaccount, | ||
| // updating the USDC AssetPosition, FundingIndex, and LastFundingPayment fields accordingly | ||
| // (does not persist any changes) and 2. a map with perpetual ID as key and last funding | ||
| // payment as value (for emitting funding payments to indexer). | ||
| func (k Keeper) getSettledSubaccount( | ||
| ctx sdk.Context, | ||
| subaccount types.Subaccount, | ||
| ) ( | ||
| settledSubaccount types.Subaccount, | ||
| fundingPayments map[uint32]dtypes.SerializableInt, | ||
| err error, | ||
| ) { | ||
| // Fetch all relevant perpetuals. | ||
| perpetuals := make(map[uint32]perptypes.Perpetual, len(subaccount.PerpetualPositions)) | ||
| for _, p := range subaccount.PerpetualPositions { | ||
| perpetual, err := k.perpetualsKeeper.GetPerpetual(ctx, p.PerpetualId) | ||
| if err != nil { | ||
| return types.Subaccount{}, nil, err | ||
| } | ||
| perpetuals[p.PerpetualId] = perpetual | ||
| settledUpdates, _, err := k.getSettledUpdates(ctx, updates, perpInfos, false) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } | ||
|
|
||
| return GetSettledSubaccountWithPerpetuals(subaccount, perpetuals) | ||
| success, successPerUpdate, err = k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType, perpInfos) | ||
| return success, successPerUpdate, err |
There was a problem hiding this comment.
Enhance error messages in CanUpdateSubaccounts to provide more context about failures, improving debuggability and maintainability.
- return false, nil, err
+ return false, nil, errorsmod.Wrapf(err, "Failed to validate updates in CanUpdateSubaccounts")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.
| perpInfos, err := k.GetAllRelevantPerpetuals(ctx, updates) | |
| if err != nil { | |
| return false, nil, err | |
| } | |
| allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx) | |
| success, successPerUpdate, err = k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType, allPerps) | |
| return success, successPerUpdate, err | |
| } | |
| // getSettledSubaccount returns 1. a new settled subaccount given an unsettled subaccount, | |
| // updating the USDC AssetPosition, FundingIndex, and LastFundingPayment fields accordingly | |
| // (does not persist any changes) and 2. a map with perpetual ID as key and last funding | |
| // payment as value (for emitting funding payments to indexer). | |
| func (k Keeper) getSettledSubaccount( | |
| ctx sdk.Context, | |
| subaccount types.Subaccount, | |
| ) ( | |
| settledSubaccount types.Subaccount, | |
| fundingPayments map[uint32]dtypes.SerializableInt, | |
| err error, | |
| ) { | |
| // Fetch all relevant perpetuals. | |
| perpetuals := make(map[uint32]perptypes.Perpetual, len(subaccount.PerpetualPositions)) | |
| for _, p := range subaccount.PerpetualPositions { | |
| perpetual, err := k.perpetualsKeeper.GetPerpetual(ctx, p.PerpetualId) | |
| if err != nil { | |
| return types.Subaccount{}, nil, err | |
| } | |
| perpetuals[p.PerpetualId] = perpetual | |
| settledUpdates, _, err := k.getSettledUpdates(ctx, updates, perpInfos, false) | |
| if err != nil { | |
| return false, nil, err | |
| } | |
| return GetSettledSubaccountWithPerpetuals(subaccount, perpetuals) | |
| success, successPerUpdate, err = k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType, perpInfos) | |
| return success, successPerUpdate, err | |
| perpInfos, err := k.GetAllRelevantPerpetuals(ctx, updates) | |
| if err != nil { | |
| return false, nil, errorsmod.Wrapf(err, "Failed to validate updates in CanUpdateSubaccounts") | |
| } | |
| settledUpdates, _, err := k.getSettledUpdates(ctx, updates, perpInfos, false) | |
| if err != nil { | |
| return false, nil, errorsmod.Wrapf(err, "Failed to validate updates in CanUpdateSubaccounts") | |
| } | |
| success, successPerUpdate, err = k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType, perpInfos) | |
| return success, successPerUpdate, err |
| perpInfos map[uint32]perptypes.PerpInfo, | ||
| ) ( | ||
| success bool, | ||
| successPerUpdate []types.UpdateResult, | ||
| err error, | ||
| ) { | ||
| success = true | ||
| successPerUpdate = make([]types.UpdateResult, len(settledUpdates)) | ||
| perpIdToMarketType := getPerpIdToMarketTypeMap(perpetuals) | ||
|
|
||
| for i, u := range settledUpdates { | ||
| result, err := isValidIsolatedPerpetualUpdates(u, perpIdToMarketType) | ||
| result, err := isValidIsolatedPerpetualUpdates(u, perpInfos) |
There was a problem hiding this comment.
Optimize the retrieval of perpetual information.
Consider optimizing the retrieval and processing of perpetual information in these functions to reduce the complexity and potential overhead associated with multiple map lookups. This could involve caching results or restructuring the data accesses to minimize redundancy.
Also applies to: 63-81, 145-172
| @@ -25,18 +24,17 @@ | |||
| func (k Keeper) checkIsolatedSubaccountConstraints( | |||
There was a problem hiding this comment.
Define missing types and correct scope issues.
+ type Keeper struct {
+ // define necessary fields and methods
+ }The Keeper type and various instances of SettledUpdate are referenced but not defined within this file or imported packages. Define these types or ensure they're properly imported to resolve the typecheck errors.
Also applies to: 26-26, 62-62, 144-144, 235-235, 318-318
Tools
golangci-lint
24-24: undefined: Keeper (typecheck)
BrendanChou
force-pushed
the
bc/collat-relevant-perpetuals
branch
from
424012b to
e578f94
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/daemons/liquidation/client/sub_task_runner.go (8 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (9 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (19 hunks)
- protocol/x/subaccounts/keeper/subaccount_helper.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/isolated_subaccount_test.go
Additional context used
golangci-lint
protocol/x/subaccounts/keeper/subaccount_helper.go
17-17: undefined: SettledUpdate (typecheck)
58-58: undefined: SettledUpdate (typecheck)
107-107: undefined: SettledUpdate (typecheck)
protocol/daemons/liquidation/client/sub_task_runner.go
28-28: undefined: Client (typecheck)
42-42: undefined: Client (typecheck)
104-104: undefined: Client (typecheck)
121-121: undefined: newContextWithQueryBlockHeight (typecheck)
protocol/x/subaccounts/keeper/isolated_subaccount.go
24-24: undefined: Keeper (typecheck)
26-26: undefined: SettledUpdate (typecheck)
62-62: undefined: SettledUpdate (typecheck)
144-144: undefined: SettledUpdate (typecheck)
235-235: undefined: Keeper (typecheck)
318-318: undefined: Keeper (typecheck)
protocol/x/subaccounts/keeper/subaccount.go
33-33: undefined: Keeper (typecheck)
61-61: undefined: Keeper (typecheck)
69-69: undefined: Keeper (typecheck)
217-217: undefined: SettledUpdate (typecheck)
551-551: undefined: SettledUpdate (typecheck)
879-879: undefined: SettledUpdate (typecheck)
311-311: undefined: GetDeltaOpenInterestFromUpdates (typecheck)
330-330: undefined: UpdatePerpetualPositions (typecheck)
336-336: undefined: UpdateAssetPositions (typecheck)
366-366: undefined: getUpdatedPerpetualPositions (typecheck)
Additional comments not posted (7)
protocol/x/subaccounts/keeper/subaccount_helper.go (1)
Line range hint
108-145: Refactor to useperpInfosmap for perpetual position updates.The refactoring to use the
perpInfosmap inUpdatePerpetualPositionsaligns with the PR's objective to optimize state reads. This should make the process more efficient by reducing the need to repeatedly fetch perpetual information.protocol/daemons/liquidation/client/sub_task_runner.go (2)
Line range hint
59-74: Refactor to useperpInfosmap in liquidation checks.The use of the
perpInfosmap inRunLiquidationDaemonTaskLoopfor liquidation checks aligns well with the PR's objective to optimize state reads. This should enhance performance by reducing the number of state reads required during the process.
Line range hint
110-178: Optimize state reads by usingperpInfosmap.The changes in
FetchApplicationStateAtBlockHeightto use theperpInfosmap for consolidating perpetual information are in line with the PR's objectives. This should effectively reduce the complexity and the number of state reads.protocol/x/subaccounts/keeper/isolated_subaccount.go (4)
27-37: Refactor to useperpInfosmap in checking isolated subaccount constraints.The refactoring to use the
perpInfosmap incheckIsolatedSubaccountConstraintsis consistent with the PR's goal to optimize state reads by centralizing perpetual information.
Line range hint
63-100: Optimize the retrieval of perpetual information in state transition calculations.The changes in
isValidIsolatedPerpetualUpdatesto use theperpInfosmap streamline the process and reduce the complexity associated with multiple map lookups, aligning with the PR's objectives.
Line range hint
145-172: Optimize the retrieval of perpetual information.The refactoring in
GetIsolatedPerpetualStateTransitionto use theperpInfosmap reduces the overhead and complexity of accessing perpetual information multiple times, which is in line with the PR's objectives.
321-327: UseperpInfosmap to compute state transitions for isolated perpetuals.The changes in
computeAndExecuteCollateralTransferto utilize theperpInfosmap for computing state transitions enhance the efficiency of the process, aligning with the PR's goal to optimize state reads.
| @@ -211,6 +211,7 @@ func (k Keeper) getRandomBytes(ctx sdk.Context, rand *rand.Rand) ([]byte, error) | |||
| func (k Keeper) getSettledUpdates( | |||
| ctx sdk.Context, | |||
| updates []types.Update, | |||
| perpInfos map[uint32]perptypes.PerpInfo, | |||
There was a problem hiding this comment.
Refactor the error handling for better clarity and maintainability.
- if err != nil {
- return nil, nil, err
+ if err != nil {
+ return settledUpdates, subaccountIdToFundingPayments, fmt.Errorf("failed to get settled subaccount with perpetuals: %w", err)
}This change clarifies the specific failure point and adheres to best practices for error handling by wrapping the error to provide more context.
Also applies to: 238-238
| perpInfos, err := k.GetAllRelevantPerpetuals(ctx, updates) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } |
There was a problem hiding this comment.
Optimize error handling and ensure consistent error propagation.
- if err != nil {
- return false, nil, err
+ if err != nil {
+ return false, successPerUpdate, fmt.Errorf("failed to get all relevant perpetuals: %w", err)
}
- if err != nil {
- return false, nil, err
+ if err != nil {
+ return success, successPerUpdate, fmt.Errorf("failed to get settled updates: %w", err)
}
- if err != nil {
- return false, nil, err
+ if err != nil {
+ return false, successPerUpdatedSubaccount, fmt.Errorf("failed to execute collateral transfer: %w", err)
}These changes improve the error messages by adding more descriptive texts, which helps in debugging and maintaining the code.
Also applies to: 294-294, 303-303, 346-346
| @@ -465,7 +443,7 @@ | |||
| // Note that this is a stateless utility function. | |||
| func GetSettledSubaccountWithPerpetuals( | |||
| subaccount types.Subaccount, | |||
| perpetuals map[uint32]perptypes.Perpetual, | |||
| perpInfos map[uint32]perptypes.PerpInfo, | |||
There was a problem hiding this comment.
Enhance error handling in GetSettledSubaccountWithPerpetuals for better clarity.
- if !found {
- return types.Subaccount{}, nil, errorsmod.Wrapf(types.ErrPerpetualInfoDoesNotExist, "%d", p.PerpetualId)
+ if !found {
+ return settledSubaccount, fundingPayments, fmt.Errorf("perpetual info does not exist for ID %d: %w", p.PerpetualId, types.ErrPerpetualInfoDoesNotExist)
}Refactoring the error handling to include more specific information about the missing perpetual helps in troubleshooting and maintaining the code.
Also applies to: 461-461
| return big.NewInt(0), big.NewInt(0), big.NewInt(0), err | ||
| perpInfo, found := perpInfos[size.GetId()] | ||
| if !found { | ||
| return big.NewInt(0), big.NewInt(0), big.NewInt(0), |
There was a problem hiding this comment.
Handle missing perpetual information more gracefully.
- return big.NewInt(0), big.NewInt(0), big.NewInt(0), errorsmod.Wrapf(types.ErrPerpetualInfoDoesNotExist, "%d", size.GetId())
+ return bigNetCollateral, bigInitialMargin, bigMaintenanceMargin, fmt.Errorf("perpetual info does not exist for ID %d: %w", size.GetId(), types.ErrPerpetualInfoDoesNotExist)This change ensures that the function returns the current state of the collateral and margins even if an error occurs, which can be crucial for error recovery and handling in calling functions.
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.
| return big.NewInt(0), big.NewInt(0), big.NewInt(0), | |
| return bigNetCollateral, bigInitialMargin, bigMaintenanceMargin, fmt.Errorf("perpetual info does not exist for ID %d: %w", size.GetId(), types.ErrPerpetualInfoDoesNotExist) |
| @@ -103,7 +105,7 @@ func getUpdatedPerpetualPositions( | |||
| // For newly created positions, use `perpIdToFundingIndex` map to populate the `FundingIndex` field. | |||
| func UpdatePerpetualPositions( | |||
| settledUpdates []SettledUpdate, | |||
There was a problem hiding this comment.
Define or import SettledUpdate to resolve type errors.
Please ensure that the SettledUpdate type is correctly defined or imported in this file to resolve the typecheck errors indicated by the static analysis tool.
Tools
golangci-lint
107-107: undefined: SettledUpdate (typecheck)
…g collateral checks (backport #1691) (#1806) Co-authored-by: Brendan Chou <[email protected]> Co-authored-by: Teddy Ding <[email protected]>
Changelist
Test Plan
Blocked On
Superceeds
Summary by CodeRabbit
Refactor
Error Handling
New Features