Skip to content
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.

Sign up for GitHub

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

fix(funding-rate-proposer): Use historical-price to propose funding rates #2881

Merged
merged 4 commits into from
Apr 20, 2021
Merged

fix(funding-rate-proposer): Use historical-price to propose funding rates #2881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
20a532a
fix(funding-rate-proposer): Use historical-price to propose funding r…
nicholaspai Apr 19, 2021
0854feb
Fix tests
nicholaspai Apr 19, 2021
f1d31e7
Update proposer.js
nicholaspai Apr 19, 2021
04a031c
Update pricefeed before fetching historical price
nicholaspai Apr 19, 2021
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
76 changes: 37 additions & 39 deletions packages/funding-rate-proposer/src/proposer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ class FundingRateProposer {
await Promise.all([
// Increase allowances for all contracts to spend the bot owner's respective collateral currency.
this._setAllowances(),
// For each contract, create, save, and update a pricefeed instance for its identifier,
// or just update the pricefeed if the identifier already has cached an instance.
this._cacheAndUpdatePriceFeeds()
// For each contract, create and save a pricefeed instance for its identifier,
// or just fetch the pricefeed if the identifier already has cached an instance.
this._createOrGetCachedPriceFeed()
]);
}

Expand Down Expand Up @@ -163,8 +163,36 @@ class FundingRateProposer {

// Assume pricefeed has been cached and updated prior to this function via the `update()` call.
const priceFeed = this.priceFeedCache[fundingRateIdentifier];
const pricefeedPrice = await this._getPriceFeedAndPrice(fundingRateIdentifier);
if (!pricefeedPrice) return;
if (!priceFeed) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This checking for !priceFeed is copied from below, line 397ish

this.logger.error({
at: "PerpetualProposer",
message: "Failed to create pricefeed for funding rate identifier",
fundingRateIdentifier
});
return null;
}
// Get hypothetical timestamp to use for a new price proposal:
// Unless `usePriceFeedTime=true`, use the latest block's timestamp as the request
nicholaspai marked this conversation as resolved.
Show resolved Hide resolved
// timestamp so that the contract does not interpret `requestTimestamp` as being in the future.
// Note: Update pricefeed so that its latest update time < latest web3 block:
await priceFeed.update();
const requestTimestamp = usePriceFeedTime
Copy link
Member Author

Choose a reason for hiding this comment

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

This requestTimestamp logic is copied from below, line 198ish

? priceFeed.getLastUpdateTime()
: (await this.web3.eth.getBlock("latest")).timestamp;
Copy link
Member

Choose a reason for hiding this comment

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

Is it a problem if the latest block time is > the price feed's latest update time?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes theoretically this could cause divergence in historical price fetched for this request time by a disputer. We could conservatively add an assert(await this.web3.eth.getBlock("latest")).timestamp <= priceFeed.getLastUpdateTime()) here and just update the pricefeed again and check the new time. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or we can do like the optimistic-oracle-proposer and just conservatively update() the priceFeed before fetching historical price. reference

Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented the above ^

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good! This isn't guaranteed to have an update time > block time, but I think this is probably as close as we can reasonably get:

  1. Price feeds usually throttle to ensure they don't update more than once per minute by default.
  2. Block times can (but usually don't) be ahead of wall-clock time (they have a 90 second or so tolerance if I remember correctly).

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah the other solution which I think is a bit over engineered for now is something like

let requestTimestamp;

// Update pricefeed until its last update time is caught up with the latest network block, or
// break early after 1 minute and assume that's good enough (or throw).
setTimeout(function() {
    while(true) {
        await priceFeed.update();
        requestTimestamp = (await web3.eth.getBlock()).timestamp
        if (priceFeed.lastUpdateTime() >= requestTimestamp) break;
    }
},
60000

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, agree with your decision to do it the way you have it in the PR. LGTM!

Copy link
Member Author

Choose a reason for hiding this comment

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

But this wouldn't be useful unless the minTimeBetweenUpdates was respected, and given the expected iteration cadence of 5 minutes we don't have much timeout period to play with. This sort of method would make sense in a looping infrastructure

let _pricefeedPrice;
try {
Copy link
Member Author

Choose a reason for hiding this comment

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

This try-catch is the new logic in that it computes a historical (funding rate) price instead of the current price.

_pricefeedPrice = (await priceFeed.getHistoricalPrice(Number(requestTimestamp))).toString();
} catch (error) {
this.logger.error({
at: "PerpetualProposer",
nicholaspai marked this conversation as resolved.
Show resolved Hide resolved
message: "Failed to query current price for funding rate identifier",
fundingRateIdentifier,
requestTimestamp,
error
});
return;
}
const pricefeedPrice = this._formatPriceToPricefeedPrecision(_pricefeedPrice, priceFeed);
let onchainFundingRate = currentFundingRateData.rate.toString();

// Check that pricefeedPrice is within [configStore.minFundingRate, configStore.maxFundingRate]
Expand Down Expand Up @@ -195,11 +223,6 @@ class FundingRateProposer {
this.toBN(this.toWei(this.fundingRateErrorPercent.toString()))
);
if (shouldUpdateFundingRate) {
// Unless `usePriceFeedTime=true`, use the latest block's timestamp as the request
// timestamp so that the contract does not interpret `requestTimestamp` as being in the future.
const requestTimestamp = usePriceFeedTime
? priceFeed.getLastUpdateTime()
: (await this.web3.eth.getBlock("latest")).timestamp;
const proposal = cachedContract.contract.methods.proposeFundingRate(
{ rawValue: pricefeedPrice },
requestTimestamp
Expand Down Expand Up @@ -294,7 +317,7 @@ class FundingRateProposer {
});
}

async _cacheAndUpdatePriceFeeds() {
async _createOrGetCachedPriceFeed() {
await Promise.map(Object.keys(this.contractCache), async contractAddress => {
const fundingRateIdentifier = this.hexToUtf8(
this.contractCache[contractAddress].state.currentFundingRateData.identifier
Expand All @@ -321,11 +344,6 @@ class FundingRateProposer {
);
this.priceFeedCache[fundingRateIdentifier] = priceFeed;
}

// If pricefeed was created or fetched from cache, update it
if (priceFeed) {
await priceFeed.update();
}
});
}

Expand Down Expand Up @@ -391,34 +409,14 @@ class FundingRateProposer {
};
}

// Load cached pricefeed and fetch current price. Returns null if failure to get either pricefeed or current price.
async _getPriceFeedAndPrice(fundingRateIdentifier) {
const priceFeed = this.priceFeedCache[fundingRateIdentifier];
if (!priceFeed) {
this.logger.error({
at: "PerpetualProposer",
message: "Failed to create pricefeed for funding rate identifier",
fundingRateIdentifier
});
return null;
}
let currentPrice = priceFeed.getCurrentPrice();
if (!currentPrice) {
this.logger.error({
at: "PerpetualProposer",
message: "Failed to query current price for funding rate identifier",
fundingRateIdentifier
});
return null;
}

// Round `offchainFundingRate` to desired number of decimals by converting back and forth between the pricefeed's
_formatPriceToPricefeedPrecision(price, priceFeed) {
// Round `price` to desired number of decimals by converting back and forth between the pricefeed's
// configured precision:
return parseFixed(
// 1) `formatFixed` converts the price in wei to a floating point.
// 2) `toFixed` removes decimals beyond `this.precision` in the floating point.
// 3) `parseFixed` converts the floating point back into wei.
Number(formatFixed(currentPrice.toString(), priceFeed.getPriceFeedDecimals()))
Number(formatFixed(price.toString(), priceFeed.getPriceFeedDecimals()))
.toFixed(this.precision)
.toString(),
priceFeed.getPriceFeedDecimals()
Expand Down
28 changes: 14 additions & 14 deletions packages/funding-rate-proposer/test/proposer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,10 @@ contract("Perpetual: proposer.js", function(accounts) {
// not set in DefaultPriceFeedConfig
latestProposalTime = startTime.toNumber() + 1;
commonPriceFeedConfig = {
currentPrice: "0.000005",
// Mocked current price. This will be scaled to the identifier's precision. 1/2 of max funding rate
historicalPrice: "0.000001",
// Mocked historical price. This should be irrelevant for these tests.
historicalPrice: "0.000005",
// Mocked historical price. This will be scaled to the identifier's precision. 1/2 of max funding rate
currentPrice: "0.000001",
// Mocked current price. This should be irrelevant for these tests.
lastUpdateTime: latestProposalTime
// On funding rate updates, proposer requests a price for this time. This must be > than the Perpetual
// contract's last update time, which is the `startTime` when it was deployed. So we start
Expand Down Expand Up @@ -282,19 +282,19 @@ contract("Perpetual: proposer.js", function(accounts) {
await proposer._setAllowances();
assert.equal(spyCount, spy.callCount);
});
it("(_cacheAndUpdatePriceFeeds)", async function() {
// `update` should create a new pricefeed for each funding rate identifier.
it("(_createOrGetCachedPriceFeed)", async function() {
// should create a new pricefeed for each funding rate identifier.
assert.equal(Object.keys(proposer.priceFeedCache).length, fundingRateIdentifiersToTest.length);
for (let i = 0; i < fundingRateIdentifiersToTest.length; i++) {
assert.isTrue(
proposer.priceFeedCache[hexToUtf8(fundingRateIdentifiersToTest[i])] instanceof PriceFeedMockScaled
);
}
// Calling `_cacheAndUpdatePriceFeeds` usually emits DEBUG logs for
// Calling `_createOrGetCachedPriceFeed` usually emits DEBUG logs for
// each newly cached object. Calling it again should do nothing since we've already
// cached the price feeds.
const spyCount = spy.callCount;
await proposer._cacheAndUpdatePriceFeeds();
await proposer._createOrGetCachedPriceFeed();
assert.equal(spyCount, spy.callCount);
});
describe("(updateFundingRate)", function() {
Expand Down Expand Up @@ -356,7 +356,7 @@ contract("Perpetual: proposer.js", function(accounts) {
let pricesToPropose = "0.000006";
for (let i = 0; i < fundingRateIdentifiersToTest.length; i++) {
const priceFeed = proposer.priceFeedCache[hexToUtf8(fundingRateIdentifiersToTest[i])];
priceFeed.setCurrentPrice(pricesToPropose);
priceFeed.setHistoricalPrice(pricesToPropose);
priceFeed.setLastUpdateTime(latestProposalTime);
}
await proposer.updateFundingRates(true);
Expand Down Expand Up @@ -402,7 +402,7 @@ contract("Perpetual: proposer.js", function(accounts) {
assert.equal(pricesToPropose.length, fundingRateIdentifiersToTest.length);
for (let i = 0; i < fundingRateIdentifiersToTest.length; i++) {
const priceFeed = proposer.priceFeedCache[hexToUtf8(fundingRateIdentifiersToTest[i])];
priceFeed.setCurrentPrice(pricesToPropose[i]);
priceFeed.setHistoricalPrice(pricesToPropose[i]);
priceFeed.setLastUpdateTime(latestProposalTime);
}

Expand All @@ -428,9 +428,9 @@ contract("Perpetual: proposer.js", function(accounts) {
for (let i = 0; i < fundingRateIdentifiersToTest.length; i++) {
const priceFeed = proposer.priceFeedCache[hexToUtf8(fundingRateIdentifiersToTest[i])];
if (i % 2 === 0) {
priceFeed.setCurrentPrice("1");
priceFeed.setHistoricalPrice("1");
} else {
priceFeed.setCurrentPrice("-1");
priceFeed.setHistoricalPrice("-1");
}
priceFeed.setLastUpdateTime(latestProposalTime);
}
Expand Down Expand Up @@ -476,10 +476,10 @@ contract("Perpetual: proposer.js", function(accounts) {
// Update once to load priceFeedCache.
await proposer.update();

// Force `getCurrentPrice()` to return null so that bot fails to fetch prices.
// Force `getHistoricalPrice(x)` to return null so that bot fails to fetch prices.
for (let i = 0; i < fundingRateIdentifiersToTest.length; i++) {
const priceFeed = proposer.priceFeedCache[hexToUtf8(fundingRateIdentifiersToTest[i])];
priceFeed.setCurrentPrice(null);
priceFeed.setHistoricalPrice(null);
}

// Update again
Expand Down