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

Implemented a Math Library to combine Pyth prices in Solidity - tested and working on Remix IDE #1732

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 167 additions & 0 deletions target_chains/ethereum/sdk/solidity/PriceCombine.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
// PriceCombine.sol
// SPDX-License-Identifier: Apache 2
pragma solidity ^0.8.0;

import "./PythStructs.sol";

// Constants for working with Pyth's number representation
int32 constant PD_EXPO = -9;
uint64 constant PD_SCALE = 1_000_000_000;
uint64 constant MAX_PD_V_U64 = (1 << 28) - 1;

// Solidity Library to easily combine 2 Pyth Prices into 1 ( for example SOL:USD/ETH:USD=SOL/ETH). For Rust implementation, refer Pyth Rust SDK https://github.com/pyth-network/pyth-sdk-rs/blob/main/pyth-sdk/src/price.rs
// Based on the Rust implementation, the functions are defined within the `impl Price` block, and they are public methods of the `Price` struct and are as follows `get_price_in_quote`, `div`, `normalize`, `scale_to_exponent`
// Similarly the functions implemented in solidity are `public` and `pure` types.

library PriceCombine {
/**
* @notice Computes the price in quote currency.
* @param self The price of the base currency.
* @param quote The price of the quote currency.
* @param resultExpo The exponent for the result.
* @return The combined price in the quote currency.
*/
function getPriceInQuote(PythStructs.Price memory self, PythStructs.Price memory quote, int32 resultExpo) public pure returns (PythStructs.Price memory) {
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({
Copy link
Contributor

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?

Copy link
Author

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

price: 0,
conf: 0,
expo: 0,
publishTime: 0
});
}
return scaleToExponent(result, resultExpo);
}

/**
* @notice Divides the price of the base currency by the quote currency price.
* @param self The price of the base currency.
* @param other The price of the quote currency.
* @return The combined price after division.
*/
function div(PythStructs.Price memory self, PythStructs.Price memory other) public pure returns (PythStructs.Price memory) {
PythStructs.Price memory base = normalize(self);
other = normalize(other);

// If the price of the quote currency is zero, return zero
if (other.price == 0) {
Copy link
Contributor

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

return PythStructs.Price({
price: 0,
conf: 0,
expo: 0,
publishTime: 0
});
}

// Convert prices to unsigned integers and get their signs
(uint64 basePrice, int64 baseSign) = toUnsigned(base.price);
(uint64 otherPrice, int64 otherSign) = toUnsigned(other.price);

// Compute the midprice
uint64 midprice = basePrice * PD_SCALE / otherPrice;
int32 midpriceExpo = base.expo - other.expo + PD_EXPO;

// Compute the confidence interval
uint64 otherConfidencePct = other.conf * PD_SCALE / otherPrice;
uint128 conf = (base.conf * PD_SCALE / otherPrice) + (otherConfidencePct * midprice) / PD_SCALE;
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Author

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


// Check for overflow and return the result
if (conf < type(uint64).max) {
return PythStructs.Price(
int64(int64(midprice) * baseSign * otherSign),
uint64(conf),
midpriceExpo,
self.publishTime < other.publishTime ? self.publishTime : other.publishTime
Copy link
Contributor

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-

Copy link
Author

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

);
} else {
// Return zero price if there's an overflow
return PythStructs.Price({
price: 0,
conf: 0,
expo: 0,
publishTime: 0
});
}
}

/**
* @notice Normalizes the price and confidence to be within acceptable range.
* @param self The price structure to normalize.
* @return The normalized price structure.
*/
function normalize(PythStructs.Price memory self) public pure returns (PythStructs.Price memory) {
(uint64 price, int64 sign) = toUnsigned(self.price);
uint64 conf = self.conf;
int32 expo = self.expo;

// Adjust the price and confidence if they are too large
while (price > MAX_PD_V_U64 || conf > MAX_PD_V_U64) {
price = price / 10;
conf = conf / 10;
expo = expo + 1;
}

return PythStructs.Price({
price: int64(price) * sign,
conf: conf,
expo: expo,
publishTime: self.publishTime
});
}

/**
* @notice Scales the price to the target exponent.
* @param self The price structure to scale.
* @param targetExpo The target exponent.
* @return The scaled price structure.
*/
function scaleToExponent(PythStructs.Price memory self, int32 targetExpo) public pure returns (PythStructs.Price memory) {
int32 delta = targetExpo - self.expo;
int64 price = self.price;
uint64 conf = self.conf;

// Adjust the price and confidence based on the exponent difference
if (delta >= 0) {
while (delta > 0 && (price != 0 || conf != 0)) {
price = price / 10;
conf = conf / 10;
delta = delta - 1;
}
return PythStructs.Price({
price: price,
conf: conf,
expo: targetExpo,
publishTime: self.publishTime
});
} else {
while (delta < 0) {
price = price * 10;
conf = conf * 10;
delta = delta + 1;
}
return PythStructs.Price({
price: price,
conf: conf,
expo: targetExpo,
publishTime: self.publishTime
});
}
}

/**
* @notice Converts a signed integer to an unsigned integer and a sign bit.
* @param x here is the signed integer.
* @return The unsigned integer and sign bit.
*/
function toUnsigned(int64 x) public pure returns (uint64, int64) {
Copy link
Contributor

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?

Copy link
Author

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

if (x == type(int64).min) {
return (uint64(type(int64).max) + 1, -1);
} else if (x < 0) {
return (uint64(-x), -1);
} else {
return (uint64(x), 1);
}
}
}
34 changes: 34 additions & 0 deletions target_chains/ethereum/sdk/solidity/PriceCombineTest.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// SPDX-License-Identifier: Apache 2
pragma solidity ^0.8.0;

import "./PythStructs.sol";
import "./PriceCombine.sol";

// This contract is used to test the library functions and tested in Remix IDE. All ABIs are present in the /abis folder
contract PriceCombineTest {
using PriceCombine for PythStructs.Price;

/**
* @notice Test the getPriceInQuote function
* @param basePrice The price of the base currency (example: SOL/USD) 13913000000 (represents the current SOL/USD $139.13 with 8 decimal places)
* @param baseConf The confidence interval of the base currency
* @param baseExpo The exponent of the base currency (here: -8 -> 8 decimal units) indicates price is scaled by 10^-8
* @param basePublishTime The publish time of the base currency (UNIX timestamp)
* @param quotePrice The price of the quote currency (example: ETH/USD) 341853000000 (represents the current ETH/USD $3418.53 with 8 decimal places)
* @param quoteConf The confidence interval of the quote currency
* @param quoteExpo The exponent of the quote currency (here: -8 -> 8 decimal units) indicates price is scaled by 10^-8
* @param quotePublishTime The publish time of the quote currency (UNIX timestamp)
* @param resultExpo The desired exponent for the result (here: -8)
* @return The price, confidence interval, exponent, and publish time of the resulting price
*/
function testGetPriceInQuote(int64 basePrice, uint64 baseConf, int32 baseExpo, uint basePublishTime,
int64 quotePrice, uint64 quoteConf, int32 quoteExpo, uint quotePublishTime,
int32 resultExpo) public pure returns (int64, uint64, int32, uint) {
PythStructs.Price memory base = PythStructs.Price(basePrice, baseConf, baseExpo, basePublishTime);
PythStructs.Price memory quote = PythStructs.Price(quotePrice, quoteConf, quoteExpo, quotePublishTime);
PythStructs.Price memory result = base.getPriceInQuote(quote, resultExpo);
return (result.price, result.conf, result.expo, result.publishTime);
}

// Add more test functions as needed
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Author

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?

Copy link
Contributor

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:

# We put the tests into the forge-test directory (instead of test) so that
# truffle doesn't try to build them
test = 'forge-test'

Copy link
Author

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

}
2 changes: 1 addition & 1 deletion target_chains/ethereum/sdk/solidity/PythStructs.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,4 @@ contract PythStructs {
// Latest available exponentially-weighted moving average price
Price emaPrice;
}
}
}
Copy link
Contributor

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?

Copy link
Author

Choose a reason for hiding this comment

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

sure

Loading