make Chainlink the canonical priceAggregator #6629

turadg · 2022-12-05T22:41:07Z

closes: #6628
part of #6571

Description

Make priceAggregatorChainlink the price aggregator contract. Updates CLI command and smoketest script to use the new result struct.

Updates the contract itself for code quality and compatibility with the Smart Wallet (using continuing invitation pattern and keying oracles by wallet address).

Security Considerations

Increases security by implementing round constraints on oracle price pushes. No new privileges.

Documentation Considerations

This changes the priceAggregator to be Chainlink style, which is different than https://github.com/Agoric/dapp-oracle/ expects. OTOH it's already broken and serves mostly an illustration instead of a functioning dapp.

Testing Considerations

Tested manually with https://github.com/Agoric/agoric-sdk/blob/6ff21e34936bdaad17ecd3d0e40dc7493a75cac3/packages/agoric-cli/test/agops-oracle-smoketest.sh

dckc

To what extent does closing #6628 mean claiming we're done with this contract?

Comparing the scope of this to the 5 tasks listed in #6571, it seems to claim to address:

make it support continuing invitation
FIXMEs (e.g. We throw away the updater but shouldn’t)
wire it into integration tests and fix whatever else comes up

but not these:

make durable
replace makeLegacyMap usage

I'm also curious whether...

limits work by message size?

... is in scope.

Is there more vstorage work to do after this? (#5854)

I suppose #5060 is likely to impact only the vaultFactory contract and not this one.

dckc · 2022-12-20T02:03:47Z

packages/agoric-cli/test/agops-oracle-smoketest.sh

-bin/agops oracle pushPrice --price 1.01 --oracleAdminAcceptOfferId "$ORACLE_OFFER_ID" >|"$PROPOSAL_OFFER"
+bin/agops oracle pushPriceRound --price 101 --roundId 1 --oracleAdminAcceptOfferId "$ORACLE_OFFER_ID" >|"$PROPOSAL_OFFER"


This makes me wonder whether the oracle operators aware of quoting prices in integers without decimals. I'm looking around.

The vstorage path segment is still ATOM-USD_price_feed. Is the number of decimals available at a nearby vstorage key?

Not presently. I expect the proposal config to suffice:

agoric-sdk/packages/inter-protocol/src/proposals/price-feed-proposal.js

Lines 292 to 296 in a3962a7

priceFeedOptions: {

IN_BRAND_NAME: inBrandName,

IN_BRAND_DECIMALS: '6',

OUT_BRAND_NAME: outBrandName,

OUT_BRAND_DECIMALS: '6',

packages/governance/src/types-ambient.js

dckc · 2022-12-20T02:09:04Z

packages/inter-protocol/src/proposals/price-feed-proposal.js

+            minSubmissionCount: 2,
+            minSubmissionValue: 1,
+            maxSubmissionCount: 5,
+            maxSubmissionValue: 99999,


What's this limitation of 100k - 1? I aim to look around a bit more...

Nothing purposeful. It's much bigger in

agoric-sdk/packages/inter-protocol/scripts/price-feed-core.js

Line 11 in a3962a7

maxSubmissionValue: 2n ** 256n,

Any suggestions?

I don't understand the purpose of the limitation. What is it limiting? What bad thing might happen if there's no limit? Why is 100k a plausible number?

What is it limiting?

It's limiting the maximum value in a submission.

I don't understand the purpose of the limitation.

One is that it came in the port of https://github.com/smartcontractkit/chainlink/blob/b045416ebca769aa69bde2da23b5109abe07a8b5/contracts/src/v0.6/FluxAggregator.sol#L155-L156

What bad thing might happen if there's no limit?

If enough oracles were compromised they could push the median up to a ludicrous price. Or if there is a software bug affecting all of them it wouldn't even require malice. TMK it's a hedge against run-away values.

Why is 100k a plausible number?

I don't know that it is. This code isn't of the economics or risk, just the functionality. Isn't that sufficient?

dckc · 2022-12-20T02:12:06Z

packages/inter-protocol/test/smartWallet/test-oracle-integration.js

-import { INVITATION_MAKERS_DESC } from '@agoric/zoe/src/contracts/priceAggregator.js';
-import buildManualTimer from '@agoric/zoe/tools/manualTimer.js';
-import { AmountMath } from '@agoric/ertp';
 import { coalesceUpdates } from '@agoric/smart-wallet/src/utils.js';
+import { INVITATION_MAKERS_DESC } from '@agoric/zoe/src/contracts/priceAggregatorChainlink.js';
+import buildManualTimer from '@agoric/zoe/tools/manualTimer.js';
 import { ensureOracleBrands } from '../../src/proposals/price-feed-proposal.js';
-import { makeDefaultTestContext } from './contexts.js';
 import { headValue } from '../supports.js';
+import { makeDefaultTestContext } from './contexts.js';


Is there anything to this change besides line order? I can't tell. Is there some motivation for changing the order?

There was some import to remove and I used Organize imports in VS Code (from TypeScript LSP)

packages/zoe/src/contractSupport/priceAuthorityTransform.js

dckc

convention calls for capitalizing makePushPriceInvitation

and I don't believe oracleAddr is actually always a string. The tests seem to use an instance.

I haven't tried reproducing the integration testing results; that's next...

dckc · 2022-12-20T20:59:19Z

packages/zoe/test/unitTests/contracts/test-priceAggregatorChainlink.js

-    const aggregatorInstallation = await E(zoe).installBundleID(
-      'b1-aggregator',
-    );
+const defaultConfig = {


Should defaultConfig be exposed more widely? Is it something operators should know about? Or is it just arbitrary test constants?

arbitrary test constants. "default" merely for this module's tests. I think the "in this test module" is implied but let me know if you think it needs a comment or something

That's fine.

I was just asking about the possibility that the numbers are not arbitrary and in fact have some relevance to production operation.

dckc · 2022-12-20T21:03:13Z

packages/zoe/test/unitTests/contracts/test-priceAggregatorChainlink.js

+  await E(pricePushAdminA).pushResult({ roundId: 1, unitPrice: 100n });
+  await E(pricePushAdminB).pushResult({ roundId: 1, unitPrice: 200n });
+  await E(pricePushAdminC).pushResult({ roundId: 1, unitPrice: 300n });
  await oracleTimer.tick();

  const round1Attempt1 = await E(aggregator.creatorFacet).getRoundData(1);


100, 200, and 300 seem pretty far apart; does the aggregator consume them no matter how far apart they are? Or is there a point where it says "sources don't agree; nothing available at this time"?

Right, it lets the prices push as long as they're within the minSubmissionValue and maxSubmissionValue. Extremity of gap doesn't matter because the feed only exposes the median.

I'm missing something about how scale works in this contract. DAI has 18 decimals. the .value of an ERTP amount of 1 DAI is bigger than 100K. How could this possibly work?

dckc · 2022-12-20T21:20:55Z

packages/zoe/src/contracts/priceAggregatorChainlink.js

   * @param {Timestamp} blockTimestamp
   */
-  const oracleRoundStateSuggestRound = (_oracle, blockTimestamp) => {
-    const oracle = oracleStatuses.get(_oracle);
+  const oracleRoundStateSuggestRound = (oracleAddr, blockTimestamp) => {


I wonder what an oracleAddr is... I think it's: an address to which an invitation to this contract was sent. Why would this contract know the addresses? hm.

I wonder what an oracleAddr is... I think it's: an address to which an invitation to this contract was sent.

Correct. I'll make clearer by documentation.

Why would this contract know the addresses?

It needs to know the oracles and it's keyed on their public address. What's the concern?

The fact that the .instance test works suggests that the keys don't need to be addresses of the validators; the validators could be identified as 'A', 'B', and 'C' and the contract would work fine, yes? The convention of using addresses seems to be external to the contract.

Oh yes, good point. I'll change it to be oracleKey.

dckc · 2022-12-20T21:26:25Z

packages/zoe/src/contracts/priceAggregatorChainlink.js

+    /**
+     * An "oracle invitation" is an invitation to be able to submit data to
+     * include in the priceAggregator's results.
+     *
+     * The offer result from this invitation is a OracleAdmin, which can be used
+     * directly to manage the price submissions as well as to terminate the
+     * relationship.
+     *
+     * @param {string} oracleAddr
+     */
+    makeOracleInvitation: async oracleAddr => {


how refreshing! docs!

dckc · 2022-12-20T21:35:49Z

packages/zoe/src/contracts/priceAggregatorChainlink.js

+       * reported data.
+       *
+       * @param {ZCFSeat} seat
+       * @returns {Promise<{admin: OracleAdmin<PriceRound>, invitationMakers: {makePushPriceInvitation: (result: PriceRound) => Promise<Invitation<void>>} }>}


Suggested change

* @returns {Promise<{admin: OracleAdmin<PriceRound>, invitationMakers: {makePushPriceInvitation: (result: PriceRound) => Promise<Invitation<void>>} }>}

* @returns {Promise<{admin: OracleAdmin<PriceRound>, invitationMakers: {MakePushPriceInvitation: (result: PriceRound) => Promise<Invitation<void>>} }>}

The sparsely-documented continuing invitation pattern seems to including capitalizing the fields of invitation makers. For example:

invitationMakers: Far('invitation makers', { AdjustBalances: vault.makeAdjustBalancesInvitation, CloseVault: vault.makeCloseInvitation, TransferVault: vault.makeTransferInvitation, }),

I think that's to avoid colliding with existing property names, as noted for assertKeywordName in Zoe.

I can imagine this argument is not very strong, but if we're not going to follow the pattern in this respect, I would want to discuss it more widely.

I guess the AdjustBalances example suggests just PushPrice.

Ah... and I think the method name is supposed to match the invitation description, which is also PushPrice.

Good call. invitationMakers.makePushPriceInvitation is redundant. Changed to invitationMakers.PushPrice.

packages/zoe/src/contracts/priceAggregatorChainlink.js

dckc · 2022-12-20T21:40:25Z

packages/zoe/src/contracts/priceAggregatorChainlink.js

+    /** @param {string} oracleAddr */
+    async initOracle(oracleAddr) {


Is this really a string? The tests suggest otherwise:

const pricePushAdminA = await E(aggregator.creatorFacet).initOracle( priceOracleA.instance, );

Thanks for raising. That test was written for the agsolo era. We only support smart wallet oracles now so I've amended to make that clear. This is also necessary for durability.

dckc

let's go.

I'm fuzzy on some of the details. And I didn't reproduce the scripted integration test. But... onward.

turadg force-pushed the 6628-chainLink-working branch from 30f8d44 to c905450 Compare December 5, 2022 23:08

turadg mentioned this pull request Dec 8, 2022

feat: assertAllDefined throws "missing X" from object literal shorthand #6651

Merged

turadg force-pushed the 6628-chainLink-working branch from 4cfd391 to 6ff21e3 Compare December 20, 2022 00:07

turadg requested review from michaelfig and dckc December 20, 2022 00:14

turadg marked this pull request as ready for review December 20, 2022 00:14

turadg changed the title ~~6628 chain link working~~ make Chainlink the canonical priceAggregator Dec 20, 2022

dckc reviewed Dec 20, 2022

View reviewed changes

dckc requested changes Dec 20, 2022

View reviewed changes

turadg force-pushed the 6628-chainLink-working branch from 6ff21e3 to d0ef5e9 Compare December 21, 2022 18:41

turadg changed the base branch from master to ta/zoeService-type December 21, 2022 18:41

turadg marked this pull request as draft December 21, 2022 18:41

Base automatically changed from ta/zoeService-type to master December 21, 2022 19:22

turadg added 17 commits December 21, 2022 11:23

build: priceAggregatorChainlink

a960dda

feat(oracle): support continuing invitation

ed950f5

feat(oracle!): roundId in price results

e1f7488

test: cleanup

a03a92d

test: context type

d63ddb2

chore(types): cast ManualTimer

28d1564

chores(types): RoundData

274a10b

docs: why not brands key on terms

a28c8f7

style: remove superflous underscores

2a2e559

refactor: clarity

9f6fd6b

test: getRoundStartNotifier

642cb37

feat(rpc): publish latestRound

9e2617b

test: chainlink-style price feed

4899d40

style: organize imports

efe9d11

test: refactor to named params

8f7601c

feat: publish price quotes

e3e3984

feat!: publish PriceDescription instead of PriceQuote

30d3966

turadg added 9 commits December 21, 2022 11:23

chore: chainlink aggregator uses bech32 key

d7ad277

test: second oracle for smoketest

5e58585

fix(chainlink): publish new quotes

9914899

refactor: clearer error message handling

bb2b503

docs

443ea1c

chore(chainlink)!: 'data' string to 'unitPrice' bigint

a8c836c

refactor(chainlink): simplify pushResult fn

61662e7

refactor: rename PushPrice invitationMaker

21018a1

chore(chainlink)!: only smart-wallet oracles

8e61373

turadg force-pushed the 6628-chainLink-working branch from d0ef5e9 to 8e61373 Compare December 21, 2022 19:58

turadg marked this pull request as ready for review December 21, 2022 20:48

turadg requested a review from dckc December 21, 2022 20:48

turadg mentioned this pull request Dec 21, 2022

verify priceAggregatorChainlink.js is production-ready #6571

Closed

5 tasks

dckc approved these changes Dec 21, 2022

View reviewed changes

turadg added the automerge:no-update (expert!) Automatically merge without updates label Dec 21, 2022

mergify bot merged commit e74278c into master Dec 21, 2022

mergify bot deleted the 6628-chainLink-working branch December 21, 2022 21:40

jacquesvcritien mentioned this pull request Jan 5, 2023

added features to priceAggregatorChainlink #6752

Closed

jacquesvcritien mentioned this pull request Jan 15, 2023

feat(fluxAggregator): add startedBy to latestRoundPublisher #6794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make Chainlink the canonical priceAggregator #6629

make Chainlink the canonical priceAggregator #6629

turadg commented Dec 5, 2022 •

edited

Loading

dckc left a comment

dckc Dec 20, 2022

dckc Dec 20, 2022

turadg Dec 21, 2022

dckc Dec 20, 2022

turadg Dec 21, 2022

dckc Dec 21, 2022

turadg Dec 21, 2022

dckc Dec 20, 2022

turadg Dec 20, 2022

dckc left a comment

dckc Dec 20, 2022

turadg Dec 21, 2022 •

edited

Loading

dckc Dec 21, 2022

dckc Dec 20, 2022

turadg Dec 21, 2022

dckc Dec 21, 2022

dckc Dec 20, 2022

turadg Dec 21, 2022

dckc Dec 21, 2022

turadg Dec 21, 2022

dckc Dec 20, 2022

dckc Dec 20, 2022

dckc Dec 20, 2022

dckc Dec 20, 2022

turadg Dec 21, 2022

dckc Dec 20, 2022

turadg Dec 21, 2022

dckc left a comment

		bin/agops oracle pushPrice --price 1.01 --oracleAdminAcceptOfferId "$ORACLE_OFFER_ID" >\|"$PROPOSAL_OFFER"
		bin/agops oracle pushPriceRound --price 101 --roundId 1 --oracleAdminAcceptOfferId "$ORACLE_OFFER_ID" >\|"$PROPOSAL_OFFER"

	priceFeedOptions: {
	IN_BRAND_NAME: inBrandName,
	IN_BRAND_DECIMALS: '6',
	OUT_BRAND_NAME: outBrandName,
	OUT_BRAND_DECIMALS: '6',

	* @returns {Promise<{admin: OracleAdmin<PriceRound>, invitationMakers: {makePushPriceInvitation: (result: PriceRound) => Promise<Invitation<void>>} }>}
	* @returns {Promise<{admin: OracleAdmin<PriceRound>, invitationMakers: {MakePushPriceInvitation: (result: PriceRound) => Promise<Invitation<void>>} }>}

		/** @param {string} oracleAddr */
		async initOracle(oracleAddr) {

make Chainlink the canonical priceAggregator #6629

make Chainlink the canonical priceAggregator #6629

Conversation

turadg commented Dec 5, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

turadg commented Dec 5, 2022 •

edited

Loading

turadg Dec 21, 2022 •

edited

Loading