-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: redstone oracle adapter test #13
Conversation
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
release-v4.5
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
redstone/getRedstonePayload.js
Outdated
return Number(bigNumberPrice); | ||
}; | ||
|
||
const pickMedian = (arr) => { |
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.
We need to get the median here as we are pushing 3 prices from different signers that can slightly defer. Redstone contract does the same (https://github.com/UMAprotocol/oval-contracts/blob/pablo/redstone-adapter/lib/RedStoneBaseContracts/@redstone-finance/evm-connector/contracts/core/RedstoneDefaultsLib.sol#L37)
test/unit/RedStoneOracle.sol
Outdated
} | ||
|
||
function testPushPrice() public { | ||
vm.warp(1234); |
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.
The timestamp associated to the price pushed is the block.timestamp
when the price is pushed.
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.
yup, same as with chainlink.
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 is awesome. it shows we can use Redstone with zero changes, treating it directly as chainlink. only downside with this PR right now is that it has a lot of copy paste files to get the deployments of tests working. I'll try create a follow on commit that moves this into more minimal imports.
test/unit/RedStoneOracle.sol
Outdated
function setUp() public { | ||
redstoneOracle = new RedstonePriceFeedWithRounds(bytes32("BTC")); | ||
sourceAdapter = new TestedSourceAdapter( | ||
IAggregatorV3Source(address(redstoneOracle)) |
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 is great! we can show that we can import redstone sources, treat them exactly as chainlink, and everything continues to work as we'd expect.
test/unit/RedStoneOracle.sol
Outdated
} | ||
|
||
function testPushPrice() public { | ||
vm.warp(1234); |
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.
yup, same as with chainlink.
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.
nice, and agree with @chrismaree to clean dependencies in follow-up
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.
LGTM!
Signed-off-by: chrismaree <christopher.maree@gmail.com>
@@ -45,26 +45,26 @@ contract ChainlinkSourceAdapterTest is CommonTest { | |||
uint256 targetTime = block.timestamp - 1 hours; | |||
(uint80 latestRound,,,,) = chainlink.latestRoundData(); | |||
(int256 lookBackPrice, uint256 lookBackTimestamp) = sourceAdapter.tryLatestDataAt(targetTime, 10); | |||
(, int256 answer, uint256 startedAt,,) = chainlink.getRoundData(latestRound - 1); | |||
assertTrue(startedAt <= targetTime); // The time from the chainlink source is at least 1 hours old. | |||
(, int256 answer,, uint256 updatedAt,) = chainlink.getRoundData(latestRound - 1); |
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.
after writing the other tests (for redstone) I realized this is inconsistent with what the adapter actually does, so updated it.
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 updated this PR somewhat (mainnet fork tests, moved scripts. toethers, moved scripts to typescript, cleaned up. someof the imports and pathing). but I'm still not totally done. I don't like that we have toe lib
as a directory of files, it should be a git submodule. This is currently broken on the redstone imports. andneeds a bit of effort to fix. we should address this before merging this PR.
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
scripts/getRedstonePayload.log.txt
Outdated
@@ -0,0 +1,2 @@ | |||
You have to provide a data FeedAn error occurred: Request failed {"reqParams":{"dataServiceId":"redstone-primary-prod","uniqueSignersCount":3,"dataFeeds":["--no-warnings"],"urls":["https://oracle-gateway-1.a.redstone.finance"]}}, Original error: AggregateError: , errors: Error: Requested data feed id is not included in response: --no-warnings |
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 not be committed and given its in the .gitignore it wont be in the future.
remappings.txt
Outdated
@@ -1,3 +1,3 @@ | |||
ds-test/=lib/forge-std/lib/ds-test/src/ | |||
forge-std/=lib/forge-std/src/ | |||
openzeppelin-contracts/=lib/openzeppelin-contracts/ | |||
redstone-oracles-monorepo/=lib/redstone-contracts/src/RedStoneBaseContracts/redstone-oracles-monorepo |
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.
do you think naming it like redstone-oracles-monorepo
makes sense? prehaps redstone-oracle
?
@@ -1,3 +1,3 @@ | |||
ds-test/=lib/forge-std/lib/ds-test/src/ | |||
forge-std/=lib/forge-std/src/ | |||
openzeppelin-contracts/=lib/openzeppelin-contracts/ |
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 what happened 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.
I realized it wasn't needed when I was cleaning up the remappings.
Everything is imported with "openzeppelin-contracts/..."
and Foundry automatically reads from the lib folder.
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 think this is great. the only thing that once could argue we don't want is the file src/oracles/RedstonePriceFeedWithRounds.sol
. this could be pulled into the test where its used? otherwise this file has no other direct usage and is a bit different to the rest of the repo?
@@ -6,6 +6,7 @@ import {SafeCast} from "openzeppelin-contracts/contracts/utils/math/SafeCast.sol | |||
import {DecimalLib} from "../lib/DecimalLib.sol"; | |||
import {IAggregatorV3Source} from "../../interfaces/chainlink/IAggregatorV3Source.sol"; | |||
import {DiamondRootOval} from "../../DiamondRootOval.sol"; | |||
import "forge-std/console.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.
import "forge-std/console.sol"; |
@@ -0,0 +1,41 @@ | |||
// SPDX-License-Identifier: BUSL-1.1 | |||
pragma solidity 0.8.17; | |||
|
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.
if this is going to live in the repo here it needs comments & natspec
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.
@chrismaree I pulled this out to https://github.com/UMAprotocol/redstone-contracts/tree/master/src/examples
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
@@ -33,10 +33,10 @@ contract CoinbaseSourceAdapterTest is CommonTest { | |||
string public ticker = "ETH"; | |||
uint256 public price = 3000e6; | |||
|
|||
function pushPrice(string memory ticker, uint256 price, uint256 timestamp) public { | |||
function pushPrice(string memory ticker, uint256 priceToPush, uint256 timestamp) public { |
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 is to remove an unrelated warning of variable shadowing
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
Signed-off-by: Pablo Maldonado <pablo@umaproject.org>
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 is great! love showing support for something by simple adding some tests :)
Proposed Changes in this PR: