Reduce usage of big.Rat #1553
Reduce usage of big.Rat #1553
Conversation
|
Important Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes focus on improving mathematical operations involving big integers within the protocol. Key updates include the introduction of a new division and rounding function, Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- protocol/lib/big_math.go (3 hunks)
- protocol/lib/big_math_test.go (2 hunks)
- protocol/lib/quantums.go (1 hunks)
- protocol/x/assets/keeper/asset.go (1 hunks)
- protocol/x/clob/keeper/clob_pair.go (1 hunks)
- protocol/x/clob/keeper/liquidations.go (1 hunks)
- protocol/x/clob/keeper/mev.go (1 hunks)
- protocol/x/clob/keeper/untriggered_conditional_orders.go (2 hunks)
- protocol/x/clob/memclob/memclob.go (1 hunks)
- protocol/x/clob/types/price_to_subticks.go (1 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (1 hunks)
- protocol/x/perpetuals/types/liquidity_tier.go (2 hunks)
- protocol/x/ratelimit/util/capacity.go (1 hunks)
- protocol/x/rewards/keeper/keeper.go (1 hunks)
- protocol/x/vault/keeper/orders.go (2 hunks)
- protocol/x/vest/keeper/keeper.go (1 hunks)
Additional Context Used
Path-based Instructions (16)
protocol/x/clob/types/price_to_subticks.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/ratelimit/util/capacity.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/lib/quantums.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/perpetuals/types/liquidity_tier.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vest/keeper/keeper.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/assets/keeper/asset.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/lib/big_math.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/rewards/keeper/keeper.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/orders.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/untriggered_conditional_orders.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/clob_pair.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/lib/big_math_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/mev.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/liquidations.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/perpetuals/keeper/perpetual.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/memclob/memclob.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (18)
protocol/x/clob/types/price_to_subticks.go (1)
74-81: The changes inSubticksToPricefunction correctly replacebig.Ratwithbig.Intoperations, aligning with the PR's objective to reducebig.Ratusage. Ensure the rounding behavior is as expected.protocol/x/ratelimit/util/capacity.go (1)
66-70: The changes inCalculateNewCapacityListfunction correctly replacebig.Ratwithbig.Intoperations, aligning with the PR's objective to reducebig.Ratusage. Ensure the new capacity calculations behave as expected.Verification successful
The
capacity_test.gofile contains comprehensive tests for theCalculateNewCapacityListfunction, covering various scenarios such as different baseline and capacity values, rounding behavior, and edge cases. These tests ensure that the new capacity calculations behave as expected.
capacity_test.goincludes tests for:
- Previous capacity equals baseline.
- Previous capacity less than baseline.
- Previous capacity with 18 decimals.
- Previous capacity equals zero.
- Previous capacity equals zero with rounding down.
- Previous capacity equals twice the baseline with rounding down.
- Previous capacity between baseline and twice the baseline.
- Previous capacity greater than twice the baseline with capacity less than baseline.
These tests validate the correctness of the changes made to the
CalculateNewCapacityListfunction.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the behavior of new capacity calculations in `CalculateNewCapacityList` function. # Test: Execute the function with known inputs and check if the capacity calculations are correct. # Expect: The output should match the expected capacity values. echo "Testing new capacity calculations..."Length of output: 82
Script:
#!/bin/bash # Locate the `CalculateNewCapacityList` function definition. ast-grep --lang go --pattern 'func CalculateNewCapacityList($_, $_, $_) $_' # Search for test files that might contain tests for `CalculateNewCapacityList`. fd --type f --name '*_test.go' --exec grep -l 'CalculateNewCapacityList' {}Length of output: 5385
Script:
#!/bin/bash # Correct the `fd` command to search for test files that might contain tests for `CalculateNewCapacityList`. fd --type f -e go -x grep -l 'CalculateNewCapacityList' {}Length of output: 185
Script:
#!/bin/bash # Extract and review the relevant tests from `capacity_test.go`. ast-grep --lang go --pattern 'func TestCalculateNewCapacityList($_) $_' ./protocol/x/ratelimit/util/capacity_test.goLength of output: 20835
protocol/lib/quantums.go (1)
89-93: The changes inQuoteToBaseQuantumsfunction correctly replacebig.Ratwithbig.Intoperations, aligning with the PR's objective to reducebig.Ratusage. Ensure the rounding behavior is as expected.Verification successful
The test cases in
quantums_test.gofor theQuoteToBaseQuantumsfunction cover various scenarios, including positive, negative, and realistic values. These test cases should adequately verify the rounding behavior using Euclidean division.
quantums_test.gocontains multiple test cases forQuoteToBaseQuantums:
- Converts from base to quote quantums
- Correctly converts negative value
- priceExponent is negative
- priceExponent is zero
- Realistic values for BTC at different prices
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the rounding behavior of `QuoteToBaseQuantums` function. # Test: Execute the function with known inputs and check if the rounding is correct. # Expect: The output should match the expected rounded values. echo "Testing QuoteToBaseQuantums function rounding behavior..."Length of output: 124
Script:
#!/bin/bash # Locate the `QuoteToBaseQuantums` function implementation ast-grep --lang go --pattern 'func QuoteToBaseQuantums($_, $_) $_ { $$$ }' # Locate test files that might contain tests for `QuoteToBaseQuantums` fd --type f --name '*_test.go' --exec grep -l 'QuoteToBaseQuantums' {}Length of output: 402
Script:
#!/bin/bash # Locate the `QuoteToBaseQuantums` function implementation ast-grep --lang go --pattern 'func QuoteToBaseQuantums($_, $_) $_ { $$$ }' # Locate test files that might contain tests for `QuoteToBaseQuantums` fd -e go -x grep -l 'QuoteToBaseQuantums' {}Length of output: 228
Script:
#!/bin/bash # Extract and review test cases for `QuoteToBaseQuantums` from `quantums_test.go` grep -A 10 'QuoteToBaseQuantums' ./protocol/lib/quantums_test.goLength of output: 800
Script:
#!/bin/bash # Extract the complete `TestQuoteToBaseQuantums` function from `quantums_test.go` ast-grep --lang go --pattern 'func TestQuoteToBaseQuantums($_) $_ { $$$ }' ./protocol/lib/quantums_test.goLength of output: 110
Script:
#!/bin/bash # Extract a larger portion of the `quantums_test.go` file around the `TestQuoteToBaseQuantums` function grep -A 50 'func TestQuoteToBaseQuantums' ./protocol/lib/quantums_test.goLength of output: 2042
protocol/x/perpetuals/types/liquidity_tier.go (1)
120-124: The changes inGetInitialMarginQuoteQuantumsfunction correctly replacebig.Ratwithbig.Intoperations, aligning with the PR's objective to reducebig.Ratusage. Ensure the rounding behavior is as expected.Verification successful
The test cases in
liquidity_tier_test.gothoroughly cover various scenarios for theGetInitialMarginQuoteQuantumsfunction, including different initial margin percentages and rounding behaviors. The function implementation and test cases align with the expected behavior of rounding up as specified.
protocol/x/perpetuals/types/liquidity_tier_test.go: Contains detailed test cases forGetInitialMarginQuoteQuantums, ensuring correct rounding behavior.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the rounding behavior of `GetInitialMarginQuoteQuantums` function. # Test: Execute the function with known inputs and check if the rounding is correct. # Expect: The output should match the expected rounded values. echo "Testing GetInitialMarginQuoteQuantums function rounding behavior..."Length of output: 144
Script:
#!/bin/bash # Locate the implementation of the GetInitialMarginQuoteQuantums function echo "Searching for GetInitialMarginQuoteQuantums function implementation..." ast-grep --lang go --pattern 'func GetInitialMarginQuoteQuantums($_) $_' > function_implementation.txt # Locate test cases for GetInitialMarginQuoteQuantums function echo "Searching for test cases for GetInitialMarginQuoteQuantums function..." rg 'GetInitialMarginQuoteQuantums' --files-with-matches > test_cases.txt # Display the results echo "Function implementation found in:" cat function_implementation.txt echo "Test cases found in:" cat test_cases.txtLength of output: 754
Script:
#!/bin/bash # Extract the GetInitialMarginQuoteQuantums function implementation echo "Extracting GetInitialMarginQuoteQuantums function implementation..." ast-grep --lang go --pattern 'func GetInitialMarginQuoteQuantums($_) $_' protocol/x/perpetuals/types/liquidity_tier.go > function_implementation.txt # Extract the test cases for GetInitialMarginQuoteQuantums function echo "Extracting test cases for GetInitialMarginQuoteQuantums function..." rg 'GetInitialMarginQuoteQuantums' protocol/x/perpetuals/keeper/perpetual.go protocol/x/perpetuals/types/liquidity_tier_test.go > test_cases.txt # Display the results echo "Function implementation:" cat function_implementation.txt echo "Test cases:" cat test_cases.txtLength of output: 1204
Script:
#!/bin/bash # Extract the TestGetInitialMarginQuoteQuantums function echo "Extracting TestGetInitialMarginQuoteQuantums function..." ast-grep --lang go --pattern 'func TestGetInitialMarginQuoteQuantums($_) $_' protocol/x/perpetuals/types/liquidity_tier_test.go > test_function.txt # Extract related test cases in perpetual.go echo "Extracting related test cases in perpetual.go..." rg 'GetInitialMarginQuoteQuantums' protocol/x/perpetuals/keeper/perpetual.go > related_test_cases.txt # Display the results echo "Test function implementation:" cat test_function.txt echo "Related test cases:" cat related_test_cases.txtLength of output: 12092
protocol/x/vest/keeper/keeper.go (1)
108-114: The changes inProcessVestingfunction correctly replacebig.Ratwithbig.Intoperations, aligning with the PR's objective to reducebig.Ratusage. Ensure the rounding behavior is as expected.protocol/x/assets/keeper/asset.go (1)
308-312: The changes inConvertAssetToCoinfunction correctly replacebig.Ratwithbig.Intoperations, aligning with the PR's objective to reducebig.Ratusage. Ensure the rounding behavior is as expected.Verification successful
The test cases for the
ConvertAssetToCoinfunction inasset_test.goinclude both success and failure scenarios. The success test cases cover various combinations ofdenomExponent,atomicResolution, andquantumsToConvert, ensuring that the function behaves correctly under different conditions. The failure test cases check for invalid asset IDs, invalid denom exponents, and invalid atomic resolutions.These tests appear to adequately cover the rounding behavior and other edge cases for the
ConvertAssetToCoinfunction.
protocol/x/assets/keeper/asset_test.go: Contains comprehensive test cases forConvertAssetToCoin.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the rounding behavior of `ConvertAssetToCoin` function. # Test: Execute the function with known inputs and check if the rounding is correct. # Expect: The output should match the expected rounded values. echo "Testing ConvertAssetToCoin function rounding behavior..."Length of output: 122
Script:
#!/bin/bash # Locate the ConvertAssetToCoin function implementation ast-grep --lang go --pattern 'func ConvertAssetToCoin($_, $_) $_' # Locate test files related to the ConvertAssetToCoin function fd -e go -e testLength of output: 71358
Script:
#!/bin/bash # Locate the ConvertAssetToCoin function implementation with more specific search patterns rg 'func ConvertAssetToCoin' --type go # Locate test files that might contain tests for ConvertAssetToCoin function rg 'ConvertAssetToCoin' --type goLength of output: 2023
Script:
#!/bin/bash # Extract the ConvertAssetToCoin function implementation ast-grep --lang go --pattern 'func (k Keeper) ConvertAssetToCoin($_, $_, $_) ($_,$_,$_){$$$}' # Extract the test cases for ConvertAssetToCoin from asset_test.go rg 'ConvertAssetToCoin' protocol/x/assets/keeper/asset_test.goLength of output: 3899
Script:
#!/bin/bash # Extract the test cases for ConvertAssetToCoin from asset_test.go sed -n '/func TestConvertAssetToCoin_Success/,/}/p' protocol/x/assets/keeper/asset_test.go sed -n '/func TestConvertAssetToCoin_Failure/,/}/p' protocol/x/assets/keeper/asset_test.goLength of output: 1574
protocol/lib/big_math.go (1)
58-60: LGTM! Ensure all usages ofBigIntMulSignedPpmare updated to handle the new behavior.Verification successful
The implementation of
BigIntDivRoundappears to be correct and aligns with the intended functionality of rounding the division result based on theroundUpflag. Additionally, the usages ofBigIntMulSignedPpmin the various files (keeper.go,pending_updates.go,mev.go, andprocess_single_match.go) have been verified to handle the new behavior correctly.
protocol/x/rewards/keeper/keeper.goprotocol/x/clob/types/pending_updates.goprotocol/x/clob/keeper/mev.goprotocol/x/clob/keeper/process_single_match.goAll usages align with the new behavior of
BigIntMulSignedPpm.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `BigIntMulSignedPpm` handle the new behavior. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'BigIntMulSignedPpm'Length of output: 905
Script:
#!/bin/bash # Description: Verify the context and parameters of `BigIntMulSignedPpm` in `keeper.go`. # Test: Extract the relevant function and surrounding lines for context. rg --context 10 'BigIntMulSignedPpm' protocol/x/rewards/keeper/keeper.goLength of output: 863
Script:
#!/bin/bash # Description: Verify the context and parameters of `BigIntMulSignedPpm` in `pending_updates.go`. # Test: Extract the relevant function and surrounding lines for context. rg --context 10 'BigIntMulSignedPpm' protocol/x/clob/types/pending_updates.goLength of output: 406
Script:
#!/bin/bash # Description: Verify the context and parameters of `BigIntMulSignedPpm` in `mev.go`. # Test: Extract the relevant function and surrounding lines for context. rg --context 10 'BigIntMulSignedPpm' protocol/x/clob/keeper/mev.goLength of output: 653
Script:
#!/bin/bash # Description: Verify the context and parameters of `BigIntMulSignedPpm` in `process_single_match.go`. # Test: Extract the relevant function and surrounding lines for context. rg --context 10 'BigIntMulSignedPpm' protocol/x/clob/keeper/process_single_match.goLength of output: 968
Script:
#!/bin/bash # Description: Extract and review the implementation of `BigIntDivRound`. # Test: Search for the implementation of `BigIntDivRound`. rg --context 10 'func BigIntDivRound' protocol/lib/big_math.goLength of output: 753
protocol/x/rewards/keeper/keeper.go (1)
300-304: LGTM! Ensure integration with other modules is verified, especially where reward calculations are used.Verification successful
To ensure the integration of the changes in
ProcessRewardsForBlockis properly handled, we need to review the implementation inmodule.goandkeeper_test.go.
The integration of the changes in
ProcessRewardsForBlockappears to be properly handled inmodule.goand thoroughly tested inkeeper_test.go. TheEndBlockfunction callsProcessRewardsForBlockand handles errors, while the tests cover various scenarios to ensure correct behavior.
module.go: TheEndBlockfunction callsProcessRewardsForBlockand logs errors if they occur.keeper_test.go: The tests verify the behavior ofProcessRewardsForBlockunder different conditions, ensuring the new behavior is correctly handled.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration with other modules for changes in `ProcessRewardsForBlock`. # Test: Search for the function usage in other modules. Expect: Proper handling of the new behavior. rg --type go $'ProcessRewardsForBlock'Length of output: 665
Script:
#!/bin/bash # Description: Review the implementation of ProcessRewardsForBlock in module.go and keeper_test.go. # Display the relevant lines in module.go rg --type go --context 10 'ProcessRewardsForBlock' protocol/x/rewards/module.go # Display the relevant lines in keeper_test.go rg --type go --context 10 'ProcessRewardsForBlock' protocol/x/rewards/keeper/keeper_test.goLength of output: 2176
protocol/x/vault/keeper/orders.go (1)
204-211: LGTM! Ensure integration with other modules is verified, especially where order size calculations are used.Verification successful
The integration of the
GetVaultClobOrdersfunction appears to be properly handled in the following locations:
types.go: The function is part of theVaultKeeperinterface, ensuring it is correctly defined for use across the module.orders.go: The function is used in theRefreshVaultClobOrdersmethod to cancel and place orders, indicating it is integrated into the order management logic.orders_test.go: The function is extensively tested, with multiple test cases verifying its behavior under different conditions.These findings suggest that the changes to the
GetVaultClobOrdersfunction are well-integrated and thoroughly tested.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration with other modules for changes in `GetVaultClobOrders`. # Test: Search for the function usage in other modules. Expect: Proper handling of the new behavior. rg --type go $'GetVaultClobOrders'Length of output: 1332
Script:
#!/bin/bash # Description: Verify integration and proper handling of the new behavior in `GetVaultClobOrders`. # Check usage in `types.go` echo "Checking types.go..." rg --type go --context 5 'GetVaultClobOrders' protocol/x/vault/types/types.go # Check usage in `orders.go` echo "Checking orders.go..." rg --type go --context 5 'GetVaultClobOrders' protocol/x/vault/keeper/orders.go # Check usage in `orders_test.go` echo "Checking orders_test.go..." rg --type go --context 5 'GetVaultClobOrders' protocol/x/vault/keeper/orders_test.goLength of output: 4315
protocol/x/clob/keeper/untriggered_conditional_orders.go (2)
231-237: The changes from lines 231 to 237 correctly implement the transition frombig.Rattobig.Intfor pessimistic rounding in thePollTriggeredConditionalOrdersfunction. This is consistent with the PR's objective to reduce the usage ofbig.Rat.
254-260: The changes from lines 254 to 260 correctly implement the transition frombig.Rattobig.Intfor pessimistic rounding in thePollTriggeredConditionalOrdersfunction. This is consistent with the PR's objective to reduce the usage ofbig.Rat.protocol/x/clob/keeper/clob_pair.go (1)
407-412: The changes from lines 407 to 412 correctly implement the transition frombig.Rattobig.Intfor calculatingcurrentOraclePriceSubticksin thevalidateOrderAgainstClobPairStatusfunction. This is consistent with the PR's objective to reduce the usage ofbig.Rat.protocol/lib/big_math_test.go (1)
536-618: The test cases forBigIntDivRoundare comprehensive and well-implemented, covering a variety of scenarios including edge cases for zero, positive, and negative numbers, as well as different rounding directions.protocol/x/clob/keeper/mev.go (1)
384-388: The changes from lines 384 to 388 successfully replacebig.Ratoperations withbig.Intoperations, aligning with the PR's objective to reduce the usage ofbig.Rat. This should enhance performance by avoiding the overhead associated with rational number calculations.protocol/x/perpetuals/keeper/perpetual.go (1)
1028-1034: The changes from lines 1028 to 1034 update the calculation of maintenance margin requirements usingbig.Ratfor precision.This change aligns with the PR's objective to enhance precision in financial calculations by replacing
big.Ratwithbig.Intoperations. The use ofBigIntDivRoundfor rounding is appropriate here, ensuring that the division results are correctly rounded to the nearest integer, which is critical in financial calculations to avoid significant rounding errors.protocol/x/clob/memclob/memclob.go (3)
2467-2471: The rounding logic ingetImpactPriceSubticksis correctly implemented to round towards zero, which is appropriate for financial calculations where precision is crucial.
2472-2473: The functionresizeReduceOnlyMatchIfNecessaryeffectively ensures that reduce-only orders do not increase the position size, adhering to trading rules.
Line range hint
2474-2475: TheGetPricePremiumfunction is well-implemented with comprehensive checks and clear handling of different market scenarios to calculate the price premium accurately.
| roundedSubticksBig := lib.BigIntDivRound( | ||
| subticksRat.Num(), | ||
| subticksRat.Denom(), | ||
| roundUp, | ||
| ) |
There was a problem hiding this comment.
Refactor ConvertFillablePriceToSubticks to use BigIntDivRound for rounding.
The change from big.Rat to big.Int in the ConvertFillablePriceToSubticks function aligns with the PR's objective to enhance precision. The use of BigIntDivRound for rounding operations is a critical improvement, ensuring that the calculations are more consistent and reliable. This change is crucial for financial calculations where precision is paramount.
| value: big.NewRat(7, 1), | ||
| value: big.NewInt(7), | ||
| base: 14, | ||
| up: true, | ||
| expectedResult: 14, | ||
| }, | ||
| "Round 123 down to a multiple of 123": { | ||
| value: big.NewRat(123, 1), | ||
| value: big.NewInt(123), | ||
| base: 123, | ||
| up: false, | ||
| expectedResult: 123, | ||
| }, | ||
| "Round 123 up to a multiple of 123": { | ||
| value: big.NewRat(123, 1), | ||
| value: big.NewInt(123), | ||
| base: 123, | ||
| up: true, | ||
| expectedResult: 123, | ||
| }, | ||
| "Round 100/6 down to a multiple of 3": { | ||
| value: big.NewRat(100, 6), | ||
| "Round 16 down to a multiple of 3": { | ||
| value: big.NewInt(16), | ||
| base: 3, | ||
| up: false, | ||
| expectedResult: 15, | ||
| }, | ||
| "Round 100/6 up to a multiple of 3": { | ||
| value: big.NewRat(100, 6), | ||
| "Round 16 up to a multiple of 3": { | ||
| value: big.NewInt(16), | ||
| base: 3, | ||
| up: true, | ||
| expectedResult: 18, | ||
| }, | ||
| "Round 7/2 down to a multiple of 1": { | ||
| value: big.NewRat(7, 2), | ||
| "Round -16 down to a multiple of 3, is clamped to zero": { | ||
| value: big.NewInt(-16), | ||
| base: 3, | ||
| up: false, | ||
| expectedResult: 0, | ||
| }, | ||
| "Round -16 up to a multiple of 3, is clamped to zero": { | ||
| value: big.NewInt(-16), | ||
| base: 3, | ||
| up: true, | ||
| expectedResult: 0, | ||
| }, | ||
| "Round 4 down to a multiple of 1": { | ||
| value: big.NewInt(4), | ||
| base: 1, | ||
| up: false, | ||
| expectedResult: 3, | ||
| expectedResult: 4, | ||
| }, | ||
| "Round 7/2 up to a multiple of 1": { | ||
| value: big.NewRat(7, 2), | ||
| "Round 4 up to a multiple of 1": { | ||
| value: big.NewInt(4), | ||
| base: 1, | ||
| up: true, | ||
| expectedResult: 4, | ||
| }, | ||
| "Round 10 down to a multiple of 0": { | ||
| value: big.NewRat(10, 1), | ||
| value: big.NewInt(10), | ||
| base: 0, | ||
| up: false, | ||
| expectedResult: 0, | ||
| }, | ||
| "Round 10 up to a multiple of 0": { | ||
| value: big.NewRat(10, 1), | ||
| value: big.NewInt(10), | ||
| base: 0, | ||
| up: true, | ||
| expectedResult: 0, | ||
| }, | ||
| "Check overflow is clamped when rounding down": { | ||
| value: big_testutil.MustFirst( | ||
| new(big.Int).SetString("99999999999999999999", 10), | ||
| ), | ||
| base: 100, | ||
| up: false, | ||
| expectedResult: 18446744073709551600, | ||
| }, | ||
| "Check overflow is clamped when rounding up": { | ||
| value: big_testutil.MustFirst( | ||
| new(big.Int).SetString("99999999999999999999", 10), | ||
| ), | ||
| base: 100, | ||
| up: true, | ||
| expectedResult: 18446744073709551600, | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| result := lib.BigRatRoundToNearestMultiple( | ||
| result := lib.BigRoundToNearestMultiple( |
There was a problem hiding this comment.
The test cases for BigRoundToNearestMultiple are generally well-implemented. However, the cases for rounding to a multiple of zero (lines 996 and 1002) should be reconsidered, as rounding to a multiple of zero is not mathematically defined and could potentially lead to confusion or errors.
- "Round 10 down to a multiple of 0": {
- value: big.NewInt(10),
- base: 0,
- up: false,
- expectedResult: 0,
- },
- "Round 10 up to a multiple of 0": {
- value: big.NewInt(10),
- base: 0,
- up: true,
- expectedResult: 0,
- },Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func TestBigRoundToNearestMultiple(t *testing.T) { | |
| tests := map[string]struct { | |
| value *big.Rat | |
| value *big.Int | |
| base uint32 | |
| up bool | |
| expectedResult uint64 | |
| }{ | |
| "Round 5 down to a multiple of 2": { | |
| value: big.NewRat(5, 1), | |
| value: big.NewInt(5), | |
| base: 2, | |
| up: false, | |
| expectedResult: 4, | |
| }, | |
| "Round 5 up to a multiple of 2": { | |
| value: big.NewRat(5, 1), | |
| value: big.NewInt(5), | |
| base: 2, | |
| up: true, | |
| expectedResult: 6, | |
| }, | |
| "Round 7 down to a multiple of 14": { | |
| value: big.NewRat(7, 1), | |
| value: big.NewInt(7), | |
| base: 14, | |
| up: false, | |
| expectedResult: 0, | |
| }, | |
| "Round 7 up to a multiple of 14": { | |
| value: big.NewRat(7, 1), | |
| value: big.NewInt(7), | |
| base: 14, | |
| up: true, | |
| expectedResult: 14, | |
| }, | |
| "Round 123 down to a multiple of 123": { | |
| value: big.NewRat(123, 1), | |
| value: big.NewInt(123), | |
| base: 123, | |
| up: false, | |
| expectedResult: 123, | |
| }, | |
| "Round 123 up to a multiple of 123": { | |
| value: big.NewRat(123, 1), | |
| value: big.NewInt(123), | |
| base: 123, | |
| up: true, | |
| expectedResult: 123, | |
| }, | |
| "Round 100/6 down to a multiple of 3": { | |
| value: big.NewRat(100, 6), | |
| "Round 16 down to a multiple of 3": { | |
| value: big.NewInt(16), | |
| base: 3, | |
| up: false, | |
| expectedResult: 15, | |
| }, | |
| "Round 100/6 up to a multiple of 3": { | |
| value: big.NewRat(100, 6), | |
| "Round 16 up to a multiple of 3": { | |
| value: big.NewInt(16), | |
| base: 3, | |
| up: true, | |
| expectedResult: 18, | |
| }, | |
| "Round 7/2 down to a multiple of 1": { | |
| value: big.NewRat(7, 2), | |
| "Round -16 down to a multiple of 3, is clamped to zero": { | |
| value: big.NewInt(-16), | |
| base: 3, | |
| up: false, | |
| expectedResult: 0, | |
| }, | |
| "Round -16 up to a multiple of 3, is clamped to zero": { | |
| value: big.NewInt(-16), | |
| base: 3, | |
| up: true, | |
| expectedResult: 0, | |
| }, | |
| "Round 4 down to a multiple of 1": { | |
| value: big.NewInt(4), | |
| base: 1, | |
| up: false, | |
| expectedResult: 3, | |
| expectedResult: 4, | |
| }, | |
| "Round 7/2 up to a multiple of 1": { | |
| value: big.NewRat(7, 2), | |
| "Round 4 up to a multiple of 1": { | |
| value: big.NewInt(4), | |
| base: 1, | |
| up: true, | |
| expectedResult: 4, | |
| }, | |
| "Round 10 down to a multiple of 0": { | |
| value: big.NewRat(10, 1), | |
| value: big.NewInt(10), | |
| base: 0, | |
| up: false, | |
| expectedResult: 0, | |
| }, | |
| "Round 10 up to a multiple of 0": { | |
| value: big.NewRat(10, 1), | |
| value: big.NewInt(10), | |
| base: 0, | |
| up: true, | |
| expectedResult: 0, | |
| }, | |
| "Check overflow is clamped when rounding down": { | |
| value: big_testutil.MustFirst( | |
| new(big.Int).SetString("99999999999999999999", 10), | |
| ), | |
| base: 100, | |
| up: false, | |
| expectedResult: 18446744073709551600, | |
| }, | |
| "Check overflow is clamped when rounding up": { | |
| value: big_testutil.MustFirst( | |
| new(big.Int).SetString("99999999999999999999", 10), | |
| ), | |
| base: 100, | |
| up: true, | |
| expectedResult: 18446744073709551600, | |
| }, | |
| } | |
| for name, tc := range tests { | |
| t.Run(name, func(t *testing.T) { | |
| result := lib.BigRatRoundToNearestMultiple( | |
| result := lib.BigRoundToNearestMultiple( | |
| }, | |
| "Check overflow is clamped when rounding down": { | |
| value: big_testutil.MustFirst( | |
| new(big.Int).SetString("99999999999999999999", 10), | |
| ), | |
| base: 100, | |
| up: false, | |
| expectedResult: 18446744073709551600, | |
| }, | |
| "Check overflow is clamped when rounding up": { | |
| value: big_testutil.MustFirst( | |
| new(big.Int).SetString("99999999999999999999", 10), | |
| ), | |
| base: 100, | |
| up: true, | |
| expectedResult: 18446744073709551600, | |
| }, | |
| } | |
| for name, tc := range tests { | |
| t.Run(name, func(t *testing.T) { | |
| result := lib.BigRoundToNearestMultiple( |
protocol/lib/big_math.go
Outdated
| // BigIntDivRound takes a numerator, denominator, and a direction to round (true for up, false for down). | ||
| func BigIntDivRound( | ||
| numerator *big.Int, | ||
| denominator *big.Int, | ||
| roundUp bool, | ||
| ) *big.Int { | ||
| result, remainder := new(big.Int).DivMod(numerator, denominator, new(big.Int)) | ||
| if remainder.Sign() > 0 && roundUp { | ||
| result.Add(result, big.NewInt(1)) | ||
| } | ||
| return resultBig | ||
| return result |
There was a problem hiding this comment.
LGTM! Consider adding comprehensive tests to cover edge cases for BigIntDivRound.
Would you like me to help with writing these tests?
Changelist
Reduce usage of Rat functions
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
BigIntDivRoundfor more precise calculations.Improvements
BigIntDivRoundfor improved precision in financial calculations.Refactor