Adding market with price logger; Integration into Futarchy oracle; #30
Conversation
Codecov Report
@@ Coverage Diff @@
## master #30 +/- ##
=========================================
+ Coverage 85.01% 91.5% +6.48%
=========================================
Files 25 27 +2
Lines 634 671 +37
Branches 106 111 +5
=========================================
+ Hits 539 614 +75
+ Misses 95 57 -38
Continue to review full report at Codecov.
|
…ses standard markets
cag
force-pushed
the
std_market_w_price
branch
from
19ddba2 to
a3e9908
Compare
Std market w price tests
| else | ||
| priceIntegral += price * (now - startDate); | ||
| lastTrade = now; | ||
| } |
There was a problem hiding this comment.
Should end date be taken into account?
There was a problem hiding this comment.
Ah actually now I think this is fine
There was a problem hiding this comment.
endDate is set once the market is closed. Once the market is closed trading is not possible anymore and the logPrice function cannot be executed anymore. With the current implementation endDate is not used at all and should probably be removed.
There was a problem hiding this comment.
Yeah. I think it is worth noting that the final transaction that is executed on the market, as well as subsequent simultaneous transactions (like if somebody wrote a contract which combined transactions, thus causing a 0 time gap), do not count towards the final average.
There was a problem hiding this comment.
This is something we could change in the getAvgPrice function:
(priceIntegral + (marketMaker.calcMarginalPrice(this, LONG) * (now - lastTrade))) / (now - startDate);
We could also use the endDate instead of now in case endDate > 0
There was a problem hiding this comment.
Yeah, that's a possibility... I'm just worried about the cost. Might be good to cache the marginal price.
There was a problem hiding this comment.
Hm yeah, its a tradeoff. Otherwise we would have to keep track of the last price after a trade.
contracts/Oracles/FutarchyOracle.sol
Outdated
| ) | ||
| public | ||
| { | ||
| // Deadline is in the future | ||
| require(_deadline > now); | ||
| require(_deadline > 0); |
There was a problem hiding this comment.
Should deadline be after start date or now?
There was a problem hiding this comment.
Ah, deadline typically refers to an absolute point in time. I recommend either renaming to trading period or restoring the original logic.
There was a problem hiding this comment.
Agree the name is missleading. Is there an easy way to set the timestamp in testrpc to a future timestamp? I found only the wait command, which makes more sense if time-periods are used. Lets rename it to tradingPeriod.
Std market w price 2
No description provided.