Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Allow replacing old value in MarketOracle even if last push was less than reportDelaySec ago #72

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow replacing old value in MarketOracle even if last push was less than reportDelaySec ago #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df5d63d
Allow for replacing old values
ahnaguib Jul 16, 2020
f61b12c
Allow for replacing old values
ahnaguib Sep 14, 2020
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
8 changes: 7 additions & 1 deletion contracts/MedianOracle.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,13 +120,18 @@ contract MedianOracle is Ownable, IOracle {
Report[2] storage reports = providerReports[providerAddress];
uint256[2] memory timestamps = [reports[0].timestamp, reports[1].timestamp];

// Checks that this providerAddress is already whitelisted
require(timestamps[0] > 0);

uint8 index_recent = timestamps[0] >= timestamps[1] ? 0 : 1;
uint8 index_past = 1 - index_recent;

uint256 minValidTimestamp = now.sub(reportExpirationTimeSec);
uint256 maxValidTimestamp = now.sub(reportDelaySec);

// Check that the push is not too soon after the last one.
require(timestamps[index_recent].add(reportDelaySec) <= now);
require(timestamps[index_past] < minValidTimestamp ||
timestamps[index_recent] <= maxValidTimestamp);

reports[index_past].timestamp = now;
reports[index_past].payload = payload;
Expand All @@ -140,6 +145,7 @@ contract MedianOracle is Ownable, IOracle {
function purgeReports() external
{
address providerAddress = msg.sender;
// Check that this providerAddress is already whitelisted
require (providerReports[providerAddress][0].timestamp > 0);
providerReports[providerAddress][0].timestamp=1;
providerReports[providerAddress][1].timestamp=1;
Expand Down
12 changes: 11 additions & 1 deletion test/unit/median_oracle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,15 @@ contract('MedianOracle:addProvider', async function (accounts) {
});
});

// TODO(naguib): Add test scenarios for:
// Reported rates (ordered by time) are r1, r2. The new rate is r.
// r2 < minValidTimestamp
// r1 < minValidTimestamp < r2 < maxValidTimestamp
// minValidTimestamp < r1 < r2 < maxValidTimestamp
// r1 < minValidTimestamp < maxValidTimestamp < r2
// minValidTimestamp < r1 < maxValidTimestamp < r2
// maxValidTimestamp < r1

contract('MedianOracle:pushReport', async function (accounts) {
before(async function () {
await setupContractsAndAccounts(accounts);
Expand All @@ -77,7 +86,8 @@ contract('MedianOracle:pushReport', async function (accounts) {
expect(await chain.isEthException(oracle.pushReport(1000000000000000000, { from: A }))).to.be.true;
oracle.addProvider(A, { from: deployer });
r = await oracle.pushReport(1000000000000000000, { from: A });
// should fail if reportDelaySec did not pass since the previous push
r = await oracle.pushReport(1000000000000000000, { from: A });
Copy link
Member

Choose a reason for hiding this comment

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

can we create a separate test for this case?

I think the original test is should only push from authorized source

Copy link
Member

@aalavandhan aalavandhan Sep 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Will be great if we can test all the cases:

Reported rates (ordered by time) are r1, r2. The new rate is r.

  1. r2 < minValidTimestamp
  2. r1 < minValidTimestamp < r2 < maxValidTimestamp
  3. minValidTimestamp < r1 < r2 < maxValidTimestamp
  4. r1 < minValidTimestamp < maxValidTimestamp < r2
  5. minValidTimestamp < r1 < maxValidTimestamp < r2
  6. maxValidTimestamp < r1

Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will make sure at least coverage is back to 100%

// should fail if push is too early
expect(await chain.isEthException(oracle.pushReport(1000000000000000000, { from: A }))).to.be.true;
});
it('should emit ProviderReportPushed message', async function () {
Expand Down