Create a Math Library for Combining Pyth Prices on Solidity #1746
Conversation
|
@0xHaku is attempting to deploy a commit to the pyth-web Team on Vercel. A member of the Team first needs to authorize it. |
| * @param discount_exponent The exponent to apply to the discounts | ||
| * @return The valuation price of the collateral | ||
| */ | ||
| function get_collateral_valuation_price( |
| * @return The valuation price of the collateral | ||
| */ | ||
| function get_collateral_valuation_price( | ||
| PythStructs.Price memory self, |
There was a problem hiding this comment.
better name than self? maybe price?
There was a problem hiding this comment.
While "price" might be a better fit according to general function naming conventions, using "self" makes sense if we assume the use of the using PriceLib for PythStructs.Price; syntax (as the first argument is omitted).
Which one do you prefer?
| int32 midpriceExpo = base.expo - other.expo + PD_EXPO; | ||
|
|
||
| uint64 otherConfidencePct = (other.conf * PD_SCALE) / otherPrice; | ||
| uint128 conf = ((uint128(base.conf) * PD_SCALE) / otherPrice) + |
There was a problem hiding this comment.
i think solidity handles casting uints up, but make sure? generally need some tests to make sure this all works
| * @return The resulting price | ||
| */ | ||
| function cmul( | ||
| PythStructs.Price memory self, |
There was a problem hiding this comment.
again rename this to something clearer, like price
| */ | ||
| function mul( | ||
| PythStructs.Price memory self, | ||
| PythStructs.Price memory other |
| * @param b The second value | ||
| * @return The minimum of a and b | ||
| */ | ||
| function min(uint64 a, uint64 b) internal pure returns (uint64) { |
There was a problem hiding this comment.
don't use this, use Math.min from openzeppelin
| * @param b The second value | ||
| * @return The minimum of a and b | ||
| */ | ||
| function min(uint256 a, uint256 b) internal pure returns (uint256) { |
| function min(uint256 a, uint256 b) internal pure returns (uint256) { | ||
| return a < b ? a : b; | ||
| } | ||
| } |
There was a problem hiding this comment.
@anihamde
I referred to forge-test/PythTestUtils.t.sol for how to write forge-test.
This test code seems to be testing the code stored in the @pythnetwork/pyth-sdk-solidity library.
However, my code is not included in the package, so I cannot use this notation.
Should I import the test target file by traversing the local directory, or should I incorporate the implementation into the package and then import the target file?
Please advise on the correct procedure.
There was a problem hiding this comment.
You can just write unit tests for this library. I would recommend using a Harness to access the methods of the library. You could write a Harness to inherit PythStructs.Price, and then do using PriceLib for PriceHarness to write all the test cases. Let me know if that makes sense?
There was a problem hiding this comment.
@anihamde
I have created a test contract that encompasses the harness and performed a series of tests, so please review it.
There was a problem hiding this comment.
This looks better. I would say it still needs a lot more tests, and there are some existing issues I pointed out. In particular on the point about needing more tests, I would like to see tests around getBorrowValuationPrice, getCollateralValuationPrice, but also would be good to see a lot more tests for the primitive ops like mul, div, etc. See how many tests are here for reference--doesn't need to be that many, but really would be good to check edge cases and make sure behavior is as intended. This shouldn't be too hard, but it is important
| uint64 midprice = (basePrice * PD_SCALE) / otherPrice; | ||
| int32 midpriceExpo = base.expo - price2.expo + PD_EXPO; | ||
|
|
||
| uint64 otherConfidencePct = (price2.conf * PD_SCALE) / otherPrice; |
There was a problem hiding this comment.
maybe some comments here to explain the math, similar to what's in the rust sdk? o/w it's close to impossible for a first-time reader of the code to understand what is going on here
| uint64 midprice = basePrice * otherPrice; | ||
| int32 midpriceExpo = base.expo + price2.expo; | ||
|
|
||
| uint64 conf = base.conf * otherPrice + price2.conf * basePrice; |
There was a problem hiding this comment.
same comment here as in div function: some comments to explain the math would be great
| }); | ||
| PythStructs.Price memory result = harness.mul(price1, price2); | ||
| assertEq(result.price, 5000, "Multiplication result should be 5000"); | ||
| assertEq(result.expo, -16, "Exponents should be added"); |
There was a problem hiding this comment.
assert the confidence is as expected?
| 5, | ||
| -9 | ||
| ); | ||
| assertEq(result.price, 10 ** 7, "Affine combination should be 150"); |
There was a problem hiding this comment.
this assert message is wrong. i think this test case is wrong too? should be 200 for y2, i think?
Overview
This pull request includes the implementation of the
get_collateral_valuation_price,get_borrow_valuation_price, andget_price_in_quotefunctions in Solidity, along with their supporting functionsaffine_combinationandfraction.These functions were ported from their original Rust implementations to Solidity to ensure compatibility and functionality within our Solidity-based smart contract environment.
Changes Made
get_collateral_valuation_price
Ported the logic to calculate the valuation of a collateral position based on the amount deposited, using a linear interpolation between initial and final discount rates.
Ensured the function handles the correct scaling and precision of rates and amounts.
get_borrow_valuation_price
Ported the logic to calculate the valuation of a borrow position based on the amount borrowed, using a linear interpolation between initial and final premium rates.
Ensured the function maintains correct scaling and precision for the premiums.
get_price_in_quote
Implemented the function to convert the price of one token into the price of another token (quote).
Ensured the function accurately handles conversions and maintains precision.
Supporting Functions
Implemented
affine_combinationto perform affine combination of two prices.Implemented
fractionto create fractions of prices with appropriate precision handling.Documentation
In-code comments were added to explain the logic and assumptions behind each function.
Additional Notes
Ensure that Solidity version compatibility is checked before merging.
Peer review and additional testing are recommended to validate the correctness of the ported functions.