-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Release alpha #319
base: beta
Are you sure you want to change the base?
Release alpha #319
Conversation
- With support for slider and incremental IMF
feat: calculate buy/sell panel estimate
WalkthroughThe recent changes encompass broad modifications, primarily to the backend calculations and testing mechanisms related to the trading order book. The updates enhance functions calculating buy/sell estimates, margin requirements, managing trade quantities, and upgrading test scripts for improved readability and accuracy. Additionally, a minor frontend change updates an index price field in the order book utility file. These adjustments enhance precision, maintainability, and user interface alignment with backend logic. Changes
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 (
|
## [4.0.0-alpha.8](v4.0.0-alpha.7...v4.0.0-alpha.8) (2024-05-23) ### Features * calculate buy/sell panel estimate ([51d31a2](51d31a2)) ### Bug Fixes * dust liquidation ws type ([372c2ca](372c2ca))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
Outside diff range and nitpick comments (6)
src/pipmath.ts (4)
6-8
: Add a brief docstring toabsBigInt
function to explain its purpose and usage.
10-12
: Add a brief docstring toarraySumBigInt
function to explain its purpose and usage.
32-36
: Add a brief description for each enum value inROUNDING
to clarify their specific rounding behaviors.
[!TIP]
Codebase VerificationThe
multiplyPips
function does not currently handle cases wherepipValue1
orpipValue2
are zero. This could lead to unexpected results, especially if zero values are not intended to be valid inputs.Locations in the codebase that need attention:
- Function Definition:
src/pipmath.ts
: ThemultiplyPips
function definition does not include error handling for zero values.- Function Usage:
src/orderbook/quantities.ts
: Multiple instances wheremultiplyPips
is used without checking for zero values.src/bridge/bridge.ts
: Usage ofmultiplyPips
without checking for zero values.Suggested Action:
Add error handling in the
multiplyPips
function to manage cases wherepipValue1
orpipValue2
are zero. This can prevent unexpected results and ensure the function behaves as intended.Analysis chain
Line range hint
88-98
: Add error handling for cases wherepipValue1
orpipValue2
are zero inmultiplyPips
to prevent unexpected results.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify handling of zero values in `multiplyPips`. # Test: Search for the function usage with zero values. Expect: Proper error handling. rg --type python $'multiplyPips'Length of output: 62
Script:
#!/bin/bash # Description: Verify handling of zero values in `multiplyPips`. # Search for the definition of the `multiplyPips` function. rg 'function multiplyPips' --type ts # Search for the usage of the `multiplyPips` function in TypeScript files. rg 'multiplyPips' --type tsLength of output: 770
Script:
#!/bin/bash # Description: Extract relevant lines to verify handling of zero values in `multiplyPips`. # Extract the definition of the `multiplyPips` function. rg 'export function multiplyPips' -A 10 src/pipmath.ts # Extract the usage of `multiplyPips` in `src/orderbook/quantities.ts`. rg 'multiplyPips' -A 3 src/orderbook/quantities.ts # Extract the usage of `multiplyPips` in `src/bridge/bridge.ts`. rg 'multiplyPips' -A 3 src/bridge/bridge.tsLength of output: 1667
src/orderbook/quantities.ts (2)
34-34
: Add a brief docstring toLeverageParametersBigInt
to explain its purpose and usage.
[!TIP]
Codebase VerificationThe test cases for
calculateGrossFillQuantities
cover various scenarios, including different order quantities and price limits. However, there is no explicit test for handling emptymakerSideOrders
or invalidtakerOrder
values.Please add error handling in
calculateGrossFillQuantities
to manage cases wheremakerSideOrders
is empty ortakerOrder
has invalid values, and ensure these cases are covered in the tests.
- File:
src/orderbook/quantities.ts
- Function:
calculateGrossFillQuantities
Analysis chain
Line range hint
356-373
: Add error handling incalculateGrossFillQuantities
to manage cases wheremakerSideOrders
is empty ortakerOrder
has invalid values.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in `calculateGrossFillQuantities`. # Test: Search for the function usage with invalid or empty inputs. Expect: Proper error handling. rg --type python $'calculateGrossFillQuantities'Length of output: 78
Script:
#!/bin/bash # Description: Verify error handling in `calculateGrossFillQuantities`. # Search for the function definition in the codebase ast-grep --lang typescript --pattern 'function calculateGrossFillQuantities($_, $_) { $$$ }' # Search for the function usage in the codebase rg 'calculateGrossFillQuantities'Length of output: 1015
Script:
#!/bin/bash # Description: Extract the implementation of `calculateGrossFillQuantities` and relevant test cases. # Extract the function implementation ast-grep --lang typescript --pattern 'function calculateGrossFillQuantities($_, $_) { $$$ }' src/orderbook/quantities.ts # Extract the relevant test cases rg -A 10 'describe\(.*calculateGrossFillQuantities' src/tests/orderbook/quantities/calculateGrossFillQuantities.test.tsLength of output: 603
Script:
#!/bin/bash # Description: Extract the complete implementation of `calculateGrossFillQuantities` and full test cases. # Extract the complete function implementation ast-grep --lang typescript --pattern 'function calculateGrossFillQuantities($_, $_) { $$$ }' src/orderbook/quantities.ts --output full # Extract the full test cases rg -A 50 'describe\(.*calculateGrossFillQuantities' src/tests/orderbook/quantities/calculateGrossFillQuantities.test.tsLength of output: 2241
Script:
#!/bin/bash # Description: Extract the complete implementation of `calculateGrossFillQuantities` and full test cases. # Extract the complete function implementation ast-grep --lang typescript --pattern 'function calculateGrossFillQuantities($_, $_) { $$$ }' src/orderbook/quantities.ts # Extract the full test cases rg -A 100 'describe\(.*calculateGrossFillQuantities' src/tests/orderbook/quantities/calculateGrossFillQuantities.test.tsLength of output: 3440
Script:
#!/bin/bash # Description: Extract the complete implementation of `calculateGrossFillQuantities` and full test cases. # Extract the complete function implementation ast-grep --lang typescript --pattern 'function calculateGrossFillQuantities($_, $_) { $$$ }' src/orderbook/quantities.ts # Extract the full test cases rg -A 200 'describe\(.*calculateGrossFillQuantities' src/tests/orderbook/quantities/calculateGrossFillQuantities.test.tsLength of output: 6555
Script:
#!/bin/bash # Description: Extract the complete implementation of `calculateGrossFillQuantities` and full test cases. # Extract the complete function implementation ast-grep --lang typescript --pattern 'function calculateGrossFillQuantities($_, $_) { $$$ }' src/orderbook/quantities.ts # Extract the full test cases rg -A 300 'describe\(.*calculateGrossFillQuantities' src/tests/orderbook/quantities/calculateGrossFillQuantities.test.tsLength of output: 9810
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package.json
is excluded by!**/*.json
and included by none
Files selected for processing (4)
- src/orderbook/quantities.ts (6 hunks)
- src/pipmath.ts (2 hunks)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts (1 hunks)
- src/tests/orderbook/quantities/calculateGrossFillQuantities.test.ts (6 hunks)
Files skipped from review due to trivial changes (1)
- src/tests/orderbook/quantities/calculateGrossFillQuantities.test.ts
Additional comments not posted (4)
src/pipmath.ts (3)
22-30
: Ensure thatdecimalToPip
handles edge cases such as non-numeric strings or extremely large decimal strings.
100-102
: Ensure thatpipToDecimal
correctly handles edge cases such as extremely largebigint
values.
67-72
: Ensure thatdividePips
handles cases wheredivisorInPips
is zero to avoid division by zero errors.src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts (1)
99-346
: Ensure that all test cases incalculateBuySellPanelEstimate.test.ts
are isolated and do not depend on external state or shared setup.Verification successful
The search results indicate that the
setUpStandardTestAccount
function is called multiple times within thecalculateBuySellPanelEstimate.test.ts
file. This suggests that each test case is using this function to set up its own state, which implies that the test cases are isolated and do not depend on shared state.
setUpStandardTestAccount
is called in each test case, ensuring isolation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify isolation of test cases in `calculateBuySellPanelEstimate.test.ts`. # Test: Search for shared setup or state between test cases. Expect: Each test case should be isolated. rg --type python $'setUpStandardTestAccount'Length of output: 74
Script:
#!/bin/bash # Description: Verify isolation of test cases in `calculateBuySellPanelEstimate.test.ts`. # Test: Search for shared setup or state between test cases. Expect: Each test case should be isolated. rg --type typescript 'setUpStandardTestAccount' src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.tsLength of output: 151
Script:
#!/bin/bash # Description: Verify isolation of test cases in `calculateBuySellPanelEstimate.test.ts`. # Test: Search for shared setup or state between test cases. Expect: Each test case should be isolated. rg 'setUpStandardTestAccount' src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.tsLength of output: 533
export function divideBigInt( | ||
dividend: bigint, | ||
divisor: bigint, | ||
rounding: ROUNDING = ROUNDING.Truncate, | ||
): bigint { | ||
if (divisor < BigInt(0)) { | ||
throw new Error( | ||
`Division by negative numbers is not supported (got ${dividend.toString()}/${divisor.toString()})`, | ||
); | ||
} | ||
const result = dividend / divisor; | ||
|
||
if ( | ||
rounding === ROUNDING.RoundUp && | ||
dividend > BigInt(0) && // Truncation is the same as rounding up in negative numbers | ||
result * divisor !== dividend | ||
) { | ||
return result + BigInt(1); | ||
} | ||
if ( | ||
rounding === ROUNDING.RoundDown && | ||
dividend < BigInt(0) && // Truncation is the same as rounding down in positive numbers | ||
result * divisor !== dividend | ||
) { | ||
return result - BigInt(1); | ||
} | ||
return result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor divideBigInt
to separate the rounding logic into a helper function for better modularity and readability.
- if (rounding === ROUNDING.RoundUp && dividend > BigInt(0) && result * divisor !== dividend) {
- return result + BigInt(1);
- }
- if (rounding === ROUNDING.RoundDown && dividend < BigInt(0) && result * divisor !== dividend) {
- return result - BigInt(1);
- }
+ return applyRounding(result, divisor, dividend, rounding);
Committable suggestion was skipped due low confidence.
export function maxBigInt(a: bigint, b: bigint): bigint { | ||
if (a >= b) { | ||
return a; | ||
} | ||
return b; | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a ternary operator for simplicity in maxBigInt
and minBigInt
.
- if (a >= b) {
- return a;
- }
- return b;
+ return a >= b ? a : b;
- if (a <= b) {
- return a;
- }
- return b;
+ return a <= b ? a : b;
Also applies to: 81-86
Committable suggestion was skipped due low confidence.
export function assetUnitsToDecimal( | ||
assetUnits: bigint, | ||
decimals: number, | ||
): string { | ||
const bn = new BigNumber(assetUnits.toString()); | ||
return bn.shiftedBy(decimals * -1).toFixed(exchangeDecimals); | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure that assetUnitsToDecimal
handles edge cases such as extremely large or small bigint
values. No tests were found for this function in the codebase. Please add tests to cover these scenarios.
- Add tests for
assetUnitsToDecimal
to handle edge cases with extremely large or smallbigint
values.
Analysis chain
Ensure that assetUnitsToDecimal
handles edge cases such as extremely large or small bigint
values.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify edge cases for `assetUnitsToDecimal` function.
# Test: Search for the function usage with extreme values. Expect: Proper handling of edge cases.
rg --type python $'assetUnitsToDecimal'
Length of output: 69
Script:
#!/bin/bash
# Description: Verify edge cases for `assetUnitsToDecimal` function.
# Search for the function definition in the codebase.
rg 'function assetUnitsToDecimal'
# Search for the usage of `assetUnitsToDecimal` in the codebase.
rg 'assetUnitsToDecimal'
# Search for test cases related to `assetUnitsToDecimal`.
rg 'assetUnitsToDecimal' --type ts
Length of output: 927
Script:
#!/bin/bash
# Description: Verify edge cases for `assetUnitsToDecimal` function.
# Display the implementation of `assetUnitsToDecimal` in `src/pipmath.ts`.
rg 'export function assetUnitsToDecimal' -A 10 src/pipmath.ts
# Display the usage of `assetUnitsToDecimal` in `src/bridge/bridge.ts`.
rg 'assetUnitsToDecimal' -A 5 src/bridge/bridge.ts
# Search for test files that might contain tests for `assetUnitsToDecimal`.
fd --type f --name '*test*'
Length of output: 1459
Script:
#!/bin/bash
# Description: Verify edge cases for `assetUnitsToDecimal` function.
# Correct the `fd` command to search for test files.
fd test
# Display the implementation of `assetUnitsToDecimal` in `src/pipmath.ts`.
rg 'export function assetUnitsToDecimal' -A 10 src/pipmath.ts
# Display the usage of `assetUnitsToDecimal` in `src/bridge/bridge.ts`.
rg 'assetUnitsToDecimal' -A 5 src/bridge/bridge.ts
Length of output: 1399
Script:
#!/bin/bash
# Description: Verify edge cases for `assetUnitsToDecimal` function.
# Search for references to `assetUnitsToDecimal` in the test files.
rg 'assetUnitsToDecimal' src/tests/
Length of output: 35
src/orderbook/quantities.ts
Outdated
|
||
const currentPosition = allWalletPositions.find( | ||
(position) => position.market === market.market, | ||
); | ||
const otherPositions = allWalletPositions.filter( | ||
(position) => position.market !== market.market, | ||
); | ||
|
||
const quoteValueOfOtherPositions = | ||
calculateNotionalQuoteValueOfPositions(otherPositions); | ||
const quoteValueOfOtherPositions2p = quoteValueOfOtherPositions * oneInPips; | ||
|
||
const initialMarginRequirementOfOtherPositions = arraySumBigInt( | ||
otherPositions.map((position) => decimalToPip(position.marginRequirement)), | ||
); | ||
const initialMarginRequirementOfOtherPositions2p = | ||
initialMarginRequirementOfOtherPositions * oneInPips; | ||
|
||
const leverageParametersBigInt = | ||
convertToLeverageParametersBigInt(leverageParameters); | ||
|
||
let additionalPositionQty = BigInt(0); // Signed | ||
let additionalPositionCostBasis = BigInt(0); // Signed | ||
let quoteBalance2p = quoteBalance * oneInPips; // Signed | ||
|
||
for (const makerOrder of makerSideOrders) { | ||
if (!doOrdersMatch(makerOrder, { side: takerSide })) { | ||
return { | ||
baseQuantity: additionalPositionQty, | ||
quoteQuantity: additionalPositionCostBasis, | ||
}; | ||
} | ||
if ( | ||
takerSide === 'buy' ? | ||
indexPrice >= makerOrder.price | ||
: indexPrice <= makerOrder.price | ||
) { | ||
return { | ||
baseQuantity: additionalPositionQty, | ||
quoteQuantity: additionalPositionCostBasis, | ||
}; | ||
} | ||
const makerOrderPrice2p = makerOrder.price * oneInPips; | ||
|
||
const positionBalance = | ||
(currentPosition ? decimalToPip(currentPosition.quantity) : BigInt(0)) + | ||
additionalPositionQty; | ||
|
||
const quoteValueOfPosition2p = positionBalance * indexPrice; // Signed | ||
|
||
const initialMarginFraction = calculateInitialMarginFractionWithOverride({ | ||
initialMarginFractionOverride, | ||
leverageParameters: leverageParametersBigInt, | ||
positionBalance, | ||
}); | ||
|
||
// Unsigned | ||
const initialMarginRequirementOfPosition2p = multiplyPips( | ||
absBigInt(quoteValueOfPosition2p), | ||
initialMarginFraction, | ||
); | ||
|
||
// Signed | ||
const maxTakerBaseQty = | ||
((-quoteBalance2p - | ||
quoteValueOfPosition2p - | ||
quoteValueOfOtherPositions2p + | ||
heldCollateral2p + | ||
remainingAvailableCollateral + | ||
initialMarginRequirementOfPosition2p + | ||
initialMarginRequirementOfOtherPositions2p) * | ||
oneInPips) / | ||
(indexPrice2p - | ||
makerOrderPrice2p + | ||
BigInt(takerSide === 'buy' ? -1 : 1) * | ||
indexPrice * | ||
initialMarginFraction); | ||
|
||
if (absBigInt(maxTakerBaseQty) < makerOrder.size) { | ||
additionalPositionQty += maxTakerBaseQty; | ||
additionalPositionCostBasis += multiplyPips( | ||
maxTakerBaseQty, | ||
makerOrder.price, | ||
); | ||
return { | ||
baseQuantity: additionalPositionQty, | ||
quoteQuantity: additionalPositionCostBasis, | ||
}; | ||
} | ||
const tradeBaseQty = makerOrder.size * BigInt(takerSide === 'buy' ? 1 : -1); | ||
const tradeQuoteQty = multiplyPips(tradeBaseQty, makerOrder.price); | ||
additionalPositionQty += tradeBaseQty; | ||
additionalPositionCostBasis += tradeQuoteQty; | ||
|
||
quoteBalance2p -= tradeBaseQty * makerOrder.price; | ||
} | ||
return { | ||
baseQuantity: additionalPositionQty, | ||
quoteQuantity: additionalPositionCostBasis, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor calculateBuySellPanelEstimate
to improve readability by breaking down complex logic into smaller, more manageable functions.
- const accountValue = quoteBalance + calculateNotionalQuoteValueOfPositions(allWalletPositions);
- const initialMarginRequirementOfAllPositions = arraySumBigInt(allWalletPositions.map((position) => decimalToPip(position.marginRequirement)));
+ const accountValue = calculateAccountValue(quoteBalance, allWalletPositions);
+ const initialMarginRequirementOfAllPositions = calculateTotalMarginRequirements(allWalletPositions);
Committable suggestion was skipped due low confidence.
src/orderbook/quantities.ts
Outdated
function calculateInitialMarginFraction( | ||
leverageParameters: LeverageParametersBigInt, | ||
positionBalance: bigint, | ||
): bigint { | ||
const absPositionBalance = absBigInt(positionBalance); | ||
if (absPositionBalance <= leverageParameters.basePositionSize) { | ||
return leverageParameters.initialMarginFraction; | ||
} | ||
return ( | ||
quoteAssetQuantity - | ||
(baseAssetQuantity * quoteAssetQuantity) / | ||
(baseAssetQuantity + grossBaseQuantity) | ||
leverageParameters.initialMarginFraction + | ||
divideBigInt( | ||
absPositionBalance - leverageParameters.basePositionSize, | ||
leverageParameters.incrementalPositionSize, | ||
ROUNDING.RoundUp, | ||
) * | ||
leverageParameters.incrementalInitialMarginFraction | ||
); | ||
} | ||
|
||
/** | ||
* @private | ||
*/ | ||
function calculateInitialMarginFractionWithOverride(args: { | ||
initialMarginFractionOverride: bigint | null; | ||
leverageParameters: LeverageParametersBigInt; | ||
positionBalance: bigint; | ||
}): bigint { | ||
const { initialMarginFractionOverride, leverageParameters, positionBalance } = | ||
args; | ||
|
||
return maxBigInt( | ||
calculateInitialMarginFraction(leverageParameters, positionBalance), | ||
initialMarginFractionOverride ?? BigInt(0), | ||
); | ||
} | ||
|
||
/** | ||
* @private | ||
*/ | ||
function calculateNotionalQuoteValueOfPosition(position: IDEXPosition): bigint { | ||
return multiplyPips( | ||
decimalToPip(position.quantity), | ||
decimalToPip(position.indexPrice), | ||
); | ||
} | ||
|
||
/** | ||
* @private | ||
*/ | ||
function calculateNotionalQuoteValueOfPositions( | ||
positions: IDEXPosition[], | ||
): bigint { | ||
return arraySumBigInt(positions.map(calculateNotionalQuoteValueOfPosition)); | ||
} | ||
|
||
/** | ||
* @private | ||
*/ | ||
function convertToLeverageParametersBigInt( | ||
leverageParameters: LeverageParameters, | ||
): LeverageParametersBigInt { | ||
return { | ||
maximumPositionSize: decimalToPip(leverageParameters.maximumPositionSize), | ||
initialMarginFraction: decimalToPip( | ||
leverageParameters.initialMarginFraction, | ||
), | ||
maintenanceMarginFraction: decimalToPip( | ||
leverageParameters.maintenanceMarginFraction, | ||
), | ||
basePositionSize: decimalToPip(leverageParameters.basePositionSize), | ||
incrementalPositionSize: decimalToPip( | ||
leverageParameters.incrementalPositionSize, | ||
), | ||
incrementalInitialMarginFraction: decimalToPip( | ||
leverageParameters.incrementalInitialMarginFraction, | ||
), | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The functions calculateInitialMarginFraction
and convertToLeverageParametersBigInt
are used within the src/orderbook/quantities.ts
file but are not directly tested in the identified test files. This indicates that these functions might not have dedicated unit tests.
- Ensure that the functions
calculateInitialMarginFraction
andconvertToLeverageParametersBigInt
are thoroughly unit tested to verify their correctness.
Analysis chain
Ensure that private functions like calculateInitialMarginFraction
and convertToLeverageParametersBigInt
are thoroughly unit tested to verify their correctness.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify unit tests for private functions in `quantities.ts`.
# Test: Search for unit tests covering these functions. Expect: Comprehensive test coverage.
rg --type python $'calculateInitialMarginFraction|convertToLeverageParametersBigInt'
Length of output: 116
Script:
#!/bin/bash
# Description: Verify unit tests for private functions in `quantities.ts`.
# Test: Search for unit tests covering these functions. Expect: Comprehensive test coverage.
rg --type js 'calculateInitialMarginFraction|convertToLeverageParametersBigInt'
Length of output: 230
Script:
#!/bin/bash
# Description: Debug ripgrep to understand why files are being skipped.
# Test: Run ripgrep with --debug to identify why no files are being searched.
rg --type js 'calculateInitialMarginFraction|convertToLeverageParametersBigInt' --debug
Length of output: 43689
Script:
#!/bin/bash
# Description: Verify unit tests for private functions in `quantities.ts`.
# Test: Search for unit tests covering these functions. Expect: Comprehensive test coverage.
rg --type js 'calculateInitialMarginFraction|convertToLeverageParametersBigInt' --no-ignore
Length of output: 242
Script:
#!/bin/bash
# Description: Verify unit tests for private functions in `quantities.ts`.
# Step 1: Find all test files.
test_files=$(fd --type f --extension js --extension ts --glob '*test*')
# Step 2: Search for the function calls within the test files.
for file in $test_files; do
echo "Searching in $file"
rg 'calculateInitialMarginFraction|convertToLeverageParametersBigInt' "$file"
done
Length of output: 1407
Script:
#!/bin/bash
# Description: Verify usage of private functions in the entire codebase.
# Step 1: Search for the usage of the functions in the entire codebase.
rg 'calculateInitialMarginFraction|convertToLeverageParametersBigInt'
Length of output: 595
chore: update alpha
- Provide either a slider value or desired base or quote trade qty
…ired-position-qty feat: buy/sell panel estimate by desired trade qty (base or quote)
## [4.0.0-alpha.10](v4.0.0-alpha.9...v4.0.0-alpha.10) (2024-05-23) ### Features * buy/sell panel estimate by desired trade qty (base or quote) ([3510957](3510957))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/orderbook/quantities.ts (1)
328-345
: The functioncalculateInitialMarginFraction
is correctly implemented to handle different position sizes. Consider adding inline comments to explain the logic, especially how the incremental margin is calculated.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package.json
is excluded by!**/*.json
and included by none
Files selected for processing (2)
- src/orderbook/quantities.ts (6 hunks)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts
Additional comments not posted (6)
src/orderbook/quantities.ts (6)
34-34
: The new typeLeverageParametersBigInt
is well-defined and aligns with the changes in the function signatures that use leverage parameters.
350-360
: The implementation ofcalculateInitialMarginFractionWithOverride
is correct. It effectively uses the override if provided, otherwise, it defaults to the calculated margin. This is a good use of the nullish coalescing operator.
377-381
: The functioncalculateNotionalQuoteValueOfPositions
efficiently aggregates the notional values usingarraySumBigInt
. This is a clean and effective use of functional programming paradigms.
Line range hint
442-459
: The functioncalculateGrossFillQuantities
is well-implemented with clear handling of double-pip precision. The use of a helper functionmakeReturnValue
for returning results based on the precision flag is a good design choice.
502-510
: The optimization suggested in the previous review has been correctly implemented indetermineTradeQuantities
. This should improve the performance by reducing redundant calculations.
67-98
: The refactoring ofcalculateBuySellPanelEstimate
to include additional parameters and return values enhances its functionality. Ensure that all calls to this function are updated to pass the new parameters.
- Only one qty input value in buy/sell panel estimate is allowed
…ping feat: improve type declarations of mutually exclusive arguments
## [4.0.0-alpha.11](v4.0.0-alpha.10...v4.0.0-alpha.11) (2024-05-24) ### Features * improve type declarations of mutually exclusive arguments ([4e967cd](4e967cd))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Outside diff range and nitpick comments (2)
src/orderbook/quantities.ts (2)
34-34
: Consider documenting theLeverageParametersBigInt
type for better clarity.Adding a brief comment describing the purpose and usage of
LeverageParametersBigInt
could improve code readability and maintainability.
Line range hint
453-470
: Improve precision handling incalculateGrossFillQuantities
.The function
calculateGrossFillQuantities
handles precision based on thereturnInDoublePipPrecision
flag. To ensure consistent and accurate precision handling across different parts of the application, consider implementing a centralized precision management system that can be easily adjusted and uniformly applied.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package.json
is excluded by!**/*.json
and included by none
Files selected for processing (2)
- src/orderbook/quantities.ts (6 hunks)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts
Additional comments not posted (1)
src/orderbook/quantities.ts (1)
339-371
: Ensure comprehensive unit testing forcalculateInitialMarginFraction
.
src/orderbook/quantities.ts
Outdated
: indexPrice <= makerOrder.price | ||
) { | ||
return { | ||
baseQuantity: additionalPositionQty, | ||
quoteQuantity: additionalPositionCostBasis, | ||
}; | ||
} | ||
const makerOrderPrice2p = makerOrder.price * oneInPips; | ||
|
||
const positionBalance = | ||
(currentPosition ? decimalToPip(currentPosition.quantity) : BigInt(0)) + | ||
additionalPositionQty; | ||
|
||
const quoteValueOfPosition2p = positionBalance * indexPrice; // Signed | ||
|
||
const initialMarginFraction = calculateInitialMarginFractionWithOverride({ | ||
initialMarginFractionOverride, | ||
leverageParameters: leverageParametersBigInt, | ||
positionBalance, | ||
}); | ||
|
||
// Unsigned | ||
const initialMarginRequirementOfPosition2p = multiplyPips( | ||
absBigInt(quoteValueOfPosition2p), | ||
initialMarginFraction, | ||
); | ||
|
||
// Signed | ||
const maxBuyingPowerBase = | ||
((-quoteBalance2p - | ||
quoteValueOfPosition2p - | ||
quoteValueOfOtherPositions2p + | ||
heldCollateral2p + | ||
desiredRemainingAvailableCollateral + | ||
initialMarginRequirementOfPosition2p + | ||
initialMarginRequirementOfOtherPositions2p) * | ||
oneInPips) / | ||
(indexPrice2p - | ||
makerOrderPrice2p + | ||
BigInt(takerSide === 'buy' ? -1 : 1) * | ||
indexPrice * | ||
initialMarginFraction); | ||
|
||
let maxTakerBaseQty = maxBuyingPowerBase; | ||
if (desiredPositionBaseQuantity) { | ||
// Limit base to buying power and desired position qty | ||
maxTakerBaseQty = | ||
takerSide === 'buy' ? | ||
minBigInt( | ||
maxBuyingPowerBase, | ||
desiredPositionBaseQuantity - additionalPositionQty, | ||
) | ||
: maxBigInt( | ||
maxBuyingPowerBase, | ||
desiredPositionBaseQuantity - additionalPositionQty, | ||
); | ||
} | ||
|
||
let maxTakerQuoteQty = multiplyPips(maxTakerBaseQty, makerOrder.price); | ||
|
||
if (desiredPositionQuoteQuantity) { | ||
// Limit quote to buying power and desired position qty | ||
const maxTakerQuoteQtyBefore = maxTakerQuoteQty; | ||
maxTakerQuoteQty = | ||
takerSide === 'buy' ? | ||
minBigInt( | ||
maxTakerQuoteQty, | ||
desiredPositionQuoteQuantity - additionalPositionCostBasis, | ||
) | ||
: maxBigInt( | ||
maxTakerQuoteQty, | ||
desiredPositionQuoteQuantity - additionalPositionCostBasis, | ||
); | ||
|
||
if (maxTakerQuoteQty !== maxTakerQuoteQtyBefore) { | ||
// Reduce base proportionally to reduction of quote | ||
maxTakerBaseQty = | ||
(maxTakerBaseQty * maxTakerQuoteQty) / maxTakerQuoteQtyBefore; | ||
} | ||
} | ||
|
||
if (absBigInt(maxTakerBaseQty) < makerOrder.size) { | ||
additionalPositionQty += maxTakerBaseQty; | ||
additionalPositionCostBasis += maxTakerQuoteQty; | ||
return { | ||
baseQuantity: additionalPositionQty, | ||
quoteQuantity: additionalPositionCostBasis, | ||
}; | ||
} | ||
const tradeBaseQty = makerOrder.size * BigInt(takerSide === 'buy' ? 1 : -1); | ||
const tradeQuoteQty = multiplyPips(tradeBaseQty, makerOrder.price); | ||
additionalPositionQty += tradeBaseQty; | ||
additionalPositionCostBasis += tradeQuoteQty; | ||
|
||
quoteBalance2p -= tradeBaseQty * makerOrder.price; | ||
} | ||
return { | ||
baseQuantity: additionalPositionQty, | ||
quoteQuantity: additionalPositionCostBasis, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor calculateBuySellPanelEstimate
to improve readability and maintainability.
The function calculateBuySellPanelEstimate
is quite lengthy and complex. Consider breaking it down into smaller, more manageable functions to improve readability and maintainability. This could involve separating the logic into functions that handle different parts of the calculation, such as slider calculations, order book execution, and error handling.
src/orderbook/quantities.ts
Outdated
function calculateInitialMarginFractionWithOverride(args: { | ||
initialMarginFractionOverride: bigint | null; | ||
leverageParameters: LeverageParametersBigInt; | ||
positionBalance: bigint; | ||
}): bigint { | ||
const { initialMarginFractionOverride, leverageParameters, positionBalance } = | ||
args; | ||
|
||
return maxBigInt( | ||
calculateInitialMarginFraction(leverageParameters, positionBalance), | ||
initialMarginFractionOverride ?? BigInt(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review the use of maxBigInt
in calculateInitialMarginFractionWithOverride
.
The use of maxBigInt
in calculateInitialMarginFractionWithOverride
assumes that the override value is always greater or equal to the calculated margin. This might not always be the case, depending on business logic. Verify that this assumption holds in all use cases, or adjust the logic to handle different scenarios.
…crease-of-available-collateral feat(buy/sell panel): support trades that increase available collateral
## [4.0.0-alpha.12](v4.0.0-alpha.11...v4.0.0-alpha.12) (2024-05-24) ### Features * **buy/sell panel:** support trades that increase available collateral ([a3b3617](a3b3617))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Outside diff range and nitpick comments (3)
src/orderbook/quantities.ts (3)
34-34
: Define theLeverageParametersBigInt
type more clearly.Consider adding a comment to describe the
LeverageParametersBigInt
type for better code clarity and maintainability.
346-362
: OptimizecalculateInitialMarginFraction
by caching results.The function
calculateInitialMarginFraction
could benefit from caching results if called frequently with the same parameters. This would reduce computational overhead and improve performance.
404-423
: Consider caching inconvertToLeverageParametersBigInt
.The function
convertToLeverageParametersBigInt
performs multiple conversions usingdecimalToPip
. If these conversions are costly and the input parameters do not change frequently, consider caching the results to improve performance.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package.json
is excluded by!**/*.json
and included by none
Files selected for processing (2)
- src/orderbook/quantities.ts (6 hunks)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts
Additional comments not posted (1)
src/orderbook/quantities.ts (1)
Line range hint
460-477
: Ensure precision incalculateGrossFillQuantities
.Verify that the division by
oneInPips
in thecalculateGrossFillQuantities
function does not lead to precision loss, especially whenreturnInDoublePipPrecision
is false.
src/orderbook/quantities.ts
Outdated
export function calculateBuySellPanelEstimate( | ||
args: { | ||
/** All the wallet's open positions, including any in the current market */ | ||
allWalletPositions: IDEXPosition[]; | ||
/** Free collateral committed to open limit orders (unsigned) */ | ||
heldCollateral: bigint; | ||
initialMarginFractionOverride: bigint | null; | ||
leverageParameters: LeverageParameters; | ||
makerSideOrders: Iterable<PriceAndSize>; | ||
market: Pick<IDEXMarket, 'market' | 'indexPrice'>; | ||
/** Quote token balance (USDC) (signed) */ | ||
quoteBalance: bigint; | ||
takerSide: OrderSide; | ||
} /** | ||
* Either desiredPositionBaseQuantity, desiredPositionQuoteQuantity, or | ||
* sliderFactor needs to be provided. | ||
*/ & ( | ||
| { | ||
/** The desired base position qty to be acquired */ | ||
desiredPositionBaseQuantity: bigint; | ||
desiredPositionQuoteQuantity?: undefined; | ||
sliderFactor?: undefined; | ||
} | ||
| { | ||
desiredPositionBaseQuantity?: undefined; | ||
/** The desired quote position qty to be acquired */ | ||
desiredPositionQuoteQuantity: bigint; | ||
sliderFactor?: undefined; | ||
} | ||
| { | ||
desiredPositionBaseQuantity?: undefined; | ||
desiredPositionQuoteQuantity?: undefined; | ||
/** | ||
* Floating point number between 0 and 1 that indicates the amount of | ||
* the available collateral to be spent | ||
*/ | ||
sliderFactor: number; | ||
} | ||
), | ||
): { | ||
baseQuantity: bigint; | ||
quoteQuantity: bigint; | ||
} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor calculateBuySellPanelEstimate
to improve readability.
The function calculateBuySellPanelEstimate
is quite complex and lengthy. Consider breaking it down into smaller, more manageable functions to handle different parts of the calculation, such as slider calculations, order book execution, and error handling. This will enhance readability and maintainability.
src/orderbook/quantities.ts
Outdated
if (undefinedQtyInputs.length !== 2) { | ||
throw new Error( | ||
'Either desiredPositionBaseQuantity, desiredPositionQuoteQuantity, or sliderFactor needs to be provided', | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error handling in calculateBuySellPanelEstimate
.
The error message "Either desiredPositionBaseQuantity, desiredPositionQuoteQuantity, or sliderFactor needs to be provided" could be more descriptive. It should inform the user about the exact issue and how to resolve it.
- throw new Error('Either desiredPositionBaseQuantity, desiredPositionQuoteQuantity, or sliderFactor needs to be provided');
+ throw new Error('Invalid input: Please ensure exactly one of desiredPositionBaseQuantity, desiredPositionQuoteQuantity, or sliderFactor is provided.');
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.
if (undefinedQtyInputs.length !== 2) { | |
throw new Error( | |
'Either desiredPositionBaseQuantity, desiredPositionQuoteQuantity, or sliderFactor needs to be provided', | |
); | |
} | |
if (undefinedQtyInputs.length !== 2) { | |
throw new Error( | |
'Invalid input: Please ensure exactly one of desiredPositionBaseQuantity, desiredPositionQuoteQuantity, or sliderFactor is provided.', | |
); | |
} |
if (sliderFactor < 0 || sliderFactor > 1) { | ||
throw new Error( | ||
'sliderFactor must be a floating point number between 0 and 1', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate sliderFactor
input more robustly.
The current validation for sliderFactor
only checks if it is between 0 and 1. Consider adding a type check to ensure that sliderFactor
is indeed a number to prevent runtime errors.
+ if (typeof sliderFactor !== 'number') {
+ throw new Error('sliderFactor must be a number');
+ }
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.
if (sliderFactor < 0 || sliderFactor > 1) { | |
throw new Error( | |
'sliderFactor must be a floating point number between 0 and 1', | |
); | |
if (typeof sliderFactor !== 'number') { | |
throw new Error('sliderFactor must be a number'); | |
} | |
if (sliderFactor < 0 || sliderFactor > 1) { | |
throw new Error( | |
'sliderFactor must be a floating point number between 0 and 1', | |
); |
- Group input values (form inputs, wallet values)
- Adds a function that calculates the maximum maker order size that can be supported by an available collateral amount
feat: update realtime obs to better handle sequence issues
## [4.0.0-alpha.13](v4.0.0-alpha.12...v4.0.0-alpha.13) (2024-05-28) ### Features * update realtime obs to better handle sequence issues ([dda8032](dda8032))
feat: also reset fully if stopped manually
## [4.0.0-alpha.14](v4.0.0-alpha.13...v4.0.0-alpha.14) (2024-05-28) ### Features * also reset fully if stopped manually ([880b19e](880b19e))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
src/client/orderBook/realTime.ts (2)
Line range hint
1-1
: Consider using thenode:
protocol when importing Node.js built-in modules.- import { EventEmitter } from 'events'; + import { EventEmitter } from 'node:events';
Line range hint
298-319
: Consider simplifying theelse
clause by removing it, as the previous branches already handle all necessary conditions.- } else { - // If an invalid update arrives, reset all data and synchronize anew - this.emit(OrderBookRealTimeClientEvent.disconnected); - this.emit( - OrderBookRealTimeClientEvent.error, - new Error( - `Missing l2 update sequence, current book is ${book.sequence} message was ${update.sequence}`, - ), - ); - // this.unsubscribe(); - this.resetInternalState(); - // this.subscribe(); - - // eslint-disable-next-line no-await-in-loop - await this.synchronizeFromRestApi(); - - this.emit(OrderBookRealTimeClientEvent.connected); - - return; - } + // If an invalid update arrives, reset all data and synchronize anew + this.emit(OrderBookRealTimeClientEvent.disconnected); + this.emit( + OrderBookRealTimeClientEvent.error, + new Error( + `Missing l2 update sequence, current book is ${book.sequence} message was ${update.sequence}`, + ), + ); + // this.unsubscribe(); + this.resetInternalState(); + // this.subscribe(); + + // eslint-disable-next-line no-await-in-loop + await this.synchronizeFromRestApi(); + + this.emit(OrderBookRealTimeClientEvent.connected); + + return;
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
package.json
is excluded by!**/*.json
and included by noneyarn.lock
is excluded by!**/*.lock
,!**/*.lock
,!yarn.lock
and included by none
Files selected for processing (1)
- src/client/orderBook/realTime.ts (5 hunks)
Additional Context Used
Biome (3)
src/client/orderBook/realTime.ts (3)
1-1: A Node.js builtin module should be imported with the node: protocol.
295-295: Unnecessary continue statement
298-319: This else clause can be omitted because previous branches break early.
Additional comments not posted (3)
src/client/orderBook/realTime.ts (3)
205-205
: Ensure that theresetInternalState
method is called with the correct argument.
413-419
: TheresetInternalState
method now conditionally clearsl2OrderBookUpdates
based on theincludeUpdates
parameter. This is a good use of a flag to control behavior.
463-463
: TheresetInternalState
method is correctly called withtrue
to ensure that updates are cleared upon disconnection.
src/client/orderBook/realTime.ts
Outdated
for (const update of updates) { | ||
// outdated sequence, ignore | ||
if (book.sequence > update.sequence) { | ||
if (book.sequence >= update.sequence) { | ||
// eslint-disable-next-line no-continue | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary continue
statement to simplify the loop logic.
- continue;
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.
continue; |
## [4.0.0-alpha.15](v4.0.0-alpha.14...v4.0.0-alpha.15) (2024-05-28) ### Features * merge beta to alpha ([ba78b5f](ba78b5f)) ### Bug Fixes * polish updates on rt sync fixes ([fa0b56f](fa0b56f))
- Determine max. size of a maker order that can be added to the books based on available collateral that does not match order book liquidity
…nsions feat: buy/sell panel estimate extensions
## [4.0.0-alpha.16](v4.0.0-alpha.15...v4.0.0-alpha.16) (2024-05-28) ### Features * **buy/sell panel:** calculate maker order size (quantities) ([d48470f](d48470f)) * **buy/sell panel:** support taker limit price input ([b10abf4](b10abf4)) * calculate max maker order size for available collateral ([d3c7234](d3c7234))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/tests/orderbook/quantities/calculateMaximumMakerOrderSizeForAvailableCollateral.test.ts (1)
42-127
: The structured approach to testing various scenarios is commendable. It's clear and covers a range of inputs. However, consider adding comments to explain the significance of specific values, especially in boundary cases like lines 66-83 and 95-117.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package.json
is excluded by!**/*.json
and included by none
Files selected for processing (3)
- src/orderbook/quantities.ts (6 hunks)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts (1 hunks)
- src/tests/orderbook/quantities/calculateMaximumMakerOrderSizeForAvailableCollateral.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts
Additional comments not posted (7)
src/tests/orderbook/quantities/calculateMaximumMakerOrderSizeForAvailableCollateral.test.ts (3)
3-5
: Imports fordecimalToPip
andorderbook
have been added to support the new test scenarios.
9-16
: ThedefaultLeverageParameters
object has been introduced, which is a good practice for reusability in tests. However, ensure that themaintenanceMarginFraction
field, marked as unused, is necessary for future tests or consider removing it if not needed.Verification successful
The
maintenanceMarginFraction
field is indeed present in multiple files, including other test files and type definitions. This indicates that it might be part of a broader test setup or used indirectly in other contexts. Therefore, it is advisable to keep the field for consistency and potential future use.
src/tests/orderbook/quantities/calculateMaximumMakerOrderSizeForAvailableCollateral.test.ts
src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts
src/orderbook/quantities.ts
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `maintenanceMarginFraction` is used elsewhere in the tests. # Test: Search for the usage of `maintenanceMarginFraction`. Expect: Find usage or confirm it's unused. rg --type ts 'maintenanceMarginFraction'Length of output: 1642
18-40
: TherunDefaultScenario
function centralizes the testing logic, which enhances maintainability and readability. Ensure that all edge cases are covered by the provided scenarios.Verification successful
The
runDefaultScenario
function is invoked with a variety ofavailableCollateral
values, ranging from very small ('0.00000010') to very large ('1000'). This range suggests a broad spectrum of test cases. However, to ensure comprehensive coverage, we need to confirm that these values adequately cover all potential edge cases.
0.00000010
: Very small value, testing precision and handling of small numbers.1
,5
,50
,60
,66
,72
,84
,91
,98
: Incremental values to test normal operation.60.00000011
,60.00000012
,84.00000013
,84.00000014
: Values with high precision to test handling of fractional parts.1000
: Very large value to test upper limits.These values appear to cover a wide range of scenarios, including small, large, and precise fractional values. Therefore, it seems that the edge cases are well-covered.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify coverage of edge cases in `runDefaultScenario`. # Test: Search for additional test scenarios that might be missing. Expect: Comprehensive coverage. rg --type ts 'runDefaultScenario'Length of output: 1948
src/orderbook/quantities.ts (4)
33-33
: Introduction ofLeverageParametersBigInt
type is a good practice for ensuring type safety and clarity in leveraging parameters.
658-677
: TheconvertToLeverageParametersBigInt
function has been optimized as suggested in previous reviews. This is a good improvement for performance.
96-400
: ThecalculateBuySellPanelEstimate
function has been significantly refactored. It's crucial to ensure that all new logic paths are covered by unit tests, especially the new error handling and boundary conditions.Verification successful
The test file
src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts
contains comprehensive unit tests for thecalculateBuySellPanelEstimate
function. These tests cover various scenarios, including:
- Different taker sides (buy and sell).
- Presence and absence of limit prices.
- Different slider factors.
- Desired position quantities (both base and quote).
- Edge cases like zero and negative available collateral.
- Error handling for invalid inputs.
Given the extensive coverage observed, it appears that the new logic paths and boundary conditions introduced in the refactored
calculateBuySellPanelEstimate
function are well-tested.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify comprehensive unit test coverage for `calculateBuySellPanelEstimate`. # Test: Search for unit tests covering new logic paths in this function. Expect: Comprehensive coverage. rg --type ts 'calculateBuySellPanelEstimate'Length of output: 2418
Script:
#!/bin/bash # Extract and display the test cases for `calculateBuySellPanelEstimate` to verify coverage of new logic paths. cat src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.tsLength of output: 33661
58-79
: ThecalculateAvailableCollateral
function has been updated to include more detailed destructuring and calculations. Ensure that the logic correctly handles all possible inputs, especially edge cases.Verification successful
The test file
src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts
includes extensive test cases for thecalculateAvailableCollateral
function. These tests cover various scenarios, including edge cases such as zero balances, negative balances, and large numbers. The tests also ensure that the function handles different market conditions and input variations correctly.
- The tests verify the function's behavior with different
takerSide
values (buy
andsell
).- They check the function's response to different
sliderFactor
values, including edge cases like zero and negative values.- The tests also cover scenarios where the available collateral is zero or negative, ensuring the function returns zero in these cases.
- Additionally, the tests validate the function's handling of incorrect input combinations and ensure it throws appropriate errors.
Given the comprehensive nature of these tests, it appears that the
calculateAvailableCollateral
function is well-covered and handles edge cases appropriately.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic of `calculateAvailableCollateral` for edge cases. # Test: Search for additional tests covering edge cases for this function. Expect: Find tests or confirm the need for more. rg --type ts 'calculateAvailableCollateral'Length of output: 996
Script:
#!/bin/bash # Display the content of the test file to review the test cases for `calculateAvailableCollateral` cat src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.tsLength of output: 33661
- In addition to calculation based on slider value
- Cost is the estimated decrease in available collateral
…nsions-2 feat: buy/sell panel estimate extensions (2)
## [4.0.0-alpha.17](v4.0.0-alpha.16...v4.0.0-alpha.17) (2024-05-29) ### Features * **buy/sell panel:** add cost to returned values ([3357f19](3357f19)) * **buy/sell panel:** calculate maker order size by desired trade qty ([9eb671f](9eb671f)) * **buy/sell panel:** return all values unsigned ([3957057](3957057)) ### Bug Fixes * **buy/sell panel:** maker order IMR is based on limit (not index) price ([02f35e5](02f35e5))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
src/orderbook/quantities.ts (1)
801-820
: The functionconvertToLeverageParametersBigInt
effectively converts leverage parameters to big integers. However, consider caching these conversions if they are costly and the input parameters do not change frequently.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package.json
is excluded by!**/*.json
and included by none
Files selected for processing (3)
- src/orderbook/quantities.ts (6 hunks)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts (1 hunks)
- src/tests/orderbook/quantities/calculateMaximumMakerOrderSizeForAvailableCollateral.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts
- src/tests/orderbook/quantities/calculateMaximumMakerOrderSizeForAvailableCollateral.test.ts
Additional comments not posted (2)
src/orderbook/quantities.ts (2)
34-34
: The new typeLeverageParametersBigInt
is well-defined and enhances type safety.
192-195
: ValidatesliderFactor
input more robustly.+ if (typeof sliderFactor !== 'number') { + throw new Error('sliderFactor must be a number'); + }Likely invalid or redundant comment.
src/orderbook/quantities.ts
Outdated
// Signed | ||
let maxTakerBaseQty = | ||
makerOrder.size * BigInt(takerSide === 'buy' ? 1 : -1); | ||
|
||
if ( | ||
takerSide === 'buy' ? | ||
makerOrder.price > | ||
multiplyPips(indexPrice, oneInPips - initialMarginFraction) | ||
: multiplyPips(indexPrice, oneInPips + initialMarginFraction) > | ||
makerOrder.price | ||
) { | ||
/* | ||
* Trade decreases available collateral; determine the taker's buying | ||
* power (may exceed the maker order size). | ||
*/ | ||
|
||
// Unsigned | ||
const initialMarginRequirementOfPosition2p = multiplyPips( | ||
absBigInt(quoteValueOfPosition2p), | ||
initialMarginFraction, | ||
); | ||
|
||
// Signed | ||
maxTakerBaseQty = | ||
((-quoteBalance2p - | ||
quoteValueOfPosition2p - | ||
quoteValueOfOtherPositions2p + | ||
heldCollateral2p + | ||
desiredRemainingAvailableCollateral2p + | ||
initialMarginRequirementOfPosition2p + | ||
initialMarginRequirementOfOtherPositions2p) * | ||
oneInPips) / | ||
(indexPrice2p - | ||
makerOrderPrice2p + | ||
BigInt(takerSide === 'buy' ? -1 : 1) * | ||
indexPrice * | ||
initialMarginFraction); | ||
} | ||
|
||
if (desiredTradeBaseQuantity) { | ||
// Limit max base to desired trade qty and buying power | ||
maxTakerBaseQty = | ||
takerSide === 'buy' ? | ||
minBigInt( | ||
maxTakerBaseQty, | ||
desiredTradeBaseQuantity - additionalPositionQty, | ||
) | ||
: maxBigInt( | ||
maxTakerBaseQty, | ||
desiredTradeBaseQuantity - additionalPositionQty, | ||
); | ||
} | ||
|
||
let maxTakerQuoteQty = multiplyPips(maxTakerBaseQty, makerOrder.price); | ||
|
||
if (desiredTradeQuoteQuantity) { | ||
// Limit max quote to desired trade qty and buying power | ||
const maxTakerQuoteQtyBefore = maxTakerQuoteQty; | ||
maxTakerQuoteQty = | ||
takerSide === 'buy' ? | ||
minBigInt( | ||
maxTakerQuoteQty, | ||
desiredTradeQuoteQuantity - additionalPositionCostBasis, | ||
) | ||
: maxBigInt( | ||
maxTakerQuoteQty, | ||
desiredTradeQuoteQuantity - additionalPositionCostBasis, | ||
); | ||
|
||
if (maxTakerQuoteQty !== maxTakerQuoteQtyBefore) { | ||
// Reduce base proportionally to reduction of quote | ||
maxTakerBaseQty = | ||
(maxTakerBaseQty * maxTakerQuoteQty) / maxTakerQuoteQtyBefore; | ||
} | ||
} | ||
|
||
if (absBigInt(maxTakerBaseQty) < makerOrder.size) { | ||
additionalPositionQty += maxTakerBaseQty; | ||
additionalPositionCostBasis += maxTakerQuoteQty; | ||
return makeReturnValue({ | ||
makerBaseQuantity: BigInt(0), | ||
makerQuoteQuantity: BigInt(0), | ||
takerBaseQuantity: additionalPositionQty, | ||
takerQuoteQuantity: additionalPositionCostBasis, | ||
}); | ||
} | ||
const tradeBaseQty = makerOrder.size * BigInt(takerSide === 'buy' ? 1 : -1); | ||
const tradeQuoteQty = multiplyPips(tradeBaseQty, makerOrder.price); | ||
additionalPositionQty += tradeBaseQty; | ||
additionalPositionCostBasis += tradeQuoteQty; | ||
|
||
quoteBalance2p -= tradeBaseQty * makerOrder.price; | ||
} | ||
|
||
return makeReturnValue({ | ||
...calculateMakerQtys(), | ||
takerBaseQuantity: additionalPositionQty, | ||
takerQuoteQuantity: additionalPositionCostBasis, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function calculateBuySellPanelEstimate
is quite complex and lengthy. Consider refactoring it to improve readability and maintainability. This could involve separating the logic into functions that handle different parts of the calculation, such as slider calculations, order book execution, and error handling.
- export function calculateBuySellPanelEstimate(args: { ... }) { ... }
+ // Refactor into smaller, more manageable functions
+ function handleSliderCalculations(args: { ... }) { ... }
+ function executeAgainstOrderBook(args: { ... }) { ... }
+ export function calculateBuySellPanelEstimate(args: { ... }) {
+ handleSliderCalculations(args);
+ executeAgainstOrderBook(args);
+ }
Committable suggestion was skipped due low confidence.
import { | ||
ROUNDING, | ||
absBigInt, | ||
arraySumBigInt, | ||
decimalToPip, | ||
divideBigInt, | ||
dividePips, | ||
maxBigInt, | ||
minBigInt, | ||
multiplyPips, | ||
oneInPips, | ||
} from '#pipmath'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider consolidating imports from #pipmath
to enhance readability.
- import {
- ROUNDING,
- absBigInt,
- arraySumBigInt,
- decimalToPip,
- divideBigInt,
- dividePips,
- maxBigInt,
- minBigInt,
- multiplyPips,
- oneInPips,
- } from '#pipmath';
+ import * as pipMath from '#pipmath';
Then, you can use pipMath.ROUNDING
, pipMath.absBigInt
, etc., throughout your code.
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.
import { | |
ROUNDING, | |
absBigInt, | |
arraySumBigInt, | |
decimalToPip, | |
divideBigInt, | |
dividePips, | |
maxBigInt, | |
minBigInt, | |
multiplyPips, | |
oneInPips, | |
} from '#pipmath'; | |
import * as pipMath from '#pipmath'; |
- Extend test coverage for maker qtys
…-qtys-for-sells fix(buy/sell panel): maker qtys are wrong for sells
## [4.0.0-alpha.18](v4.0.0-alpha.17...v4.0.0-alpha.18) (2024-06-03) ### Bug Fixes * **buy/sell panel:** maker qtys are wrong for sells ([46a5f14](46a5f14))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package.json
is excluded by!**/*.json
and included by none
Files selected for processing (2)
- src/orderbook/quantities.ts (6 hunks)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts (1 hunks)
Files not summarized due to errors (1)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts: Error: Message exceeds token limit
Additional context used
Biome
Additional comments not posted (1)
src/orderbook/quantities.ts (1)
34-34
: New typeLeverageParametersBigInt
introduced for better type safety.This change enhances type safety by ensuring that leverage parameters are consistently handled as
bigint
, which is crucial for precision in financial calculations.
function convertToLeverageParametersBigInt( | ||
leverageParameters: LeverageParameters, | ||
): LeverageParametersBigInt { | ||
return { | ||
maximumPositionSize: decimalToPip(leverageParameters.maximumPositionSize), | ||
initialMarginFraction: decimalToPip( | ||
leverageParameters.initialMarginFraction, | ||
), | ||
maintenanceMarginFraction: decimalToPip( | ||
leverageParameters.maintenanceMarginFraction, | ||
), | ||
basePositionSize: decimalToPip(leverageParameters.basePositionSize), | ||
incrementalPositionSize: decimalToPip( | ||
leverageParameters.incrementalPositionSize, | ||
), | ||
incrementalInitialMarginFraction: decimalToPip( | ||
leverageParameters.incrementalInitialMarginFraction, | ||
), | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimize convertToLeverageParametersBigInt
by caching repeated conversions.
The function converts multiple parameters using decimalToPip
. If these conversions are costly and the input parameters do not change frequently, consider caching the results to improve performance.
const makerQuantity2p = makerOrder.size * oneInPips; | ||
|
||
// Limit by base | ||
const fillBaseQty2p = | ||
isMaxTakerQuantityInQuote ? makerQuantity2p : ( | ||
pipmath.minBigInt(maxTakerQuantity2p, makerQuantity2p) | ||
minBigInt(maxTakerQuantity2p, makerQuantity2p) | ||
); | ||
|
||
const fillQuoteQty2p = pipmath.multiplyPips(fillBaseQty2p, makerOrder.price); | ||
const fillQuoteQty2p = multiplyPips(fillBaseQty2p, makerOrder.price); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle division by zero in determineTradeQuantities
.
The function does not explicitly handle division by zero, which could potentially lead to runtime errors if such a situation arises.
+ if (maxTakerQuantity2p === BigInt(0)) {
+ throw new Error('Division by zero error');
+ }
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.
const makerQuantity2p = makerOrder.size * oneInPips; | |
// Limit by base | |
const fillBaseQty2p = | |
isMaxTakerQuantityInQuote ? makerQuantity2p : ( | |
pipmath.minBigInt(maxTakerQuantity2p, makerQuantity2p) | |
minBigInt(maxTakerQuantity2p, makerQuantity2p) | |
); | |
const fillQuoteQty2p = pipmath.multiplyPips(fillBaseQty2p, makerOrder.price); | |
const fillQuoteQty2p = multiplyPips(fillBaseQty2p, makerOrder.price); | |
const makerQuantity2p = makerOrder.size * oneInPips; | |
// Limit by base | |
const fillBaseQty2p = | |
isMaxTakerQuantityInQuote ? makerQuantity2p : ( | |
minBigInt(maxTakerQuantity2p, makerQuantity2p) | |
); | |
if (maxTakerQuantity2p === BigInt(0)) { | |
throw new Error('Division by zero error'); | |
} | |
const fillQuoteQty2p = multiplyPips(fillBaseQty2p, makerOrder.price); |
export function calculateAvailableCollateral(wallet: { | ||
/** Free collateral committed to open limit orders (unsigned) */ | ||
heldCollateral: bigint; | ||
/** All the wallet's open positions */ | ||
positions: Position[]; | ||
/** Quote token balance (USDC) (signed) */ | ||
quoteBalance: bigint; | ||
}): bigint { | ||
const { | ||
heldCollateral, | ||
positions: allWalletPositions, | ||
quoteBalance, | ||
} = wallet; | ||
|
||
const accountValue = | ||
quoteBalance + calculateNotionalQuoteValueOfPositions(allWalletPositions); | ||
|
||
const initialMarginRequirementOfAllPositions = arraySumBigInt( | ||
allWalletPositions.map((position) => position.marginRequirement), | ||
); | ||
return accountValue - initialMarginRequirementOfAllPositions - heldCollateral; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor calculateAvailableCollateral
to improve readability and maintainability.
The function is quite complex. Consider breaking it down into smaller, more manageable functions to handle different parts of the calculation, such as calculating account value and initial margin requirements separately.
+ function calculateAccountValue(quoteBalance, allWalletPositions) {
+ return quoteBalance + calculateNotionalQuoteValueOfPositions(allWalletPositions);
+ }
+ function calculateTotalMarginRequirements(allWalletPositions) {
+ return arraySumBigInt(allWalletPositions.map((position) => position.marginRequirement));
+ }
- const accountValue = quoteBalance + calculateNotionalQuoteValueOfPositions(allWalletPositions);
- const initialMarginRequirementOfAllPositions = arraySumBigInt(allWalletPositions.map((position) => position.marginRequirement));
+ const accountValue = calculateAccountValue(quoteBalance, allWalletPositions);
+ const initialMarginRequirementOfAllPositions = calculateTotalMarginRequirements(allWalletPositions);
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.
export function calculateAvailableCollateral(wallet: { | |
/** Free collateral committed to open limit orders (unsigned) */ | |
heldCollateral: bigint; | |
/** All the wallet's open positions */ | |
positions: Position[]; | |
/** Quote token balance (USDC) (signed) */ | |
quoteBalance: bigint; | |
}): bigint { | |
const { | |
heldCollateral, | |
positions: allWalletPositions, | |
quoteBalance, | |
} = wallet; | |
const accountValue = | |
quoteBalance + calculateNotionalQuoteValueOfPositions(allWalletPositions); | |
const initialMarginRequirementOfAllPositions = arraySumBigInt( | |
allWalletPositions.map((position) => position.marginRequirement), | |
); | |
return accountValue - initialMarginRequirementOfAllPositions - heldCollateral; | |
} | |
export function calculateAvailableCollateral(wallet: { | |
/** Free collateral committed to open limit orders (unsigned) */ | |
heldCollateral: bigint; | |
/** All the wallet's open positions */ | |
positions: Position[]; | |
/** Quote token balance (USDC) (signed) */ | |
quoteBalance: bigint; | |
}): bigint { | |
const { | |
heldCollateral, | |
positions: allWalletPositions, | |
quoteBalance, | |
} = wallet; | |
function calculateAccountValue(quoteBalance, allWalletPositions) { | |
return quoteBalance + calculateNotionalQuoteValueOfPositions(allWalletPositions); | |
} | |
function calculateTotalMarginRequirements(allWalletPositions) { | |
return arraySumBigInt(allWalletPositions.map((position) => position.marginRequirement)); | |
} | |
const accountValue = calculateAccountValue(quoteBalance, allWalletPositions); | |
const initialMarginRequirementOfAllPositions = calculateTotalMarginRequirements(allWalletPositions); | |
return accountValue - initialMarginRequirementOfAllPositions - heldCollateral; | |
} |
// Signed | ||
let maxTakerBaseQty = | ||
makerOrder.size * BigInt(takerSide === 'buy' ? 1 : -1); | ||
|
||
if ( | ||
takerSide === 'buy' ? | ||
makerOrder.price > | ||
multiplyPips(indexPrice, oneInPips - initialMarginFraction) | ||
: multiplyPips(indexPrice, oneInPips + initialMarginFraction) > | ||
makerOrder.price | ||
) { | ||
/* | ||
* Trade decreases available collateral; determine the taker's buying | ||
* power (may exceed the maker order size). | ||
*/ | ||
|
||
// Unsigned | ||
const initialMarginRequirementOfPosition2p = multiplyPips( | ||
absBigInt(quoteValueOfPosition2p), | ||
initialMarginFraction, | ||
); | ||
|
||
// Signed | ||
maxTakerBaseQty = | ||
((-quoteBalance2p - | ||
quoteValueOfPosition2p - | ||
quoteValueOfOtherPositions2p + | ||
heldCollateral2p + | ||
desiredRemainingAvailableCollateral2p + | ||
initialMarginRequirementOfPosition2p + | ||
initialMarginRequirementOfOtherPositions2p) * | ||
oneInPips) / | ||
(indexPrice2p - | ||
makerOrderPrice2p + | ||
BigInt(takerSide === 'buy' ? -1 : 1) * | ||
indexPrice * | ||
initialMarginFraction); | ||
} | ||
|
||
if (desiredTradeBaseQuantity) { | ||
// Limit max base to desired trade qty and buying power | ||
maxTakerBaseQty = | ||
takerSide === 'buy' ? | ||
minBigInt( | ||
maxTakerBaseQty, | ||
desiredTradeBaseQuantity - additionalPositionQty, | ||
) | ||
: maxBigInt( | ||
maxTakerBaseQty, | ||
desiredTradeBaseQuantity - additionalPositionQty, | ||
); | ||
} | ||
|
||
let maxTakerQuoteQty = multiplyPips(maxTakerBaseQty, makerOrder.price); | ||
|
||
if (desiredTradeQuoteQuantity) { | ||
// Limit max quote to desired trade qty and buying power | ||
const maxTakerQuoteQtyBefore = maxTakerQuoteQty; | ||
maxTakerQuoteQty = | ||
takerSide === 'buy' ? | ||
minBigInt( | ||
maxTakerQuoteQty, | ||
desiredTradeQuoteQuantity - additionalPositionCostBasis, | ||
) | ||
: maxBigInt( | ||
maxTakerQuoteQty, | ||
desiredTradeQuoteQuantity - additionalPositionCostBasis, | ||
); | ||
|
||
if (maxTakerQuoteQty !== maxTakerQuoteQtyBefore) { | ||
// Reduce base proportionally to reduction of quote | ||
maxTakerBaseQty = | ||
(maxTakerBaseQty * maxTakerQuoteQty) / maxTakerQuoteQtyBefore; | ||
} | ||
} | ||
|
||
if (absBigInt(maxTakerBaseQty) < makerOrder.size) { | ||
additionalPositionQty += maxTakerBaseQty; | ||
additionalPositionCostBasis += maxTakerQuoteQty; | ||
return makeReturnValue({ | ||
makerBaseQuantity: BigInt(0), | ||
makerQuoteQuantity: BigInt(0), | ||
takerBaseQuantity: additionalPositionQty, | ||
takerQuoteQuantity: additionalPositionCostBasis, | ||
}); | ||
} | ||
const tradeBaseQty = makerOrder.size * BigInt(takerSide === 'buy' ? 1 : -1); | ||
const tradeQuoteQty = multiplyPips(tradeBaseQty, makerOrder.price); | ||
additionalPositionQty += tradeBaseQty; | ||
additionalPositionCostBasis += tradeQuoteQty; | ||
|
||
quoteBalance2p -= tradeBaseQty * makerOrder.price; | ||
} | ||
|
||
return makeReturnValue({ | ||
...calculateMakerQtys(), | ||
takerBaseQuantity: additionalPositionQty, | ||
takerQuoteQuantity: additionalPositionCostBasis, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure proper error handling in calculateBuySellPanelEstimate
.
The function calculateBuySellPanelEstimate
includes checks for undefined values and throws errors for certain conditions. However, there is no evidence of handling division by zero errors. Please review the function to ensure all edge cases, such as division by zero, are properly handled to prevent runtime errors.
- Add checks to handle division by zero scenarios.
- Ensure all input types are validated and appropriate errors are thrown.
Analysis chain
Ensure proper error handling in calculateBuySellPanelEstimate
.
The function is complex and handles various calculations related to trading. Ensure that all edge cases, such as division by zero and invalid input types, are properly handled to prevent runtime errors.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in `calculateBuySellPanelEstimate`.
# Test: Search for error handling related to division by zero and invalid input types. Expect: Proper checks in place.
rg --type ts 'calculateBuySellPanelEstimate' -A 20 -B 5 src/orderbook/quantities.ts
Length of output: 978
Script:
#!/bin/bash
# Description: Verify error handling in `calculateBuySellPanelEstimate`.
# Test: Search for error handling related to division by zero, invalid input types, and other edge cases. Expect: Proper checks in place.
# Check for division by zero handling
rg '\/ 0' src/orderbook/quantities.ts
# Check for invalid input type handling
rg 'typeof' src/orderbook/quantities.ts
# Check for other potential edge cases
rg 'throw new Error' src/orderbook/quantities.ts
Length of output: 503
Script:
#!/bin/bash
# Description: Verify error handling in `calculateBuySellPanelEstimate`.
# Test: Search for error handling related to division by zero, invalid input types, and other edge cases. Expect: Proper checks in place.
# Corrected check for division by zero handling
rg '\/ 0' src/orderbook/quantities.ts || rg '/ 0' src/orderbook/quantities.ts
# Re-run check for invalid input type handling
rg 'typeof' src/orderbook/quantities.ts
# Re-run check for other potential edge cases
rg 'throw new Error' src/orderbook/quantities.ts
Length of output: 542
Script:
#!/bin/bash
# Description: Verify error handling in `calculateBuySellPanelEstimate`.
# Test: Search for error handling related to division by zero, invalid input types, and other edge cases. Expect: Proper checks in place.
# Corrected check for division by zero handling
rg ' / 0' src/orderbook/quantities.ts
# Re-run check for invalid input type handling
rg 'typeof' src/orderbook/quantities.ts
# Re-run check for other potential edge cases
rg 'throw new Error' src/orderbook/quantities.ts
Length of output: 432
const defaultLeverageParameters: orderbook.LeverageParametersBigInt = { | ||
initialMarginFraction: decimalToPip('0.03'), | ||
incrementalInitialMarginFraction: decimalToPip('0.01'), | ||
basePositionSize: decimalToPip('10000'), | ||
incrementalPositionSize: decimalToPip('10000'), | ||
maximumPositionSize: decimalToPip('100000'), // Unused | ||
maintenanceMarginFraction: decimalToPip('0.01'), // Unused | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The current test cases involving defaultLeverageParameters
do not explicitly cover comprehensive edge cases. It is recommended to ensure that edge cases are tested to validate the robustness of the implementation.
- Ensure edge cases for
defaultLeverageParameters
are comprehensively tested in the following files:src/tests/orderbook/quantities/calculateMaximumMakerOrderSizeForAvailableCollateral.test.ts
src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts
Analysis chain
Ensure comprehensive edge case testing for defaultLeverageParameters
.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify edge cases for `defaultLeverageParameters` in tests.
# Test: Search for the usage of `defaultLeverageParameters` with edge cases. Expect: Comprehensive test coverage.
rg --type ts 'defaultLeverageParameters'
Length of output: 645
Script:
#!/bin/bash
# Extract and review test cases involving `defaultLeverageParameters` in both files.
# Extract test cases from calculateMaximumMakerOrderSizeForAvailableCollateral.test.ts
echo "Test cases in calculateMaximumMakerOrderSizeForAvailableCollateral.test.ts:"
rg -A 10 'defaultLeverageParameters' src/tests/orderbook/quantities/calculateMaximumMakerOrderSizeForAvailableCollateral.test.ts
# Extract test cases from calculateBuySellPanelEstimate.test.ts
echo "Test cases in calculateBuySellPanelEstimate.test.ts:"
rg -A 10 'defaultLeverageParameters' src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts
Length of output: 2122
…ce-update fix: obs realtime client applies indexPrice changes
## [4.0.0-alpha.19](v4.0.0-alpha.18...v4.0.0-alpha.19) (2024-06-04) ### Bug Fixes * obs realtime client applies indexPrice changes ([a789721](a789721))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package.json
is excluded by!**/*.json
and included by none
Files selected for processing (1)
- src/client/orderBook/utils.ts (1 hunks)
Additional comments not posted (1)
src/client/orderBook/utils.ts (1)
37-37
: EnsureindexPrice
is properly integrated throughout the application.
chore: update alpha
## [4.0.0-alpha.20](v4.0.0-alpha.19...v4.0.0-alpha.20) (2024-06-28) ### Features * add not found status to cancel orders response ([848edb9](848edb9)) * four hour candle intervals ([82c7dd8](82c7dd8)) * monitor realtime client via header ([104acaa](104acaa)) ### Bug Fixes * orderId is optional for not found client order ids ([fb96f49](fb96f49)) * remove 30d fields from get wallets response ([c3af722](c3af722)) * update config for stargate v2 mainnet ([0e571e4](0e571e4)) * update IDEXPayoutProgram type ([1cc62fa](1cc62fa))
- Including positions that change sides
…ction-of-positions test(buy/sell panel): cover reduction of positions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package.json
is excluded by!**/*.json
and included by none
Files selected for processing (1)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts (1 hunks)
Files not summarized due to errors (1)
- src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts: Error: Message exceeds token limit
Additional comments not posted (3)
src/tests/orderbook/quantities/calculateBuySellPanelEstimate.test.ts (3)
114-135
: LGTM!The
setUpStandardTestAccount
function correctly sets up a standard test account with predefined values.
1209-1247
: LGTM!The
calculateAvailableCollateral
function correctly calculates the available collateral based on various parameters.
1249-1328
: LGTM!The
runTradeIncreasesAvailableCollateralBuyScenario
function correctly tests the scenario where a trade increases the available collateral.
function makeAPosition(props: { | ||
market: IDEXMarket; | ||
quantity: bigint; | ||
initialMarginRequirement: bigint; | ||
}): IDEXPosition { | ||
// All empty values are not used by the functions under test | ||
return { | ||
market: props.market.market, | ||
quantity: pipToDecimal(props.quantity), | ||
maximumQuantity: '', | ||
entryPrice: '', | ||
exitPrice: '', | ||
markPrice: '', | ||
indexPrice: props.market.indexPrice, | ||
liquidationPrice: '', | ||
value: '', | ||
realizedPnL: '', | ||
unrealizedPnL: '', | ||
marginRequirement: pipToDecimal(props.initialMarginRequirement), | ||
leverage: '', | ||
totalFunding: '', | ||
totalOpen: '', | ||
totalClose: '', | ||
adlQuintile: 0, | ||
openedByFillId: '', | ||
lastFillId: '', | ||
time: 0, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comprehensive tests for makeAPosition
to ensure it handles various scenarios and edge cases correctly.
Consider adding test cases that cover different market conditions, quantities, and margin requirements.
Would you like me to help by drafting some test cases for makeAPosition
?
function makeAMarket( | ||
indexPrice: bigint, | ||
baseAssetSymbol = 'FOO', | ||
leverageParameters: orderbook.LeverageParametersBigInt = defaultLeverageParameters, | ||
): IDEXMarket { | ||
// All empty values are not used by the functions under test | ||
return { | ||
market: `${baseAssetSymbol}-USD`, | ||
type: 'perpetual', | ||
status: 'active', | ||
baseAsset: baseAssetSymbol, | ||
quoteAsset: 'USD', | ||
stepSize: '', | ||
tickSize: '', | ||
indexPrice: pipToDecimal(indexPrice), | ||
indexPrice24h: '', | ||
indexPricePercentChange: '', | ||
lastFundingRate: null, | ||
currentFundingRate: null, | ||
nextFundingTime: 0, | ||
makerOrderMinimum: '', | ||
takerOrderMinimum: '', | ||
marketOrderExecutionPriceLimit: '', | ||
limitOrderExecutionPriceLimit: '', | ||
minimumPositionSize: '', | ||
|
||
maximumPositionSize: pipToDecimal(leverageParameters.maximumPositionSize), | ||
initialMarginFraction: pipToDecimal( | ||
leverageParameters.initialMarginFraction, | ||
), | ||
maintenanceMarginFraction: pipToDecimal( | ||
leverageParameters.maintenanceMarginFraction, | ||
), | ||
basePositionSize: pipToDecimal(leverageParameters.basePositionSize), | ||
incrementalPositionSize: pipToDecimal( | ||
leverageParameters.incrementalPositionSize, | ||
), | ||
incrementalInitialMarginFraction: pipToDecimal( | ||
leverageParameters.incrementalInitialMarginFraction, | ||
), | ||
|
||
makerFeeRate: '', | ||
takerFeeRate: '', | ||
volume24h: '', | ||
trades24h: 0, | ||
openInterest: '', | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor makeAMarket
to improve readability by separating concerns.
Consider breaking down the function into smaller helper functions to improve readability and maintainability.
+ const createMarketDetails = (baseAssetSymbol, indexPrice) => ({
+ market: `${baseAssetSymbol}-USD`,
+ type: 'perpetual',
+ status: 'active',
+ baseAsset: baseAssetSymbol,
+ quoteAsset: 'USD',
+ stepSize: '',
+ tickSize: '',
+ indexPrice: pipToDecimal(indexPrice),
+ indexPrice24h: '',
+ indexPricePercentChange: '',
+ lastFundingRate: null,
+ currentFundingRate: null,
+ nextFundingTime: 0,
+ makerOrderMinimum: '',
+ takerOrderMinimum: '',
+ marketOrderExecutionPriceLimit: '',
+ limitOrderExecutionPriceLimit: '',
+ minimumPositionSize: '',
+ maximumPositionSize: pipToDecimal(leverageParameters.maximumPositionSize),
+ initialMarginFraction: pipToDecimal(leverageParameters.initialMarginFraction),
+ maintenanceMarginFraction: pipToDecimal(leverageParameters.maintenanceMarginFraction),
+ basePositionSize: pipToDecimal(leverageParameters.basePositionSize),
+ incrementalPositionSize: pipToDecimal(leverageParameters.incrementalPositionSize),
+ incrementalInitialMarginFraction: pipToDecimal(leverageParameters.incrementalInitialMarginFraction),
+ makerFeeRate: '',
+ takerFeeRate: '',
+ volume24h: '',
+ trades24h: 0,
+ openInterest: '',
+ });
+ const createLeverageDetails = (leverageParameters) => ({
+ maximumPositionSize: pipToDecimal(leverageParameters.maximumPositionSize),
+ initialMarginFraction: pipToDecimal(leverageParameters.initialMarginFraction),
+ maintenanceMarginFraction: pipToDecimal(leverageParameters.maintenanceMarginFraction),
+ basePositionSize: pipToDecimal(leverageParameters.basePositionSize),
+ incrementalPositionSize: pipToDecimal(leverageParameters.incrementalPositionSize),
+ incrementalInitialMarginFraction: pipToDecimal(leverageParameters.incrementalInitialMarginFraction),
+ });
function makeAMarket(
indexPrice: bigint,
baseAssetSymbol = 'FOO',
leverageParameters: orderbook.LeverageParametersBigInt = defaultLeverageParameters,
): IDEXMarket {
// All empty values are not used by the functions under test
const marketDetails = createMarketDetails(baseAssetSymbol, indexPrice);
const leverageDetails = createLeverageDetails(leverageParameters);
return { ...marketDetails, ...leverageDetails };
}
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.
function makeAMarket( | |
indexPrice: bigint, | |
baseAssetSymbol = 'FOO', | |
leverageParameters: orderbook.LeverageParametersBigInt = defaultLeverageParameters, | |
): IDEXMarket { | |
// All empty values are not used by the functions under test | |
return { | |
market: `${baseAssetSymbol}-USD`, | |
type: 'perpetual', | |
status: 'active', | |
baseAsset: baseAssetSymbol, | |
quoteAsset: 'USD', | |
stepSize: '', | |
tickSize: '', | |
indexPrice: pipToDecimal(indexPrice), | |
indexPrice24h: '', | |
indexPricePercentChange: '', | |
lastFundingRate: null, | |
currentFundingRate: null, | |
nextFundingTime: 0, | |
makerOrderMinimum: '', | |
takerOrderMinimum: '', | |
marketOrderExecutionPriceLimit: '', | |
limitOrderExecutionPriceLimit: '', | |
minimumPositionSize: '', | |
maximumPositionSize: pipToDecimal(leverageParameters.maximumPositionSize), | |
initialMarginFraction: pipToDecimal( | |
leverageParameters.initialMarginFraction, | |
), | |
maintenanceMarginFraction: pipToDecimal( | |
leverageParameters.maintenanceMarginFraction, | |
), | |
basePositionSize: pipToDecimal(leverageParameters.basePositionSize), | |
incrementalPositionSize: pipToDecimal( | |
leverageParameters.incrementalPositionSize, | |
), | |
incrementalInitialMarginFraction: pipToDecimal( | |
leverageParameters.incrementalInitialMarginFraction, | |
), | |
makerFeeRate: '', | |
takerFeeRate: '', | |
volume24h: '', | |
trades24h: 0, | |
openInterest: '', | |
}; | |
const createMarketDetails = (baseAssetSymbol, indexPrice) => ({ | |
market: `${baseAssetSymbol}-USD`, | |
type: 'perpetual', | |
status: 'active', | |
baseAsset: baseAssetSymbol, | |
quoteAsset: 'USD', | |
stepSize: '', | |
tickSize: '', | |
indexPrice: pipToDecimal(indexPrice), | |
indexPrice24h: '', | |
indexPricePercentChange: '', | |
lastFundingRate: null, | |
currentFundingRate: null, | |
nextFundingTime: 0, | |
makerOrderMinimum: '', | |
takerOrderMinimum: '', | |
marketOrderExecutionPriceLimit: '', | |
limitOrderExecutionPriceLimit: '', | |
minimumPositionSize: '', | |
maximumPositionSize: pipToDecimal(leverageParameters.maximumPositionSize), | |
initialMarginFraction: pipToDecimal(leverageParameters.initialMarginFraction), | |
maintenanceMarginFraction: pipToDecimal(leverageParameters.maintenanceMarginFraction), | |
basePositionSize: pipToDecimal(leverageParameters.basePositionSize), | |
incrementalPositionSize: pipToDecimal(leverageParameters.incrementalPositionSize), | |
incrementalInitialMarginFraction: pipToDecimal(leverageParameters.incrementalInitialMarginFraction), | |
makerFeeRate: '', | |
takerFeeRate: '', | |
volume24h: '', | |
trades24h: 0, | |
openInterest: '', | |
}); | |
const createLeverageDetails = (leverageParameters) => ({ | |
maximumPositionSize: pipToDecimal(leverageParameters.maximumPositionSize), | |
initialMarginFraction: pipToDecimal(leverageParameters.initialMarginFraction), | |
maintenanceMarginFraction: pipToDecimal(leverageParameters.maintenanceMarginFraction), | |
basePositionSize: pipToDecimal(leverageParameters.basePositionSize), | |
incrementalPositionSize: pipToDecimal(leverageParameters.incrementalPositionSize), | |
incrementalInitialMarginFraction: pipToDecimal(leverageParameters.incrementalInitialMarginFraction), | |
}); | |
function makeAMarket( | |
indexPrice: bigint, | |
baseAssetSymbol = 'FOO', | |
leverageParameters: orderbook.LeverageParametersBigInt = defaultLeverageParameters, | |
): IDEXMarket { | |
// All empty values are not used by the functions under test | |
const marketDetails = createMarketDetails(baseAssetSymbol, indexPrice); | |
const leverageDetails = createLeverageDetails(leverageParameters); | |
return { ...marketDetails, ...leverageDetails }; | |
} |
This PR was generated automatically. Merging it will transition alpha into beta.
Summary by CodeRabbit
New Features
Bug Fixes
indexPrice
property is correctly assigned, enhancing the accuracy of trading data.Tests
expect
statements for better readability and maintainability.