Skip to content
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

Update pow10 functions #1622

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Update pow10 functions #1622

merged 1 commit into from
Jun 11, 2024

Conversation

BrendanChou
Copy link
Contributor

@BrendanChou BrendanChou commented Jun 3, 2024

Changelist

  • Replace func BigMulPow10(val *big.Int, exponent int32) *big.Rat and func BigPow10(exponent uint64) *big.Int with BigPow10(int exponent) (*big.Int, bool) and BigIntMulPow10(input *big.Int, exponent int, roundUp bool) *big.Int
  • Try and simplify the rest of the code with these new functions

Test Plan

  • Added, removed, or modified unit tests

Summary by CodeRabbit

  • Refactor

    • Improved efficiency and accuracy of various mathematical operations involving exponentiation and multiplication by powers of 10.
    • Enhanced logic for converting between different units and handling scaling factors.
    • Unified and simplified conversion functions across multiple modules.
  • Tests

    • Updated and added new test cases to ensure correct behavior of refactored functions and calculations.
    • Integrated test cases for better coverage and maintainability.
  • New Features

    • Introduced more precise and robust handling of big integer operations in financial calculations.

Copy link
Contributor

coderabbitai bot commented Jun 3, 2024

Walkthrough

The recent updates primarily focus on enhancing the precision and efficiency of mathematical operations involving powers of 10 across various components. This includes refactoring functions for exponentiation and multiplication, adjusting test cases, and simplifying conversion logic between different numerical representations. The changes aim to improve accuracy and performance, particularly in financial calculations and asset conversions.

Changes

File/Path Change Summary
protocol/daemons/pricefeed/client/price_function/util.go Modified reverseShiftBigFloatSlice to use lib.BigPow10 with new variables for exponent calculation.
protocol/daemons/pricefeed/client/price_function/util_test.go Integrated TestReverseShiftBigFloatWithPow10 test cases into TestReverseShiftBigFloatSlice.
protocol/lib/big_math.go Refactored BigPow10 and BigIntMulPow10 functions to handle exponentiation and multiplication by powers of 10 differently.
protocol/lib/big_math_test.go Modified TestBigPow10 and TestBigMulPow10 to accommodate changes in exponent parameter types and expected results.
protocol/lib/quantums.go Adjusted conversion logic between base and quote quantums using the new BigPow10 function.
protocol/testutil/big/big.go Updated Int64MulPow10 to use lib.BigIntMulPow10 with revised arguments.
protocol/testutil/perpetuals/perpetuals.go Refactored MustHumanSizeToBaseQuantums to use lib.BigIntMulPow10 for calculations.
protocol/testutil/perpetuals/perpetuals_test.go Added new test cases for TestMustHumanSizeToBaseQuantums with different scaling factors.
protocol/testutil/prices/market_param_price.go Refactored MustHumanPriceToMarketPrice to use lib.BigIntMulPow10 for calculations.
protocol/testutil/prices/market_param_price_test.go Added new test cases for price conversion in TestMustHumanPriceToMarketPrice.
protocol/x/assets/keeper/asset.go Simplified ConvertAssetToCoin logic for asset to coin conversion.
protocol/x/clob/keeper/liquidations.go Revised ConvertFillablePriceToSubticks to handle exponentiation differently.
protocol/x/clob/types/price_to_subticks.go Refactored PriceToSubticks and SubticksToPrice to use custom logic for exponentiation and conversion.
protocol/x/clob/types/quantums.go Simplified FillAmountToQuoteQuantums and GetAveragePriceSubticks logic for value calculation.
protocol/x/prices/keeper/market_price.go Refactored UpdateMarketPrices to use BigPow10 and BigMul for price calculation and conversion.
protocol/x/ratelimit/types/params.go Updated calculation method for BigBaselineMinimum1Hr and BigBaselineMinimum1Day using lib.BigIntMulPow10.

Poem

In the realm of numbers, swift and bright,
Powers of ten now take their flight.
With code refactored, precise and clear,
Financial math we hold dear.
From assets to coins, conversions flow,
In this digital dance, our numbers grow.
🎉🚀


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@BrendanChou BrendanChou marked this pull request as ready for review June 3, 2024 20:46
@BrendanChou BrendanChou requested a review from roy-dydx June 3, 2024 20:46
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Outside diff range and nitpick comments (3)
protocol/x/clob/keeper/liquidations.go (3)

Line range hint 35-35: Resolve the undefined reference to Keeper.

It appears that the Keeper struct is not defined within this file or imported from another package. This could be a result of an incomplete code snippet or a missing import statement. Please ensure that the Keeper struct is correctly defined or imported to resolve this issue.


Line range hint 164-164: Ensure consistency in the definition or import of Keeper.

Similar to the previous comment, the Keeper struct is referenced multiple times in the file but seems to be undefined or not imported. This issue needs to be addressed to ensure the file compiles without errors.

Also applies to: 197-197


Line range hint 692-692: Address the undefined function getFillQuoteQuantums.

The function getFillQuoteQuantums is called but not defined or imported in this file. This could lead to runtime errors. Please define this function or ensure it is correctly imported from the appropriate module.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 86eb47d and 2f2ba76.

Files selected for processing (16)
  • protocol/daemons/pricefeed/client/price_function/util.go (1 hunks)
  • protocol/daemons/pricefeed/client/price_function/util_test.go (1 hunks)
  • protocol/lib/big_math.go (1 hunks)
  • protocol/lib/big_math_test.go (1 hunks)
  • protocol/lib/quantums.go (2 hunks)
  • protocol/testutil/big/big.go (1 hunks)
  • protocol/testutil/perpetuals/perpetuals.go (2 hunks)
  • protocol/testutil/perpetuals/perpetuals_test.go (1 hunks)
  • protocol/testutil/prices/market_param_price.go (2 hunks)
  • protocol/testutil/prices/market_param_price_test.go (1 hunks)
  • protocol/x/assets/keeper/asset.go (1 hunks)
  • protocol/x/clob/keeper/liquidations.go (1 hunks)
  • protocol/x/clob/types/price_to_subticks.go (2 hunks)
  • protocol/x/clob/types/quantums.go (2 hunks)
  • protocol/x/prices/keeper/market_price.go (1 hunks)
  • protocol/x/ratelimit/types/params.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • protocol/testutil/prices/market_param_price_test.go
Additional context used
golangci-lint
protocol/x/ratelimit/types/params.go

28-28: undefined: Limiter


34-34: undefined: Limiter


41-41: undefined: LimitParams


52-52: undefined: LimitParams


42-42: undefined: LimitParams


44-44: undefined: Limiter


59-59: undefined: ErrInvalidRateLimitPeriod


63-63: undefined: ErrInvalidBaselineMinimum


67-67: undefined: ErrInvalidBaselineTvlPpm

protocol/x/clob/types/quantums.go

29-29: undefined: Subticks

protocol/x/clob/types/price_to_subticks.go

28-28: undefined: ClobPair


66-66: undefined: Subticks


68-68: undefined: ClobPair

protocol/x/prices/keeper/market_price.go

25-25: undefined: Keeper


30-30: undefined: Keeper


118-118: undefined: Keeper


102-102: undefined: GenerateMarketPriceUpdateIndexerEvents

protocol/daemons/pricefeed/client/price_function/util.go

16-16: undefined: validator


62-62: undefined: validator


72-72: undefined: validator

protocol/x/clob/keeper/liquidations.go

35-35: undefined: Keeper


164-164: undefined: Keeper


197-197: undefined: Keeper


692-692: undefined: getFillQuoteQuantums

Additional comments not posted (44)
protocol/testutil/perpetuals/perpetuals_test.go (1)

20-21: The new test cases enhance coverage for edge cases.

The added test cases for MustHumanSizeToBaseQuantums are well-formed and improve the robustness of the testing suite.

protocol/testutil/prices/market_param_price.go (1)

91-107: The updated function enhances clarity and maintainability.

The changes to MustHumanPriceToMarketPrice correctly utilize the new BigIntMulPow10 function, improving the clarity and maintainability of the code.

protocol/lib/quantums.go (2)

43-48: Refactored to use BigPow10 for exponentiation, simplifying the logic and improving readability.


78-82: Successfully refactored to utilize BigPow10 for handling exponentiation, enhancing code clarity and maintainability.

protocol/testutil/perpetuals/perpetuals.go (1)

100-109: Refactored to use BigIntMulPow10 for converting human-readable size to quantums, simplifying the logic and ensuring consistency with the main application code.

protocol/x/prices/keeper/market_price.go (1)

68-75: Refactored to use BigPow10 for handling exponentiation during market price updates, aligning with the new standard for handling powers of ten across the codebase.

protocol/x/assets/keeper/asset.go (1)

302-312: Successfully refactored to use BigPow10 for exponentiation in asset-to-coin conversion, enhancing the clarity and maintainability of the code.

protocol/daemons/pricefeed/client/price_function/util.go (1)

147-157: Refactored to use BigPow10 for handling exponentiation in the reversal of big float slices, aligning with the new standard for handling powers of ten across the codebase.

protocol/lib/big_math.go (1)

35-67: Implemented BigPow10 and BigIntMulPow10 using generics to handle exponentiation and multiplication by powers of ten flexibly across different integer types. This central change supports the simplification efforts seen throughout the codebase.

protocol/daemons/pricefeed/client/price_function/util_test.go (9)

Line range hint 1-1: Ensure the package and imports are correctly defined and necessary.


Line range hint 14-14: Validate the constant definitions for accuracy and relevance to the tests.


Line range hint 20-20: Check the error message for clarity and correctness in positiveTagValidationError.


Line range hint 22-22: Review the test function TestIsExchangeError_Mixed for logic and correctness.


Line range hint 42-42: Examine the test function TestGetApiResponseValidator_validatePositiveNumericString_Mixed for proper validation logic and error handling.


Line range hint 64-64: Assess the test function TestGetOnlyTickerAndExponent for correct handling of different scenarios and error conditions.


Line range hint 86-86: Evaluate the test function TestGetUint64MedianFromShiftedBigFloatValues for correct median calculation and error handling.


Line range hint 108-108: Review the test function TestReverseShiftBigFloatSlice for correct functionality and precision handling.


Line range hint 130-130: Check the test function TestConvertFloat64ToString for correct string conversion and formatting.

protocol/lib/big_math_test.go (25)

Line range hint 1-1: Ensure the package and imports are correctly defined and necessary.


Line range hint 10-10: Review the benchmark function BenchmarkBigI for correct functionality and performance testing.


Line range hint 18-18: Evaluate the benchmark function BenchmarkBigU for correct functionality and performance testing.


Line range hint 26-26: Check the test function TestBigI for correct integer conversion.


Line range hint 34-34: Assess the test function TestBigU for correct unsigned integer conversion.


Line range hint 42-42: Review the benchmark function BenchmarkBigMulPpm_RoundDown for correct functionality and performance testing.


Line range hint 50-50: Evaluate the benchmark function BenchmarkBigMulPpm_RoundUp for correct functionality and performance testing.


Line range hint 58-58: Check the test function TestBigMulPpm for correct multiplication and rounding behavior.


149-171: Review the test function TestBigPow10 for correct power calculation and inverse flag behavior.


179-194: Evaluate the test functions TestBigPow10AllValuesInMemo and TestBigPow10AllValueNotInMemo for correct memoization and power calculation.


Line range hint 202-202: Assess the test function TestBigIntMulPpm for correct multiplication and precision handling.


Line range hint 230-230: Check the test functions TestBigMin and TestBigMax for correct minimum and maximum calculations.


Line range hint 258-258: Review the test function TestBigRatMulPpm for correct rational number multiplication and precision handling.


Line range hint 286-286: Evaluate the benchmark function BenchmarkBigRatMulPpm for correct functionality and performance testing.


Line range hint 294-294: Check the test function TestBigRatClamp for correct clamping behavior.


Line range hint 322-322: Assess the test function TestBigIntClamp for correct clamping behavior.


Line range hint 350-350: Evaluate the benchmark function BenchmarkBigDivCeil for correct functionality and performance testing.


Line range hint 358-358: Check the test function TestBigDivCeil for correct division and ceiling behavior.


Line range hint 386-386: Review the test function TestBigRatRound for correct rounding behavior.


Line range hint 414-414: Evaluate the test function TestBigIntRoundToMultiple for correct rounding to multiple behavior.


Line range hint 442-442: Assess the test functions TestBigInt0, TestBigFloat0, TestBigFloatMaxUint64, TestBigRat0, and TestBigRat1 for correct zero and one value initializations.


Line range hint 460-460: Check the test function TestBigUint64Clamp for correct clamping behavior.


Line range hint 488-488: Evaluate the test function TestBigInt32Clamp for correct clamping behavior.


Line range hint 516-516: Review the test function TestMustConvertBigIntToInt32 for correct conversion and error handling.


Line range hint 544-544: Assess the test function TestBigRatRoundToNearestMultiple for correct rounding to nearest multiple behavior.

protocol/x/clob/keeper/liquidations.go (1)

1057-1070: Refactor ConvertFillablePriceToSubticks to use the new BigPow10 function.

The refactoring of ConvertFillablePriceToSubticks to use the new BigPow10 function from lib is a good improvement. It simplifies the handling of exponentiation and makes the code more readable and maintainable. Ensure that the new function behaves as expected in all edge cases, especially with negative or zero exponents.

protocol/x/ratelimit/types/params.go Show resolved Hide resolved
protocol/x/clob/types/quantums.go Show resolved Hide resolved
protocol/x/clob/types/price_to_subticks.go Show resolved Hide resolved
power10Exponent := BigPow10(uint64(AbsInt32(exponent)))
if exponent > 0 {
result.Quo(result, power10Exponent)
p10, inverse := BigPow10(exponent)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you aren't using BigIntMulPow10 here?

@BrendanChou BrendanChou merged commit b431e04 into main Jun 11, 2024
17 of 19 checks passed
@BrendanChou BrendanChou deleted the bc/math/pow10 branch June 11, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants