fix: fees does not require additional funds #1718
Conversation
|
Warning Rate Limit Exceeded@Unique-Divine has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 7 minutes and 57 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. 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. WalkthroughThe changes involve a significant overhaul of fee calculations, margin handling, and assertions in tests for a cryptocurrency trading platform. New computations for fees and notional values have been introduced, and existing logic has been refined. The codebase has also been updated to include a new function for donations to the ecosystem fund, and comments have been improved for clarity. Changes
Assessment against linked issues
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 X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (12)
- wasmbinding/querier_perp_test.go (1 hunks)
- x/perp/v2/client/cli/cli_test.go (12 hunks)
- x/perp/v2/keeper/admin_test.go (1 hunks)
- x/perp/v2/keeper/amm.go (3 hunks)
- x/perp/v2/keeper/clearing_house.go (10 hunks)
- x/perp/v2/keeper/clearing_house_test.go (42 hunks)
- x/perp/v2/keeper/margin_test.go (12 hunks)
- x/perp/v2/keeper/msg_server.go (2 hunks)
- x/perp/v2/keeper/position.go (1 hunks)
- x/perp/v2/keeper/settlement.go (2 hunks)
- x/perp/v2/keeper/settlement_test.go (2 hunks)
- x/perp/v2/types/amm.go (2 hunks)
Files not summarized due to errors (1)
- x/perp/v2/keeper/clearing_house_test.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- x/perp/v2/types/amm.go
Additional comments: 80
wasmbinding/querier_perp_test.go (1)
- 284-297: The test
TestPositionhas been updated to include new logic for fee calculation and assertion. The changes seem to align with the PR's objective of fixing fee handling. The assertions now include a new fee calculation based on themarketanddefaultFee, and the expected exchanged notional value is adjusted accordingly.However, it's crucial to ensure that the new fee calculation logic is consistent with the intended fee handling changes in the rest of the codebase. The test should accurately reflect the new fee logic to ensure that it's correctly implemented.
x/perp/v2/client/cli/cli_test.go (6)
247-260: The assertions in
TestMarketOrdersAndCloseCmdhave been updated with new decimal values. Ensure these values correctly reflect the intended fee handling logic changes.281-286: The assertions in
TestMarketOrdersAndCloseCmdfor a different scenario have also been updated. Verify that the new values are consistent with the expected behavior after the fee handling fix.305-321: The assertions for market reserves and trader positions have been modified. Confirm that these changes are in line with the new fee handling logic and that the test still accurately reflects the system's behavior.
341-347: The test case checks for margin ratio within a delta. Given the changes to fee handling, ensure that this delta is still appropriate and that the test remains reliable.
387-401: The assertions in
TestPartialCloseCmdhave been updated. It's important to validate that these new expected values are correct and that the test cases still serve their purpose after the fee handling changes.433-447: The updates to the assertions in
TestPartialCloseCmdfor a different test scenario should be checked for accuracy and relevance to the changes in fee handling.x/perp/v2/keeper/admin_test.go (1)
- 269-269: The change in the expected settlement price in the test case aligns with the PR's objective to fix fee handling. Ensure that this new expected value has been verified against the updated business logic to confirm its correctness.
x/perp/v2/keeper/amm.go (5)
38-38: The invocation of
handleMarketUpdateCostinEditPriceMultiplierhas been updated to match the new function signature.66-66: The invocation of
handleMarketUpdateCostinEditSwapInvarianthas been updated to match the new function signature.81-84: The
handleMarketUpdateCostfunction has been updated to remove thepairparameter, and the code correctly handles positive and negativecostAmtvalues.81-84: The handling of
costAmtinhandleMarketUpdateCostimplicitly covers the zero case by not performing any action, which is acceptable.81-84: The comment within the negative cost branch of
handleMarketUpdateCostexplains the business logic decision to proceed with the repeg even if there's no money to cover the cost.x/perp/v2/keeper/clearing_house.go (10)
67-74: The logic for fee transfer and subsequent adjustment of
quoteAssetAmttoquoteAssetAmtMinusFeesseems correct and aligns with the PR's objective to handle fees properly from the position.85-85: The
increasePositionfunction call correctly usesquoteAssetAmtMinusFeesinstead of the originalquoteAssetAmt, ensuring that fees are taken into account when increasing a position.92-92: The
openReversePositionfunction call correctly usesquoteAssetAmtMinusFeesconverted tosdk.Decfor thequoteAssetAmountparameter, which is consistent with the fee handling changes.120-120: The
afterPositionUpdatefunction has been updated to include a new parametertransferredFee. This change should be cross-verified with all calls to this function to ensure they pass the correct argument.Verification successful
The verification process has confirmed that all calls to the
afterPositionUpdatefunction have been updated to include the newtransferredFeeparameter. The calls are correctly passing the appropriate arguments in each context where the function is invoked.* 546-546: The addition of the `transferredFee` parameter in the `afterPositionUpdate` function signature is consistent with the changes made to handle fees correctly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to `afterPositionUpdate` to ensure they include the new `transferredFee` parameter. ast-grep --lang go --pattern $'k.afterPositionUpdate($_, $_, $_, $_, $_, $_, $_)'Length of output: 1887
585-592: The conditional logic to handle fee transfer when the position is closed entirely seems correct. It ensures that fees are taken from the margin if the position has margin to be transferred to the user.
734-734: The
closePositionEntirelyfunction call does not include theupdatedAMMparameter anymore, which is consistent with the changes mentioned in the AI-generated summary.761-761: The
afterPositionUpdatefunction is called withsdk.ZeroInt()for thetransferredFeeparameter when closing a position, which seems appropriate as the position is being closed entirely and there would be no fee transfer needed.905-910: The
transferFeefunction is called before decreasing the position size inPartialClose, and the resulting fee is subtracted from the notional amount, which aligns with the PR's objective to handle fees correctly.933-933: The
afterPositionUpdatefunction is called with thefeesTransferredparameter inPartialClose, which is consistent with the new function signature and the PR's objective.x/perp/v2/keeper/clearing_house_test.go (30)
73-89: The test case "new long position" seems to be asserting various aspects of a market order. Ensure that the expected values in the assertions match the intended outcomes of the market order operation.
141-154: The test case "existing long position, go more long" is asserting the state after a second market order. Verify that the expected values for margin, open notional, and size are correctly calculated based on the market conditions and the effects of the second order.
291-305: In the test case "existing long position, decrease a bit", the assertions should reflect the expected state after a market order that reduces the position size. Check that the expected values for margin, open notional, exchanged position size, and position notional are consistent with the market dynamics and the order details.
385-399: The test case "existing long position, decrease a lot" is asserting the state after a significant reduction in position size. Ensure that the expected values for margin, open notional, exchanged position size, and position notional accurately reflect the impact of such a large order on the position.
436-439: The test case "existing long position, decrease a lot but there's bad debt" includes a setup where the account is funded with an amount that seems inconsistent with the position size and margin. Verify that the funding amount is appropriate for the test scenario and that the assertions account for the presence of bad debt.
506-519: The test case "new short position" asserts the state after opening a new short position. Confirm that the expected values for margin, open notional, and size are correct and that the assertions for exchanged notional value and position size are consistent with the expected outcomes of the market order.
573-587: In the test case "existing short position, go more short", the assertions should reflect the expected state after an additional market order that increases the short position size. Ensure that the expected values for margin, open notional, exchanged position size, and position notional are correct based on the market conditions and the effects of the additional order.
665-679: The test case "existing short position, decrease a bit" is asserting the state after a market order that reduces the short position size. Verify that the expected values for margin, open notional, exchanged position size, and position notional are consistent with the market dynamics and the order details.
758-772: In the test case "existing short position, decrease a lot", the assertions should reflect the expected state after a significant reduction in the short position size. Check that the expected values for margin, open notional, exchanged position size, and position notional accurately reflect the impact of such a large order on the position.
808-811: The test case "existing short position, decrease a lot but there's bad debt" includes a setup where the account is funded with an amount that seems inconsistent with the position size and margin. Verify that the funding amount is appropriate for the test scenario and that the assertions account for the presence of bad debt.
1111-1114: The test case "partial close long position with positive PnL" involves partially closing a long position and asserts the expected state of the position after the partial close. Ensure that the expected values for margin, open notional, and size are correct and that the assertions for realized PnL and exchanged notional are consistent with the expected outcomes of the partial close operation.
1182-1207: In the test case "partial close long position with negative PnL", the assertions should reflect the expected state after a partial close that results in a negative profit and loss. Confirm that the expected values for margin, open notional, and size are correct and that the assertions for realized PnL and exchanged notional are consistent with the market conditions and the partial close details.
1237-1262: The test case "partial close long position without bad debt but below maintenance margin ratio" asserts the state after a partial close that does not result in bad debt but leaves the position below the maintenance margin ratio. Verify that the expected values for margin, open notional, and size are correct and that the assertions for realized PnL and exchanged notional are consistent with the expected outcomes of the partial close operation.
1300-1320: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1293-1317]
In the test case "partial close long position with bad debt", the assertions should reflect the expected state after a partial close that results in bad debt. Ensure that the expected values for margin, open notional, and size are correct and that the assertions for realized PnL, bad debt, and exchanged notional are consistent with the market conditions and the partial close details.
1346-1371: The test case "partial close short position with positive PnL" involves partially closing a short position and asserts the expected state of the position after the partial close. Confirm that the expected values for margin, open notional, and size are correct and that the assertions for realized PnL and exchanged notional are consistent with the expected outcomes of the partial close operation.
1401-1426: In the test case "partial close short position with negative PnL", the assertions should reflect the expected state after a partial close that results in a negative profit and loss. Verify that the expected values for margin, open notional, and size are correct and that the assertions for realized PnL and exchanged notional are consistent with the market conditions and the partial close details.
1456-1481: The test case "partial close short position with no bad debt but below maintenance margin ratio" asserts the state after a partial close that does not result in bad debt but leaves the position below the maintenance margin ratio. Ensure that the expected values for margin, open notional, and size are correct and that the assertions for realized PnL and exchanged notional are consistent with the expected outcomes of the partial close operation.
1520-1540: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1513-1537]
In the test case "partial close short position with bad debt", the assertions should reflect the expected state after a partial close that results in bad debt. Confirm that the expected values for margin, open notional, and size are correct and that the assertions for realized PnL, bad debt, and exchanged notional are consistent with the market conditions and the partial close details.
1605-1613: The test case "close long position with positive PnL" asserts the state after closing a long position with a positive profit and loss. Verify that the assertions for realized PnL and exchanged notional are consistent with the expected outcomes of the close position operation.
1652-1660: In the test case "close long position with negative PnL", the assertions should reflect the expected state after closing a long position with a negative profit and loss. Ensure that the assertions for realized PnL and exchanged notional are consistent with the market conditions and the close position details.
1701-1712: The test case "close long position with bad debt" involves closing a long position that results in bad debt. Confirm that the assertions for realized PnL, bad debt, and exchanged notional are correct and that the module balances are accurately updated to reflect the bad debt.
1750-1758: The test case "close short position with positive PnL" asserts the state after closing a short position with a positive profit and loss. Verify that the assertions for realized PnL and exchanged notional are consistent with the expected outcomes of the close position operation.
1797-1805: In the test case "close short position with negative PnL", the assertions should reflect the expected state after closing a short position with a negative profit and loss. Ensure that the assertions for realized PnL and exchanged notional are consistent with the market conditions and the close position details.
1846-1857: The test case "close short position with bad debt" involves closing a short position that results in bad debt. Confirm that the assertions for realized PnL, bad debt, and exchanged notional are correct and that the module balances are accurately updated to reflect the bad debt.
1913-1921: The test case "only long position - increasing k" asserts the state after editing the swap invariant and closing the position. Ensure that the expected swap invariant value after the edit is correct and that the module balances are accurately updated to reflect the changes.
1933-1939: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1933-1945]
In the test case "only short position - increasing k", the assertions should reflect the state after editing the swap invariant and closing the position. Verify that the expected swap invariant value after the edit is correct and that the module balances are accurately updated to reflect the changes.
- 1954-1960: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1954-1965]
The test case "only long position - decreasing k" asserts the state after editing the swap invariant and closing the position. Confirm that the expected swap invariant value after the edit is correct and that the module balances are accurately updated to reflect the changes.
- 1974-1980: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1974-1985]
In the test case "only short position - decreasing k", the assertions should reflect the state after editing the swap invariant and closing the position. Ensure that the expected swap invariant value after the edit is correct and that the module balances are accurately updated to reflect the changes.
- 2000-2006: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [2000-2014]
The test case "long and short position - increasing k" involves editing the swap invariant with both long and short positions open. Verify that the expected bias and swap invariant values after the edit are correct and that the module balances are accurately updated to reflect the changes.
- 2031-2037: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [2031-2045]
In the test case "long and short position - reducing k", the assertions should reflect the state after editing the swap invariant with both long and short positions open. Confirm that the expected bias and swap invariant values after the edit are correct and that the module balances are accurately updated to reflect the changes.
x/perp/v2/keeper/margin_test.go (16)
33-33: Ensure that the
FundAccountfunction is correctly simulating the funding of Alice's account with the expected amount of collateral for the tests to be valid.45-47: The updated
Size_,Margin, andOpenNotionalvalues in thePositionShouldBeEqualassertion need to be verified to ensure they reflect the correct state of the position after fees are taken into account.62-62: The
PositionNotionalvalue in thePositionChangedEventShouldBeEqualassertion should be consistent with the updated position state, including the impact of fees.83-83: Similar to the previous comment, verify that the
FundAccountfunction is correctly simulating the funding of Alice's account with the expected amount of collateral for the tests to be valid.95-97: Again, verify that the updated
Size_,Margin, andOpenNotionalvalues in thePositionShouldBeEqualassertion are correct and reflect the expected state of the position after the fee handling fix.112-112: Ensure that the
PositionNotionalvalue in thePositionChangedEventShouldBeEqualassertion is consistent with the updated position state, including the impact of fees.165-165: Confirm that the
FundAccountfunction is correctly simulating the funding of Alice's account with the expected amount of collateral for the tests to be valid.176-178: Verify that the updated
Size_,Margin, andOpenNotionalvalues in thePositionShouldBeEqualassertion are correct and reflect the expected state of the position after the fee handling fix.192-192: Ensure that the
PositionNotionalvalue in thePositionChangedEventShouldBeEqualassertion is consistent with the updated position state, including the impact of fees.213-213: Confirm that the
FundAccountfunction is correctly simulating the funding of Alice's account with the expected amount of collateral for the tests to be valid.224-226: Verify that the updated
Size_,Margin, andOpenNotionalvalues in thePositionShouldBeEqualassertion are correct and reflect the expected state of the position after the fee handling fix.240-240: Confirm that the
FundAccountfunction is correctly simulating the funding of Alice's account with the expected amount of collateral for the tests to be valid.251-253: Verify that the updated
Size_,Margin, andOpenNotionalvalues in thePositionShouldBeEqualassertion are correct and reflect the expected state of the position after the fee handling fix.267-267: Ensure that the
PositionNotionalvalue in thePositionChangedEventShouldBeEqualassertion is consistent with the updated position state, including the impact of fees.288-288: Confirm that the
FundAccountfunction is correctly simulating the funding of Alice's account with the expected amount of collateral for the tests to be valid.299-301: Verify that the updated
Size_,Margin, andOpenNotionalvalues in thePositionShouldBeEqualassertion are correct and reflect the expected state of the position after the fee handling fix.x/perp/v2/keeper/msg_server.go (2)
130-133:
The implementation ofDonateToEcosystemFundappears correct. It uses theBankKeeperto transfer coins from the sender's account to the module account, which is a standard pattern in Cosmos SDK modules for handling token transfers.144-145:
The updated comment forChangeCollateralDenomis a minor stylistic change that improves readability and consistency in the codebase.x/perp/v2/keeper/position.go (1)
- 12-12: The addition of the comment to the
GetPositionfunction is a good practice for code documentation and clarity.x/perp/v2/keeper/settlement.go (2)
- 45-53: The addition of
sdk.ZeroInt()as an argument to theafterPositionUpdatefunction call is consistent with the PR's objective to fix fee handling. Ensure that theafterPositionUpdatefunction has been updated to accept this new argument and that the logic within handles it correctly.Verification successful
The previous shell script was intended to verify if the
afterPositionUpdatefunction in the Go codebase had been updated to accept a newsdk.Intargument, as suggested by the changes in the PR. However, the script did not produce any output, which means that either the function does not exist with the expected signature, or the pattern used inast-grepdid not match the actual function signature in the codebase.To resolve this, I will adjust the
ast-greppattern to be less specific about the number of arguments before thesdk.Inttype. This will help to catch the function regardless of how many arguments it has, as long as it ends with ansdk.Intargument.
The second attempt to verify the
afterPositionUpdatefunction signature usingast-grepalso did not produce any output. This could mean that the function signature does not match the pattern provided, or there may be an issue with theast-grepcommand itself.Given that the direct approach with
ast-grephas not yielded results, I will now attempt to use a different strategy to verify the function signature. I will userg(ripgrep) to search for the function definition in the codebase. This approach will allow us to search the raw text of the files without relying on the abstract syntax tree, which might give us the results we need.
The output from the
rgcommand indicates that theafterPositionUpdatefunction is defined in the filex/perp/v2/keeper/clearing_house.go. However, the output does not show the full function signature, which is necessary to confirm whether the function has been updated to include the newsdk.Intargument as per the PR changes.To get the full function signature, I will now generate a script that uses
rgto extract the lines around the function definition, which should include the full signature.
The output from the
rgcommand provides the beginning of theafterPositionUpdatefunction signature from thex/perp/v2/keeper/clearing_house.gofile, but it does not include the full signature, specifically the part where the newsdk.Intargument would be expected.To ensure we get the complete function signature, I will adjust the script to extract more lines following the function definition. This should provide us with the information needed to verify if the
sdk.Intargument has been added.
The output now provides the complete function signature for
afterPositionUpdatein thex/perp/v2/keeper/clearing_house.gofile. It confirms that the function has been updated to include a new argument of typesdkmath.Int, which is namedtransferredFee. This matches the expected change from the PR, wheresdk.ZeroInt()was added as an argument in the function call within theSettlePositionfunction.With this confirmation, we can conclude that the
afterPositionUpdatefunction has been correctly updated to accept the new argument as part of the PR's changes to fix fee handling.The addition of
sdk.ZeroInt()as an argument to theafterPositionUpdatefunction call is consistent with the PR's objective to fix fee handling, and the function signature has been verified to accept this new argument correctly.* 27-33: The provided hunk still shows the `updatedAMM` variable being returned from the `settlePosition` function. This contradicts the AI-generated summary, which mentioned its removal. Verify if `updatedAMM` is still required and used correctly within the `SettlePosition` function and the broader codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `afterPositionUpdate` function has been updated to accept the new argument. ast-grep --lang go --pattern $'func (k Keeper) afterPositionUpdate($_, $_, $_, $_, $_, $_, sdk.Int) $$$'Length of output: 105
Script:
#!/bin/bash # Verify that the `afterPositionUpdate` function has been updated to accept the new argument. ast-grep --lang go --pattern $'func (k Keeper) afterPositionUpdate($$$, sdk.Int) $$$'Length of output: 86
Script:
#!/bin/bash # Use ripgrep to search for the 'afterPositionUpdate' function definition. rg "func \(k Keeper\) afterPositionUpdate"Length of output: 116
Script:
#!/bin/bash # Extract the function signature and a few lines of context for 'afterPositionUpdate'. rg -C 3 "func \(k Keeper\) afterPositionUpdate" x/perp/v2/keeper/clearing_house.goLength of output: 259
Script:
#!/bin/bash # Extract more lines following the function definition to get the complete signature. rg -A 10 "func \(k Keeper\) afterPositionUpdate" x/perp/v2/keeper/clearing_house.goLength of output: 421
Verification successful
The verification process has confirmed that the
updatedAMMvariable has been removed from theSettlePositionfunction inx/perp/v2/keeper/settlement.go, as stated in the AI-generated summary. The variable is still present and used in thesettlePositionfunction and elsewhere in the codebase, which is consistent with the code's logic and does not indicate an issue. The changes to theSettlePositionfunction are verified as correct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of `updatedAMM` within the `SettlePosition` function and the broader codebase. ast-grep --lang go --pattern $'func (k Keeper) SettlePosition($_, $_, $_, $_) $$$' # Check for references to `updatedAMM` in the broader codebase. rg --type go 'updatedAMM'Length of output: 6302
x/perp/v2/keeper/settlement_test.go (6)
79-79: The updated margin ratio in the test case should reflect the new logic for fee handling. Verify that the new value
-0.090096188479034643is consistent with the expected outcome based on the changes made to fee computation.103-103: The
MarginToVaultvalue has been changed to-1.009493031710765502. Ensure that this new value is correctly calculated based on the updated fee handling logic and that it represents the expected state after settling the position.104-104: The
BadDebtvalue has been updated tosdk.ZeroDec(). Confirm that this change accurately reflects the scenario being tested, particularly if the test case is meant to simulate a situation where bad debt is expected to occur.110-110: The
MarginToVaultvalue for Bob's settlement has been updated. Make sure that the new value-1076.990506968289234498is the correct expected value after the changes to the fee handling logic.117-117: Alice's balance is expected to be
sdk.NewInt(5)after settlement. Validate that this balance is the correct expected result based on the updated logic for fee deductions from the position.118-118: Bob's balance is expected to be
sdk.NewInt(1095)after settlement. Confirm that this expected balance is consistent with the new fee handling logic and represents the correct state of Bob's account after the position is settled.
x/perp/v2/keeper/amm.go
Outdated
| func (k Keeper) handleMarketUpdateCost(ctx sdk.Context, costAmt sdkmath.Int) (err error) { | ||
| collateral, err := k.Collateral.Get(ctx) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
The handleMarketUpdateCost function's error handling for insufficient funds is appropriate, but consider reviewing the use of nolint:staticcheck to ensure that it's justified and that no potential issues are being suppressed.
jgimeno
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- x/perp/v2/client/cli/cli_test.go (12 hunks)
- x/perp/v2/keeper/admin_test.go (1 hunks)
- x/perp/v2/keeper/clearing_house.go (10 hunks)
- x/perp/v2/keeper/clearing_house_test.go (42 hunks)
- x/perp/v2/keeper/msg_server.go (2 hunks)
- x/perp/v2/types/amm.go (2 hunks)
Files not summarized due to errors (1)
- x/perp/v2/keeper/clearing_house_test.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (4)
- x/perp/v2/client/cli/cli_test.go
- x/perp/v2/keeper/admin_test.go
- x/perp/v2/keeper/msg_server.go
- x/perp/v2/types/amm.go
Files skipped from review as they are similar to previous changes (1)
- x/perp/v2/keeper/clearing_house.go
Additional comments: 10
x/perp/v2/keeper/clearing_house_test.go (10)
76-89: The expected position values in the test case "new long position" seem to be hardcoded. Ensure that these values are consistent with the current system logic and that they will be valid even if underlying constants or logic change.
141-154: The test case "existing long position, go more long" includes assertions for the expected position after a market order. Verify that the expected values for
Margin,OpenNotional,Size_, and other position attributes are calculated based on the current system logic and that they are correct.291-305: In the test case "existing long position, decrease a bit", verify that the expected changes to the position after a market order are correct, especially the
OpenNotionalandSize_values, which are critical to the position's state.385-399: For the test case "existing long position, decrease a lot", ensure that the expected position values after the market order are consistent with the system's logic for handling large decreases in position size.
436-439: The test case "existing long position, decrease a lot but there's bad debt" includes funding an account with a specific amount. Verify that the amount funded is sufficient for the test scenario and that it reflects a realistic situation where bad debt could occur.
506-519: In the test case "new short position", verify that the expected position values are correct and that they reflect the system's logic for opening a new short position.
573-587: For the test case "existing short position, go more short", verify that the expected position values after the market order are correct, especially since going more short could have significant implications for the position's state.
665-679: In the test case "existing short position, decrease a bit", ensure that the expected position values after the market order are correct and reflect the system's logic for handling decreases in short positions.
758-772: For the test case "existing short position, decrease a lot", verify that the expected position values after the market order are correct, especially since large decreases in position size could have significant implications for the position's state.
808-811: The test case "existing short position, decrease a lot but there's bad debt" includes funding an account with a specific amount. Verify that the amount funded is sufficient for the test scenario and that it reflects a realistic situation where bad debt could occur.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1718 +/- ##
==========================================
- Coverage 73.96% 73.89% -0.08%
==========================================
Files 192 192
Lines 15128 15144 +16
==========================================
+ Hits 11190 11191 +1
- Misses 3285 3297 +12
- Partials 653 656 +3
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Purpose
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests
Chores