Implement RSI Oracle #70
Conversation
| uint256 negativeDataSum = 0; | ||
|
|
||
| for (uint256 i = 1; i < _dataArray.length; i++) { | ||
| // If current day price is greater than previous day's |
There was a problem hiding this comment.
feels like this statement needs an ending
|
|
||
| it('returns the correct RSI value of 0', async () => { | ||
| const output = await subject(); | ||
| expect(output).to.be.bignumber.equal(0); |
There was a problem hiding this comment.
We have a constant for this ZERO
| negativeDataSum = 1; | ||
| } | ||
|
|
||
| uint256 hundred = 100; |
There was a problem hiding this comment.
Make this a constant instead of a declared variable. so uint256 constant HUNDRED = 100;
There was a problem hiding this comment.
And place at top of the file
| * @returns The RSI value | ||
| */ | ||
| function calculate( | ||
| uint256[] memory _dataArray |
There was a problem hiding this comment.
Add validation that array is greater than zero.
| }); | ||
| }); | ||
|
|
||
| describe('using only one seeded value', async () => { |
There was a problem hiding this comment.
Now have this test check as follows:
it('should revert', async () => {
await expectRevertError(subject());
});
There was a problem hiding this comment.
You'll have to add a test where prices don't change to cover the if statement for negativeDataSum == 0 && positiveDataSum == 0
contracts/meta-oracles/RSIOracle.sol
Outdated
| returns (uint256) | ||
| { | ||
| // Get data from price feed | ||
| uint256[] memory dataArray = timeSeriesFeedInstance.read(_dataPoints); |
There was a problem hiding this comment.
Think we need to pull _dataPoints + 1 dataPoints from the TimeSeriesFeed to get a _dataPoints length RSI
contracts/meta-oracles/RSIOracle.sol
Outdated
| * Get RSI over defined amount of data points by querying price feed and | ||
| * calculating using RSILibrary. Returns uint256. | ||
| * | ||
| * @param _dataPoints Number of data points to create average from |
There was a problem hiding this comment.
Better description might be RSI time period because it actually needs an extra day of data points
There was a problem hiding this comment.
I'd rename the variable too actually. Since we're looking for a time period not the amount of data points. The MAs it maps 1:1 but here it doesn't
|
|
||
| for (uint256 i = 1; i < _dataArray.length; i++) { | ||
| // If current day price is greater than previous day's | ||
| if (_dataArray[i - 1] > _dataArray[i]) { |
There was a problem hiding this comment.
Consider putting _dataArray[i - 1] and _dataArray[i] into local variables
| negativeDataSum = 1; | ||
| } | ||
|
|
||
| uint256 hundred = 100; |
| * | ||
| * Our implementation is simplified to the following for efficiency | ||
| * RSI = 100 - (100 * SUM(Loss) / ((SUM(Loss) + SUM(Gain))) | ||
| * |
There was a problem hiding this comment.
I know its not on the other EMA file, but let's add documentation here what a, b, and c below map to in this formula.
| }); | ||
|
|
||
| describe('using only one seeded value', async () => { | ||
| before(async () => { |
There was a problem hiding this comment.
The before and after should be consistent - otherwise you'll have weird issues with any tests that run after this. What I mean is, it needs to be beforeAll and afterAll or beforeEach and afterEach - and not a mix
| uint256 b = positiveDataSum.add(negativeDataSum); | ||
| uint256 c = a.div(b); | ||
|
|
||
| return hundred.sub(c); |
There was a problem hiding this comment.
So if the positiveDataSum and negativeDataSum are 0, then the RSI is 0?
There was a problem hiding this comment.
Good point feels like this should actually revert cause in the non-modified formula it would be undefined. Definitely don't want an edge case like this to trigger a rebalance when it really shouldn't.
| } | ||
| require( | ||
| negativeDataSum > 0 || positiveDataSum > 0, | ||
| "Not valid RSI Value" |
There was a problem hiding this comment.
Our error messaging conventions are contractName.functionName so here RSILibrary.calculate. Fix for above require as well.
| uint256 positiveDataSum = 0; | ||
| uint256 negativeDataSum = 0; | ||
|
|
||
| require( |
There was a problem hiding this comment.
Add comment explaining why this is necessary
| } | ||
| } | ||
|
|
||
| require( |
There was a problem hiding this comment.
Add comment explaining logic here
| _dataArray.length > 1, | ||
| "Length of data array must be greater than 1" | ||
| ); | ||
|
|
There was a problem hiding this comment.
Add comment saying what this for block is doing
| if (currentPrice > previousPrice) { | ||
| positiveDataSum = positiveDataSum.add(currentPrice).sub(previousPrice); | ||
| } | ||
| else { |
There was a problem hiding this comment.
knit: I think we generally do our else block the same line as the end brace
No description provided.