[OTE-378] Remove offchain updates from ProcessSingleMatch
#1618
Conversation
|
Warning Rate limit exceeded@teddyding has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 48 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 recent changes across multiple files in the protocol involve the removal of the Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
protocol/mocks/MemClobKeeper.go (1)
Line range hint
29-29: Fix method calls to match the updated interface.- _m.Called + // Correct method calls as per the updated interface or library version.Also applies to: 73-73, 108-108, 440-440
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- protocol/mocks/ClobKeeper.go (3 hunks)
- protocol/mocks/MemClobKeeper.go (3 hunks)
- protocol/testutil/memclob/keeper.go (3 hunks)
- protocol/x/clob/keeper/process_operations.go (2 hunks)
- protocol/x/clob/keeper/process_single_match.go (14 hunks)
- protocol/x/clob/memclob/memclob.go (1 hunks)
- protocol/x/clob/types/clob_keeper.go (1 hunks)
- protocol/x/clob/types/mem_clob_keeper.go (1 hunks)
Additional context used
golangci-lint
protocol/mocks/MemClobKeeper.go
29-29: _m.Called undefined (type *MemClobKeeper has no field or method Called)
73-73: _m.Called undefined (type *MemClobKeeper has no field or method Called)
108-108: _m.Called undefined (type *MemClobKeeper has no field or method Called)
440-440: mock.AssertExpectations undefined (type *MemClobKeeper has no field or method AssertExpectations)
Additional comments not posted (17)
protocol/x/clob/types/mem_clob_keeper.go (1)
Line range hint
14-24: LGTM! The removal ofoffchainUpdatesfromProcessSingleMatchsimplifies the interface and aligns with the PR's objectives.protocol/x/clob/types/clob_keeper.go (1)
Line range hint
14-24: LGTM! The removal ofoffchainUpdatesfromProcessSingleMatchsimplifies the interface and aligns with the PR's objectives.Verification successful
The verification confirms that the
offchainUpdatesparameter has been removed from allProcessSingleMatchmethod signatures across the codebase.
protocol/mocks/ClobKeeper.goprotocol/mocks/MemClobKeeper.goprotocol/testutil/memclob/keeper.goprotocol/x/clob/keeper/process_single_match.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all implementations of `ClobKeeper` have been updated. # Test: Search for the old function signature. Expect: No occurrences. rg --type go $'ProcessSingleMatch.*OffchainUpdates'Length of output: 52
Script:
#!/bin/bash # Description: Verify all implementations of `ClobKeeper` have been updated. # Test: Search for the `ProcessSingleMatch` method across the codebase. rg --type go 'ProcessSingleMatch'Length of output: 2259
Script:
#!/bin/bash # Description: Verify that the `offchainUpdates` parameter has been removed from all `ProcessSingleMatch` method signatures. # Test: Search for the `ProcessSingleMatch` method signatures in the identified files. rg --type go 'ProcessSingleMatch' protocol/x/clob/types/clob_keeper.go protocol/x/clob/types/mem_clob_keeper.go protocol/x/clob/memclob/memclob.go protocol/x/clob/keeper/process_single_match.go protocol/x/clob/keeper/process_operations.go protocol/testutil/memclob/keeper.go protocol/mocks/MemClobKeeper.go protocol/mocks/ClobKeeper.goLength of output: 2385
Script:
#!/bin/bash # Description: Verify that the `offchainUpdates` parameter has been removed from all `ProcessSingleMatch` method signatures. # Test: Search for the full method signatures of `ProcessSingleMatch` in the identified files. rg --type go 'func (.*) ProcessSingleMatch' protocol/x/clob/types/clob_keeper.go protocol/x/clob/types/mem_clob_keeper.go protocol/x/clob/memclob/memclob.go protocol/x/clob/keeper/process_single_match.go protocol/x/clob/keeper/process_operations.go protocol/testutil/memclob/keeper.go protocol/mocks/MemClobKeeper.go protocol/mocks/ClobKeeper.goLength of output: 942
protocol/mocks/MemClobKeeper.go (1)
Line range hint
313-351: LGTM! The update inProcessSingleMatchaligns with the interface changes.protocol/testutil/memclob/keeper.go (1)
Line range hint
366-413: LGTM! The update inProcessSingleMatchaligns with the interface changes and simplifies the method.protocol/x/clob/keeper/process_single_match.go (9)
90-90: Check for the existence ofClobPairbefore proceeding.This is a good practice as it ensures that operations do not proceed on non-existent entities, which could lead to undefined behaviors or errors.
107-107: Handle potential errors fromgetFillQuoteQuantumsgracefully.Proper error handling is crucial, especially in financial applications where precision and accuracy are paramount.
130-130: ValidatePerpetualIdretrieval to prevent processing on invalid data.This validation is essential for ensuring that the operations are performed on valid and existing perpetuals, which is critical for maintaining system integrity.
156-156: Properly handle errors fromvalidateMatchedLiquidation.It's important to handle such errors immediately as they can affect the financial calculations and the state of the system significantly.
189-189: Ensure that the new total fill amount does not exceed the order's base quantums.This check is crucial for maintaining the integrity of order fills and preventing overfilling, which could lead to financial discrepancies.
211-211: Verify the remaining quantums before updating the order fill amount.This is a necessary validation to ensure that orders are not overfilled, maintaining the accuracy and reliability of the trading system.
226-226: Handle errors frompersistMatchedOrderseffectively.Proper error handling in this context is critical as it involves updating financial states and transferring funds, which are sensitive operations.
237-237: Handle potential errors fromGetNetNotionalmethod.This is important for ensuring that the calculations related to liquidations are based on accurate and valid data.
289-289: Confirm successful execution ofProcessSingleMatch.Returning a success status is a good practice as it provides clear feedback on the operation's outcome, which is essential for robust error handling and system reliability.
protocol/x/clob/keeper/process_operations.go (2)
617-617: Ensure all calls toProcessSingleMatchare updated to the new signature.Verification successful
The search results indicate that there are multiple occurrences of
ProcessSingleMatchacross different files. We need to verify that all calls toProcessSingleMatchhave been updated to match the new signature. Here are the specific files and lines that need attention:
protocol/x/clob/types/clob_keeper.go: Contains a call toProcessSingleMatch.protocol/x/clob/types/mem_clob_keeper.go: Contains a call toProcessSingleMatch.protocol/x/clob/memclob/memclob.go: Contains calls toProcessSingleMatch.protocol/x/clob/keeper/process_operations.go: Contains calls toProcessSingleMatch.protocol/testutil/memclob/keeper.go: Contains a function definition forProcessSingleMatch.protocol/mocks/ClobKeeper.go: Contains a mock function definition forProcessSingleMatch.protocol/mocks/MemClobKeeper.go: Contains a mock function definition forProcessSingleMatch.We need to ensure that all these calls and definitions match the new signature without
OffchainUpdates.
The search results indicate that all occurrences of
ProcessSingleMatchacross the identified files have been reviewed. Here are the findings:
- protocol/x/clob/types/clob_keeper.go: The function call matches the new signature.
- protocol/x/clob/types/mem_clob_keeper.go: The function call matches the new signature.
- protocol/x/clob/memclob/memclob.go: The function calls match the new signature.
- protocol/x/clob/keeper/process_operations.go: The function calls match the new signature.
- protocol/testutil/memclob/keeper.go: The function definition matches the new signature.
- protocol/mocks/ClobKeeper.go: The mock function definition matches the new signature.
- protocol/mocks/MemClobKeeper.go: The mock function definition matches the new signature.
All calls and definitions of
ProcessSingleMatchhave been updated to match the new signature withoutOffchainUpdates.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ProcessSingleMatch` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go $'ProcessSingleMatch'Length of output: 2259
Script:
#!/bin/bash # Description: Verify all function calls to `ProcessSingleMatch` match the new signature. # Extract and review the function calls in the identified files. files=( "protocol/x/clob/types/clob_keeper.go" "protocol/x/clob/types/mem_clob_keeper.go" "protocol/x/clob/memclob/memclob.go" "protocol/x/clob/keeper/process_operations.go" "protocol/testutil/memclob/keeper.go" "protocol/mocks/ClobKeeper.go" "protocol/mocks/MemClobKeeper.go" ) for file in "${files[@]}"; do echo "File: $file" rg --no-filename 'ProcessSingleMatch' "$file" echo doneLength of output: 2810
Script:
#!/bin/bash # Description: Verify all function calls to `ProcessSingleMatch` match the new signature. # Extract and review the function calls in the identified files. files=( "protocol/x/clob/types/clob_keeper.go" "protocol/x/clob/types/mem_clob_keeper.go" "protocol/x/clob/memclob/memclob.go" "protocol/x/clob/keeper/process_operations.go" "protocol/testutil/memclob/keeper.go" "protocol/mocks/ClobKeeper.go" "protocol/mocks/MemClobKeeper.go" ) for file in "${files[@]}"; do echo "File: $file" rg --no-filename 'ProcessSingleMatch' "$file" -A 3 -B 3 echo doneLength of output: 4184
509-509: Ensure all calls toProcessSingleMatchare updated to the new signature.Verification successful
The search results indicate that there are multiple occurrences of
ProcessSingleMatchacross different files. However, the output does not provide enough detail to confirm whether all calls have been updated to the new signature. We need to inspect the specific lines whereProcessSingleMatchis called to ensure they match the new signature.Let's generate a script to extract and display the relevant lines from each file to verify the function calls.
The extracted lines confirm that all calls to
ProcessSingleMatchhave been updated to the new signature, which no longer includesOffchainUpdates. Here are the relevant locations:
protocol/x/clob/types/mem_clob_keeper.goprotocol/x/clob/types/clob_keeper.goprotocol/x/clob/memclob/memclob.goprotocol/x/clob/keeper/process_operations.goprotocol/testutil/memclob/keeper.goprotocol/mocks/MemClobKeeper.goprotocol/mocks/ClobKeeper.goAll these files show the correct usage of
ProcessSingleMatchwith the updated signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ProcessSingleMatch` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go $'ProcessSingleMatch'Length of output: 2259
Script:
#!/bin/bash # Description: Verify all function calls to `ProcessSingleMatch` match the new signature. # Test: Check each identified file for the correct function signature. # Check in protocol/x/clob/types/mem_clob_keeper.go echo "Checking protocol/x/clob/types/mem_clob_keeper.go" rg --type go 'ProcessSingleMatch\(' protocol/x/clob/types/mem_clob_keeper.go # Check in protocol/x/clob/types/clob_keeper.go echo "Checking protocol/x/clob/types/clob_keeper.go" rg --type go 'ProcessSingleMatch\(' protocol/x/clob/types/clob_keeper.go # Check in protocol/x/clob/memclob/memclob.go echo "Checking protocol/x/clob/memclob/memclob.go" rg --type go 'ProcessSingleMatch\(' protocol/x/clob/memclob/memclob.go # Check in protocol/x/clob/keeper/process_operations.go echo "Checking protocol/x/clob/keeper/process_operations.go" rg --type go 'ProcessSingleMatch\(' protocol/x/clob/keeper/process_operations.go # Check in protocol/testutil/memclob/keeper.go echo "Checking protocol/testutil/memclob/keeper.go" rg --type go 'ProcessSingleMatch\(' protocol/testutil/memclob/keeper.go # Check in protocol/mocks/MemClobKeeper.go echo "Checking protocol/mocks/MemClobKeeper.go" rg --type go 'ProcessSingleMatch\(' protocol/mocks/MemClobKeeper.go # Check in protocol/mocks/ClobKeeper.go echo "Checking protocol/mocks/ClobKeeper.go" rg --type go 'ProcessSingleMatch\(' protocol/mocks/ClobKeeper.goLength of output: 1977
Script:
#!/bin/bash # Description: Extract and display lines where `ProcessSingleMatch` is called to verify the new signature. # Extract lines from protocol/x/clob/types/mem_clob_keeper.go echo "Extracting from protocol/x/clob/types/mem_clob_keeper.go" rg --type go 'ProcessSingleMatch\(' protocol/x/clob/types/mem_clob_keeper.go -A 2 -B 2 # Extract lines from protocol/x/clob/types/clob_keeper.go echo "Extracting from protocol/x/clob/types/clob_keeper.go" rg --type go 'ProcessSingleMatch\(' protocol/x/clob/types/clob_keeper.go -A 2 -B 2 # Extract lines from protocol/x/clob/memclob/memclob.go echo "Extracting from protocol/x/clob/memclob/memclob.go" rg --type go 'ProcessSingleMatch\(' protocol/x/clob/memclob/memclob.go -A 2 -B 2 # Extract lines from protocol/x/clob/keeper/process_operations.go echo "Extracting from protocol/x/clob/keeper/process_operations.go" rg --type go 'ProcessSingleMatch\(' protocol/x/clob/keeper/process_operations.go -A 2 -B 2 # Extract lines from protocol/testutil/memclob/keeper.go echo "Extracting from protocol/testutil/memclob/keeper.go" rg --type go 'ProcessSingleMatch\(' protocol/testutil/memclob/keeper.go -A 2 -B 2 # Extract lines from protocol/mocks/MemClobKeeper.go echo "Extracting from protocol/mocks/MemClobKeeper.go" rg --type go 'ProcessSingleMatch\(' protocol/mocks/MemClobKeeper.go -A 2 -B 2 # Extract lines from protocol/mocks/ClobKeeper.go echo "Extracting from protocol/mocks/ClobKeeper.go" rg --type go 'ProcessSingleMatch\(' protocol/mocks/ClobKeeper.go -A 2 -B 2Length of output: 3146
protocol/mocks/ClobKeeper.go (1)
Line range hint
1022-1060: The changes to theProcessSingleMatchfunction align with the PR's objectives to removeOffchainUpdates. This simplifies the function signature and avoids potential issues with unsafe concurrent map reads. Ensure that all dependent code correctly adapts to this new function signature.protocol/x/clob/memclob/memclob.go (1)
1683-1683: Verify the removal ofOffchainUpdatesacross all affected functions.
| @@ -275,24 +271,22 @@ func (k Keeper) ProcessSingleMatch( | |||
| // Liquidation orders can only be placed when a subaccount is liquidatable | |||
| // and cannot be replayed, therefore we don't need to track their filled amount in state. | |||
| if !matchWithOrders.TakerOrder.IsLiquidation() { | |||
| takerOffchainUpdates := k.setOrderFillAmountsAndPruning( | |||
| k.setOrderFillAmountsAndPruning( | |||
There was a problem hiding this comment.
Review the logic for setting order fill amounts and pruning.
Consider abstracting some of this functionality into separate, smaller functions to improve readability and maintainability of the code.
Also applies to: 282-282
| @@ -74,14 +72,13 @@ | |||
|
|
|||
| // Perform stateless validation on the match. | |||
| if err := matchWithOrders.Validate(); err != nil { | |||
| return false, takerUpdateResult, makerUpdateResult, nil, errorsmod.Wrapf( | |||
| return false, takerUpdateResult, makerUpdateResult, errorsmod.Wrapf( | |||
There was a problem hiding this comment.
Ensure proper error wrapping for better error traceability.
Consider adding more context to the error message to aid in debugging. For example, include specific attributes of MatchWithOrders that led to the validation failure.
| @@ -1680,7 +1680,7 @@ func (m *MemClobPriceTimePriority) mustPerformTakerOrderMatching( | |||
| FillAmount: matchedAmount, | |||
| } | |||
|
|
|||
| success, takerUpdateResult, makerUpdateResult, _, err := m.clobKeeper.ProcessSingleMatch(ctx, &matchWithOrders) | |||
| success, takerUpdateResult, makerUpdateResult, err := m.clobKeeper.ProcessSingleMatch(ctx, &matchWithOrders) | |||
There was a problem hiding this comment.
Refactor mustUpdateMemclobStateWithMatches to improve readability and maintainability.
This function is complex and performs multiple operations on the memclob state. Consider breaking it down into smaller, more manageable functions or methods. This can improve readability and make the code easier to maintain and test.
Changelist
Remove
OffchainUpdatesas a return value fromProcessSingleMatch. This field is not currently used anywhere. Populating this field requires read access to MemClob, which would result in unsafe concurrent map read duringDeliverTxif OE is enabled on indexer full node.Test Plan
Existing tests
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
Refactor
ProcessSingleMatchfunction across multiple modules by removing theoffchainUpdatesparameter and return type.Bug Fixes
ProcessSingleMatchfunction, enhancing stability and reliability.Performance Improvements