fix(uniswap-pf): fixes a low volume bug in the uniswap price feed#2465
fix(uniswap-pf): fixes a low volume bug in the uniswap price feed#2465mrice32 merged 2 commits intoUMAprotocol:masterfrom
Conversation
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
chrismaree
left a comment
There was a problem hiding this comment.
This is great. really clever implementation. I left one small nit and one question but otherwise this looks good to me!
| let startBlock = Infinity; // Arbitrary initial value > 0. | ||
|
|
||
| // For loop continues until the start block hits 0 or the first event is before the earlest lookback time. | ||
| for (let i = 0; !(startBlock === 0 || events[0]?.timestamp <= earliestLookbackTime); i++) { |
| } | ||
|
|
||
| async _getSortedSyncEvents(startBlock, endBlock) { | ||
| const events = await this.uniswap.getPastEvents("Sync", { fromBlock: startBlock, toBlock: endBlock }); |
There was a problem hiding this comment.
tiny nit: it might be cleaner to just refer to fromBlock and toBlock through out this file so we don't need to have this translation between fromBlock: startBlock & toBlock: endBlock. feel free to ignore this comment but it just seems that it would be more consistent to always use fromBlock and toBlock notation for these vars.
| const endBlock = events[0] ? events[0].blockNumber - 1 : "latest"; | ||
|
|
||
| // By taking larger powers of 2, this doubles the lookback each time. | ||
| startBlock = Math.max(0, latestBlockNumber - lookbackBlocks * 2 ** i); |
| // By taking larger powers of 2, this doubles the lookback each time. | ||
| startBlock = Math.max(0, latestBlockNumber - lookbackBlocks * 2 ** i); | ||
|
|
||
| const newEvents = await this._getSortedSyncEvents(startBlock, endBlock).then(newEvents => { |
There was a problem hiding this comment.
seeing this does not run in parallel any more, what is the upper bound on how long this update function can now take to run, assuming this needs to search until the 0th block and never finds any sync events( for example a mis parameterized feed)?
There was a problem hiding this comment.
Check my math here:
avg_block_time = 13.5;
standard_lookback = 7200; // Standard lookback we use with no TWAP.
total_blocks = 14140850; // ~block number in a year
lookback_in_blocks = standard_lookback / avg_block_time = 533.333333333;
iterations = log2(total_blocks / lookback_in_blocks) = 14.6944718176; // Number of times needed to double to reach the total blocks.
This means it's basically 15 ish loops, which would equate to O(15) serial calls, since no timestamp calls would be required in the average loop since a timestamp call would mean we found a price, terminating the looping.
Does that match your expectation?
There was a problem hiding this comment.
If we prefer, we can change this from 2 to 3 or even 10, etc if we want to make the ramp up faster. For instance, if we change it to 10, that brings the max iterations down from 15 to 5 ish, but it would mean that the queries would be much more painful for the node to fulfill much more quickly.
nicholaspai
left a comment
There was a problem hiding this comment.
Looks good! I'm surprised this doesn't change any of the unit tests, do we currently test the following situation:
- one data point within the look back window: twap should factor in the single data point and the last one before it
- no data point in the window: twap should only factor in the last one before it(?)
| // Approximate the first block from which we'll need price data from based on the | ||
| // lookback and twap length: | ||
| const lookbackWindow = this.twapLength + this.historicalLookback; | ||
| const currentTime = await this.getTime(); |
There was a problem hiding this comment.
Can you assume that getTime is async?
There was a problem hiding this comment.
We cannot, but await just passes the value through if it isn't a promise: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await, so no special casing should be necessary.
@nicholaspai Both of these are tested (pretty much):
I may try to add a test where the loop has to repeat to grab enough blocks, although this is kinda difficult with ganache since I don't think you can create blocks with arbitrary block numbers. |
|
After this PR got merged at cf2570e I fail to run getMedianHistoricalPrice.js with following error: Before cf2570e I could run getMedianHistoricalPrice script without errors. Could this commit be the issue or it has something to do with my local setup? |
Can you try upgrading your node version to >14? The |
Thanks a lot, this solved the issue! |
Motivation
The Uniswap price feed has a bug where few trades (last trade long before the lookback window) will cause the uniswap price feed to just take the spot price rather than the TWAP.
Summary
Reimplemented the update function to streamline and simplify the implementation by condensing most of the complex looping logic into a single looping block.
To fix the issue, this adds an exponential lookback to quickly make larger and larger historical queries to always find the correct most recent sync event before the window.
Testing
Check a box to describe how you tested these changes and list the steps for reviewers to test.
Note: this could probably use some specific tests for low volume pools. Will add.
Issue(s)
N/A