-
Notifications
You must be signed in to change notification settings - Fork 209
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
Implemented a Math Library to combine Pyth prices in Solidity - tested and working on Remix IDE #1732
Conversation
@lopeselio is attempting to deploy a commit to the pyth-web Team on Vercel. A member of the Team first needs to authorize it. |
Hey @ali-bahjati can you please help review this PR #1732 ? |
* @param x here is the signed integer. | ||
* @return The unsigned integer and sign bit. | ||
*/ | ||
function toUnsigned(int64 x) public pure returns (uint64, int64) { |
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.
this function can be internal. is it intended to be used for anything other than a helper method within this library?
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.
I agree, we can keep it internal pure
other = normalize(other); | ||
|
||
// If the price of the quote currency is zero, return zero | ||
if (other.price == 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.
I would suggest you throw an error here or return (bool validOutputPrice, PythStructs.Price memory outputPrice)
, to avoid any downstream issues with consumption of the 0 price. Being explicit here with the output would help protect users from incorrectly using an invalid price
|
||
// Compute the confidence interval | ||
uint64 otherConfidencePct = other.conf * PD_SCALE / otherPrice; | ||
uint128 conf = (base.conf * PD_SCALE / otherPrice) + (otherConfidencePct * midprice) / PD_SCALE; |
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.
this should be dividing the sum by PD_SCALE
, so you need to include parentheses here
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.
also i presume you want to cast each of the summands to uint128
so that you avoid any overflow in the sum. if so, you need to call uint128(base.conf * PD_SCALE / otherPrice)
and likewise for the other summand
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.
However, if you want to ensure that conf is less than the max uint64
value, just don't cast to uint128
. solidity has built-in overflow protection on summing post 0.8, so you won't need to worry about checking, it'll just throw an overflow error if the two summands sum to more than the max uint64 val
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.
I will make this change
int64(int64(midprice) * baseSign * otherSign), | ||
uint64(conf), | ||
midpriceExpo, | ||
self.publishTime < other.publishTime ? self.publishTime : other.publishTime |
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.
you can use OpenZeppelin's Math library for better readability: https://docs.openzeppelin.com/contracts/3.x/api/math#Math-min-uint256-uint256-
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.
Alright, I will look into this
PythStructs.Price memory result = div(self, quote); | ||
// Return zero price so the error can be gracefully handled by the caller | ||
if (result.price == 0 && result.conf == 0) { | ||
return PythStructs.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.
why not put this check in scaleToExponent
?
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.
I will try to update the check in scaleToExponent
@@ -30,4 +30,4 @@ contract PythStructs { | |||
// Latest available exponentially-weighted moving average price | |||
Price emaPrice; | |||
} | |||
} | |||
} |
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.
can you remove this change?
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.
sure
return (result.price, result.conf, result.expo, result.publishTime); | ||
} | ||
|
||
// Add more test functions as needed |
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.
maybe make a forge project and add test cases there? There should be a lot more tests before this can be merged
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.
forge allows for fuzz testing as well which can be useful so that you don't have to manually define all the cases
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.
I see. Can you provide me more details on forge? Do the forge tests also need to be part of this codebase?
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.
https://book.getfoundry.sh/reference/forge/forge
you should be able to include this library in a forge environment and add tests there. there should be tests somewhere before this is merged. you can add tests to the forge-test
folder, see the note here:
pyth-crosschain/target_chains/ethereum/contracts/foundry.toml
Lines 6 to 8 in 3cb45ec
# We put the tests into the forge-test directory (instead of test) so that | |
# truffle doesn't try to build them | |
test = 'forge-test' |
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.
Got it, thanks for the info
We have decided to move forward with #1746 |
Description
This is a solidity implementation of a Math library on Solana that can easily combine two Pyth Prices into one, for instance: (SOL:USD/ETH:USD=SOL/ETH)
result.price
is shown as$0.04069877
which is4069877 * 10^-8
, as you can see in the code implementation belowabis
folder from Remix IDEPublic methods implemented in Solidity from the Pyth Rust SDK
The methods like
get_price_in_quote
,div
,normalize
,scale_to_exponent
, andto_unsigned
, are public because they are part of theimpl Price
block in the Rust based Math Library implementation. Correspondingly inPriceCombine.sol
, functions likegetPriceInQuote
,div
,normalize
,scaleToExponent
,toUnsigned
have been written in the Solidity LibrarygetPriceInQuote
div
normalize
MIN_PD_V_I64
andMAX_PD_V_I64
constantsscaleToExponent
toUnsigned
This PR has been submitted as a solution to the [Pyth Superteam Bounty] on Superteam earn platform (https://earn.superteam.fun/listings/bounty/create-a-math-library-for-combining-pyth-prices-on-solidity/)