Adopt dapp-oracle contracts into Zoe #1952
Conversation
There was a problem hiding this comment.
This is a review of oracle.js and test-oracle.js only. I will make a PR with the changes that I suggest - feel free to take from it as you wish.
Edit: PR for oracle.js here: #1953
michaelfig
force-pushed
the
mfig/adopt-oracle-contracts
branch
from
40f336f
to
212c446
Compare
|
R4R. |
There was a problem hiding this comment.
I couldn't do as deep of a dive as I would have liked due to time constraints, but I think we should make the interface between the priceAuthority and the priceAggregator simpler so there are fewer back and forth calls. Also, the baseValueOut and baseValueIn I found confusing. I think we have to explain them a lot more, or use more familiar terms, like units.
| return createQuote(calcAmountOut => ({ | ||
| amountIn, | ||
| amountOut: calcAmountOut(amountIn), | ||
| })); |
There was a problem hiding this comment.
This seems pretty complex for getting a quote amount and then minting a payment. Is there a reason not to take in a mint as one of the opts and do the minting within the priceAggregator? I understand we need to actually calculate the quoted amount, but can we make the API for that easier?
| }; | ||
|
|
||
| // Calculate the quote. | ||
| const quote = priceQuery(calcAmountOut, calcAmountIn); |
There was a problem hiding this comment.
The way in which the priceAuthority calls the priceAggregator which calls the priceAuthority (createQuote -> priceQuery -> calcAmountOut/calcAmountIn ) seems convoluted to me. Can we take in the mint and a function for getting the quoted amount as an argument in the price authority and move the code for producing a quote, given a mint, into some shared library?
| * | ||
| * @param {PriceQuoteValue} quote | ||
| */ | ||
| const authenticateQuote = async quote => { |
There was a problem hiding this comment.
When I think of authenticate I think of verifying something that someone has already made (minted). This is the minting. I think we should just call this mintQuote where a quote is made up of a quoteAmount and quotePayment
There was a problem hiding this comment.
I think this should be in the priceAuthority code or be a shared library.
| await priceAuthorityAdmin.fireTriggers( | ||
| makeCreateQuote({ overrideValueOut: median, timestamp }), | ||
| ); |
There was a problem hiding this comment.
I don't understand this call.
| }; | ||
|
|
||
| // Authenticate the quote by minting it with our quote issuer, then publish. | ||
| const authenticatedQuote = await authenticateQuote([quote]); |
There was a problem hiding this comment.
Hm, we could potentially pass a promise for a quote. Why do we need to await?
There was a problem hiding this comment.
Because the getPriceNotifier is lossy. We can fix that by deprecating it and introducing a new getQuoteNotifier, using @erights new lossy/lossless hybrid (such as was implemented for map-unum.js).
|
|
||
| pushResult(result); | ||
| await updateQuote( | ||
| [...oracleRecords].map(({ lastSample }) => lastSample), |
There was a problem hiding this comment.
huh, is this a "rolling median" where any time we get a response back from an oracle, we recalculate? That seems strange to me, but I don't know if I can see a downside directly. Would the alternative (waiting for responses from all the oracles) mean that one oracle could cause a liveness problem?
There was a problem hiding this comment.
We may want to explain this more.
There was a problem hiding this comment.
Yes. It is hoped that people will replace this rolling median with more sophisticated ways of collation.
We should further distinguish the rolling median from the underlying sample collector. Adding an oracle to the collector should be able to provide a normalize method that knows how to interpret just that oracle's results (default to the parseInt normaliser that we currently have).
michaelfig
force-pushed
the
mfig/adopt-oracle-contracts
branch
from
4ef5a56
to
7991d3a
Compare
michaelfig
force-pushed
the
mfig/adopt-oracle-contracts
branch
from
7991d3a
to
1f49dc0
Compare
Closes #1944, Enables Agoric/dapp-oracle#11
This is a large chunk to swallow, but I'm happy to say it only required minor tweaks from what was in dapp-oracle to land it in Zoe with tests passing.
I'm mostly interested in getting a starting point, from which we can improve. Please review in as much detail as you'd like.