-
Notifications
You must be signed in to change notification settings - Fork 260
feat(target_chains/ethereum/sdk/solidity): add convertToUint method to the sdk #1390
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
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.
thank you for moving this.
Before I approve this, can you please add some tests for this logic ? If we are providing utilities for users to import, then we need to subject them to the same rigorous standards as other on-chain code we provide for them. You can make a new test file here https://github.com/pyth-network/pyth-crosschain/tree/main/target_chains/ethereum/contracts/forge-test
pragma solidity ^0.8.0; | ||
|
||
library PythUtils { | ||
/// @notice Converts a Pyth price to a uint256 with a target number of decimals |
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 give an example here to clarify what this means (use ethereum and 18 decimals)
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 also note that this function can lose precision and you need to be careful about that? For example, if the price is 0.003 and you set targetdecimals to 2, then the result will be 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.
Added the tests PythTestUtils.t.sol
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 please run the npm run generate-abi
to add the ABI files as well?
e310c50
to
f52c14c
Compare
f52c14c
to
1b41cde
Compare
target_chains/ethereum/contracts/forge-test/utils/PythTestUtils.t.sol
Outdated
Show resolved
Hide resolved
fab9ada
to
72f3f56
Compare
…o the sdk (#1390) * Moving convertToUint to utils * pre-commit fix * reversing OracleSwap example * pre-commit] * added test * abi-gen * Added solc to sdk * resolved comments
No description provided.