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

Price feed governable addition #545

Merged
merged 23 commits into from
Apr 16, 2020
Merged

Price feed governable addition #545

merged 23 commits into from
Apr 16, 2020

Conversation

NicholasDotSol
Copy link
Contributor

@NicholasDotSol NicholasDotSol commented Apr 1, 2020

Instead of holding two feeds, ETH/USD and BTC/USD, hold an array
of direct ETH/BTC feeds.

When calculating price, look for the first active price feed from the
ETH/BTC array and use that one to calculate the wei/sat price. Then
use this to calculate lot size in wei where needed.

Feeds can only be added to this array by TBTCSystem; TBTCSystem
has a 90-day timer before a price feed addition can be finalized.

Also includes #576 .

NicholasDotSol added 7 commits March 31, 2020 22:09
Peek allows us to query the Medianizer wihtout reverting.
This will be useful if
we just want to check if the price feed is live
BTCETHPriceFeed Should implement the functions needed to
add price feeds in case one breaks.
Instead of holding one pair of feeds, hold an array of feeds for each (btcUsdFeeds and ethUsdFeeds)

When calculating price, look for the first active price feed
from each array and use that one for to get
the corresponding asset price.

Only `TBTCSystem` is allowed to add a new Medianizer feed
peek does not revert on zero-value feed querry, therefore it is
useful for checking Medianizer status without reprocusions.
the addition is two-step just like all the other governable values.

eg: initializeAddBtcUsdFeed is called, and after
 `priceFeedGovernanceTimeDelay` has passed
finalizeAddBtcUsdFeed can be called

Events are emitted during every step of the process.
btcEthPriceFeed.initialize() now
takes TBTCSystem address for ACL
IMedianizer btcPriceFeed;
IMedianizer ethPriceFeed;
IMedianizer[] private btcUsdFeeds;
IMedianizer[] private ethUsdFeeds;
Copy link
Contributor

Choose a reason for hiding this comment

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

Really it's two changes here… We'll have a single direct BTCETH price feed, in addition to this way of dealing with failures.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, we just need one array

Copy link
Member

@mhluongo mhluongo left a comment

Choose a reason for hiding this comment

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

Piling on 😆

IMedianizer btcPriceFeed;
IMedianizer ethPriceFeed;
IMedianizer[] private btcUsdFeeds;
IMedianizer[] private ethUsdFeeds;
Copy link
Member

Choose a reason for hiding this comment

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

Yep, we just need one array

@Shadowfiend
Copy link
Contributor

Shadowfiend commented Apr 5, 2020

Piling on 😆

but why?

NicholasDotSol added 3 commits April 7, 2020 06:02
Instead pf using BTCUSD and ETHUSD medianizers,
use a direct BTCETH medianizer
NicholasDotSol and others added 6 commits April 14, 2020 15:22
Use the finaince definition pair, where ETHBTC represents
the number of BTC in an ETH. This is what the input is.
As a price feed output, we use saewei, meaning the number of wei in a satoshi.
We will use satwei to clculate lot size value
In preparation for an ETHBTC feed.
So that new system deployments don't require new contract authorizations
on the Maker side, an intermediate medianizer has already been deployed
to Ropsten for Maker to authorize. It is designed to have an updatable
reference to the Maker medianizer, and pass through reads to that
instance.

The already-deployed address is added to the externals file, while the
code is checked in to the price-feed directory, but not migrated as part
of the system migrations.
Inter-medi[anizer]-ary: Add intermediary authorized medianizer access contract for Ropsten
Something got mangled somewhere along the way...
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Left a few notes, still need to go through the tests in more detail.

solidity/contracts/price-feed/SatWeiPriceFeed.sol Outdated Show resolved Hide resolved
solidity/contracts/price-feed/SatWeiPriceFeed.sol Outdated Show resolved Hide resolved
solidity/contracts/price-feed/SatWeiPriceFeed.sol Outdated Show resolved Hide resolved
solidity/contracts/price-feed/SatWeiPriceFeed.sol Outdated Show resolved Hide resolved
solidity/contracts/interfaces/ISatWeiPriceFeed.sol Outdated Show resolved Hide resolved
Shadowfiend and others added 2 commits April 15, 2020 20:14
The new intermediary medianizer incorporates the peek() method.
Ensure the correct unsits are used and add missing docs
@mhluongo
Copy link
Member

Can we get an updated / accurate PR description before merge? Dismissing my review for now, happy to take a second look when ready

@Shadowfiend
Copy link
Contributor

Description is accurate already as far as I can tell.

NicholasDotSol added 3 commits April 16, 2020 10:36
Clarify that `SatWeiPriceFeed` utilizes a price feed
operated by MakerDao, not that it is operated by MkerDao itself.
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Okay, let's go.

@Shadowfiend Shadowfiend merged commit 1750314 into master Apr 16, 2020
@Shadowfiend Shadowfiend deleted the price-feed-gov branch April 16, 2020 18:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants