Skip to content

Chore(evm) add combine pricefeed #2665

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

aditya520
Copy link
Member

@aditya520 aditya520 commented May 8, 2025

Summary

Rationale

How has this been tested?

  • Current tests cover my changes
  • Added new tests
  • Manually tested the code

Copy link

vercel bot commented May 8, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
api-reference ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 13, 2025 0:37am
7 Skipped Deployments
Name Status Preview Comments Updated (UTC)
component-library ⬜️ Skipped (Inspect) Jun 13, 2025 0:37am
developer-hub ⬜️ Skipped (Inspect) Jun 13, 2025 0:37am
entropy-debugger ⬜️ Skipped (Inspect) Jun 13, 2025 0:37am
entropy-explorer ⬜️ Skipped (Inspect) Jun 13, 2025 0:37am
insights ⬜️ Skipped (Inspect) Jun 13, 2025 0:37am
proposals ⬜️ Skipped (Inspect) Jun 13, 2025 0:37am
staking ⬜️ Skipped (Inspect) Jun 13, 2025 0:37am

@aditya520 aditya520 marked this pull request as ready for review May 9, 2025 18:40
@vercel vercel bot temporarily deployed to Preview – staking June 11, 2025 14:23 Inactive
@vercel vercel bot temporarily deployed to Preview – entropy-explorer June 11, 2025 14:23 Inactive
@vercel vercel bot temporarily deployed to Preview – developer-hub June 11, 2025 14:23 Inactive
@vercel vercel bot temporarily deployed to Preview – proposals June 11, 2025 14:23 Inactive
@vercel vercel bot temporarily deployed to Preview – entropy-debugger June 11, 2025 14:23 Inactive
@vercel vercel bot temporarily deployed to Preview – component-library June 11, 2025 14:23 Inactive
@vercel vercel bot temporarily deployed to Preview – entropy-explorer June 13, 2025 09:28 Inactive
@vercel vercel bot temporarily deployed to Preview – developer-hub June 13, 2025 09:28 Inactive
@vercel vercel bot temporarily deployed to Preview – proposals June 13, 2025 09:28 Inactive
@vercel vercel bot temporarily deployed to Preview – insights June 13, 2025 09:28 Inactive
@vercel vercel bot temporarily deployed to Preview – component-library June 13, 2025 09:28 Inactive
@vercel vercel bot temporarily deployed to Preview – staking June 13, 2025 09:28 Inactive
@vercel vercel bot temporarily deployed to Preview – entropy-debugger June 13, 2025 09:28 Inactive
@vercel vercel bot temporarily deployed to Preview – component-library June 13, 2025 12:34 Inactive
@vercel vercel bot temporarily deployed to Preview – developer-hub June 13, 2025 12:34 Inactive
@vercel vercel bot temporarily deployed to Preview – insights June 13, 2025 12:34 Inactive
@vercel vercel bot temporarily deployed to Preview – proposals June 13, 2025 12:34 Inactive
@vercel vercel bot temporarily deployed to Preview – staking June 13, 2025 12:34 Inactive
@vercel vercel bot temporarily deployed to Preview – entropy-explorer June 13, 2025 12:34 Inactive
@vercel vercel bot temporarily deployed to Preview – entropy-debugger June 13, 2025 12:34 Inactive
@aditya520 aditya520 changed the title WIP: Chore(evm) add combine pricefeed Chore(evm) add combine pricefeed Jun 13, 2025
@aditya520 aditya520 requested review from ali-behjati and jayantk June 13, 2025 12:41
Copy link
Collaborator

Choose a reason for hiding this comment

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

why didn't you import it?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah pls import as a dependency

Copy link
Member Author

@aditya520 aditya520 Jun 13, 2025

Choose a reason for hiding this comment

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

It’s better to copy specific methods from another library when writing your own, especially if you’re only using a limited subset.
This helps reduce contract code size and lowers deployment costs.
In my opinion, focused Solidity libraries are always the better choice.

Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

this is much better than last time. couple comments here. I suggest we sit down in person and work through them -- should be pretty quick


uint8 priceDecimals = uint8(uint32(-1 * expo));
// price is int64, always >= 0 here
uint256 unsignedPrice = uint256(uint64(price));
Copy link
Contributor

Choose a reason for hiding this comment

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

the comment here is a bit confusing -- this is safe because you have a revert ^. I would say that explicitly

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah pls import as a dependency


int32 factoredExpo = combinedExpo - targetExpo;

if (factoredExpo > 77 || factoredExpo < -77) revert PythErrors.ExponentOverflow();
Copy link
Contributor

Choose a reason for hiding this comment

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

can't you put all of this exponent stuff into the Math.mulDiv call ^ and be done?

like you're trying to compute z in:

z * 10^c = (x * 10^a) / (y * 10^b)

which you can rewrite to:

z = (x * 10^(a - (b + c))) / y

so can't you do like:

uint256 result = Math.mulDiv(uint64(price1), 10 ** (expo1 - (expo2 + targetExpo)), uint64(price2));

and then you're done?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Math.mulDiv is a good find -- that's a very useful primitive for this kind of arithmetic)

@@ -33,4 +33,38 @@ library PythUtils {
10 ** uint32(priceDecimals - targetDecimals);
}
}

/// @notice Combines two prices to get a cross-rate
Copy link
Contributor

Choose a reason for hiding this comment

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

The thing i would document in particular is: under what range of exponents is this function guaranteed not to overflow for any set of prices. That's the safe range to use this function in.

@@ -33,4 +33,38 @@ library PythUtils {
10 ** uint32(priceDecimals - targetDecimals);
}
}

/// @notice Combines two prices to get a cross-rate
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 also document that you get the floor of the result in the target exponent, so small numbers can round to 0.

if (expo < -255) {
revert PythErrors.InvalidInputExpo();
}
int32 combinedExpo = int32(uint32(targetDecimals)) + expo;
Copy link
Contributor

Choose a reason for hiding this comment

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

the name of this variable is odd -- this is an offset or multiplier for the result of some sort right?

I would be careful with names like "expo" and "decimal" because one is the negative of the other. When I see this line, I go "wait you can't add a decimal and an expo and get an expo".

// Bounds check: prevent overflow/underflow with base 10 exponentiation
// Calculation: 10 ** n <= (2 ** 256) - 1
// n <= log10((2 ** 256) - 1)
// n <= 77.2
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't actually the bound you want. You want to use 2**(256 - 64) to guarantee that there's enough space for the full int64 price to fit in the result

Copy link
Contributor

Choose a reason for hiding this comment

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

(the case that you want to worry about is when there is a price-dependent overflow. The exponent and decimal will be set once at contract deployment time, but the price will change over time.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants