6701 EC addOracles to a governed fluxAggregator #6975

turadg · 2023-02-11T20:15:09Z

part of: #6701
split from #6963 and cleaned up for easier review
Next will be removeOracles which requires the caretaker pattern

Description

Grants EC the power to addOracles to a price feed.

feat(price): addOracles by EC

Do that required giving the EC the ability to VoteOnApiCall:

feat(ec-charter): VoteOnApiCall

Supporting changes:

refactor(fluxAggregator): split singleton / contract
feat: puppetGovernance setup tools
fix(governApi): harden returns
test(smartWallet): extract helpers from psm integration

PR also includes some incidental cleanup:

BREAKING CHANGE: remove 'aggregators' from space (I don't think it worked anymore. cc @michaelfig )
refactor: DRY WalletName.depositFacet
test(price): rename test-fluxAggregator
test(price): simplify fluxAggregator test

Security Considerations

Makes the fluxAggregator subject to changes by governance. This is granted only to the EC.

Scaling Considerations

--

Documentation Considerations

removing aggregators from the space might impact REPL usage. Users are better off doing a lookup imo.

Testing Considerations

New integration test of FA governance with wallet.

michaelfig · 2023-02-12T00:17:32Z

BREAKING CHANGE: remove 'aggregators' from space (I don't think it worked anymore. cc @michaelfig )

I'm okay with this. Its intention was to provide control over the oracles to the BLDer DAO. I presume there's enough machinery added to the bootstrap promise spaces so that BLDer DAO proposals can do the same sorts of things that the EC can do?

Chris-Hibbert · 2023-02-12T22:04:33Z

packages/inter-protocol/src/price/fluxAggregator.contract.js

+    quoteMintP,
+    zcf.saveIssuer(E(quoteMintP).getIssuer(), 'Quote'),
+  ]);
+  const quoteKit = {


If the fluxAggregator has a quoteMint, it seems like that should be publicly visible. I see that it eventually makes it way to the priceAuthorities, which presumably share it, but shouldn't it be available from the fluxAggregator as well?

If the fluxAggregator has a quoteMint, it seems like that should be publicly visible.

Why?

I see that it eventually makes it way to the priceAuthorities, which presumably share it

Not sure where you're seeing that. Here the PA gets just quoteKit.issuer,

const { priceAuthority } = makeOnewayPriceAuthorityKit({ createQuote: roundsManagerKit.contract.makeCreateQuote(), notifier: makeNotifierFromSubscriber(answerSubscriber), quoteIssuer: quoteKit.issuer, timer: timerPresence, actualBrandIn: brandIn, actualBrandOut: brandOut, });

The other read of quoteKit in this module is to pass it to makeRoundsManagerKit, which uses it only for,

const quoteAmount = AmountMath.make(quoteKit.brand, harden(quote)); const quotePayment = await E(quoteKit.mint).mintPayment(quoteAmount);

Sorry, I meant that the issuer should be public. The priceAuthorities provide access to their quoteIssuer (I think). Shouldn't the aggregator do the same? Are there parties to the conversation that know the aggregator and don't talk to individual PAs? If not, then I guess it's not necessary, but it feels like the Aggregator should be the main authority on the Issuer if it's the one that makes it.

Are there parties to the conversation that know the aggregator and don't talk to individual PAs?

Not on the consumption side, TMK.

Chris-Hibbert · 2023-02-12T22:06:09Z

packages/inter-protocol/src/price/fluxAggregator.contract.js

+  const { timer: timerP } = zcf.getTerms();
+
+  const quoteMintP =
+    privateArgs.quoteMint || makeIssuerKit('quote', AssetKind.SET).mint;


Do we want this to be durable/recoverable, even if it didn't come from privateArgs?

Out of scope. Those are concerns for #6853

Chris-Hibbert · 2023-02-12T22:09:41Z

packages/inter-protocol/src/proposals/utils.js

@@ -52,6 +50,12 @@ export const reserveThenGetNames = async (nameAdmin, names) =>
    names.map(name => [name]),
  );

+/**
+ * @param {string} debugName


Maybe displayName? debugName seems pejorative.

these are just jsdocs for params that were already there. The type annotation serves this PR. Changing the param name doesn't. It doesn't seem worth a drive-by either. Since you said "maybe" I take it that's not an approval requirement.

Chris-Hibbert · 2023-02-12T22:14:07Z

packages/inter-protocol/src/econCommitteeCharter.js

+   * @param {string[]} methodArgs
+   * @param {import('@agoric/time').TimestampValue} deadline
+   */
+  const makeApiInvocationInvitation = (


There's no test for the charter? Hmm. Seems like an oversight.

I'm okay with it since we need to test uses of the charter to govern and we do in the smartWallet integration tests.

Chris-Hibbert · 2023-02-12T22:19:06Z

packages/inter-protocol/test/smartWallet/contexts.js

+ * @param {Awaited<ReturnType<typeof coalesceUpdates>>} state
+ * @param {Brand<'nat'>} brand
+ */
+export const purseBalance = (state, brand) => {


passing state to a helper seems like an anti-pattern, since state is mutable. Can this be done by passing state.balances since the operation is supposed to be read-only?

it could be but since this is a test I've opted for ease of writing and reading.

Is this a blocking request?

Not if it's a lot of work. How much work does it save?

My objection wasn't on amount of work to make the change. It was that it made calling require more typing.

In this case the function was no longer used so I've removed it.

Chris-Hibbert · 2023-02-12T22:24:05Z

packages/inter-protocol/src/price/fluxAggregator.contract.js

+import { reserveThenDeposit } from '../proposals/utils.js';
+import { provideFluxAggregator } from './fluxAggregator.js';
+
+const trace = makeTracer('FluxAgg');


Suggested change

const trace = makeTracer('FluxAgg');

const trace = makeTracer('FluxAgg, false');

Chris-Hibbert · 2023-02-12T22:26:43Z

packages/inter-protocol/src/price/fluxAggregator.contract.js

-    creatorFacet: fa.creatorFacet,
-    publicFacet: fa.publicFacet,
+    creatorFacet: governorFacet,
+    // XXX this is a lot of API to put on every public facet


You can pick and choose if it shouldn't all be there.

Oh good. I think this came when I ran into some type problems in #6963 . Removed now.

Chris-Hibbert · 2023-02-12T22:34:48Z

packages/inter-protocol/test/price/test-fluxAggregator.contract.js

+  await oracleTimer.tick();
+  await oracleTimer.tick();
+  await oracleTimer.tick();
+  await oracleTimer.tick(); // --- should time out here


tickN(6) would be better and is supposed to do the same thing. I recently noticed that it doesn't seem to be equivalent. Can you see why it makes a difference?

This file was redundant with test-fluxAggregator and I removed it in a subsequent PR. I've rebased to remove it here.

Chris-Hibbert

I don't have a strong objection to .contract.js, but it's a change/addition to our naming scheme. I'd prefer to stick to our existing pattern until we have a decision to switch.

No tests and appears not to be used. Keying by 'terms' was impractical. Instead use `.lookup` by instance name

Chris-Hibbert · 2023-02-12T23:27:33Z

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

  // Create the price feed.
-  const aggregator = await E(zoe).startInstance(
-    priceAggregator,
+  /** @type {{ creatorFacet: import('@agoric/governance/src/contractGovernor.js').GovernedContractFnFacetAccess<import('@agoric/inter-protocol/src/price/fluxAggregator.contract.js').start>, publicFacet: GovernorPublic, instance: Instance, adminFacet: AdminFacet }} */


s/fluxAggregator.contract.js/fluxAggregatorContract.js

Good eyes. it’s passing CI because it’s wrapping type is not resolved and thus any. (Unfortunately JSDoc TS doesn't complain about that.) I'll get to fixing this in #6987

turadg requested review from dckc and Chris-Hibbert February 11, 2023 20:15

turadg force-pushed the 6701-ec-addOracles branch from 3f54923 to 91d6594 Compare February 11, 2023 21:04

turadg mentioned this pull request Feb 11, 2023

6701 EC removeOracles #6977

Merged

turadg force-pushed the 6701-ec-addOracles branch from 91d6594 to e972759 Compare February 11, 2023 23:56

This was referenced Feb 12, 2023

6701 ec oracles list #6963

Closed

6707 pause feed #6982

Closed

Chris-Hibbert requested changes Feb 12, 2023

View reviewed changes

Chris-Hibbert reviewed Feb 12, 2023

View reviewed changes

turadg added 11 commits February 12, 2023 15:03

refactor(fluxAggregator): split singleton / contract

5d17445

feat: puppetGovernance setup tools

7ed591b

test(price): rename test-fluxAggregator

f02786d

test(price): simplify fluxAggregator test

d5a4f6e

refactor: DRY WalletName.depositFacet

7b414cd

feat(ec-charter): VoteOnApiCall

8f3adf6

fix(governApi): harden returns

eee09d4

test(smartWallet): extract helpers from psm integration

8c91d56

BREAKING CHANGE: remove 'aggregators' from space

0ece3b9

No tests and appears not to be used. Keying by 'terms' was impractical. Instead use `.lookup` by instance name

chore: rm unused PRICE_FEEDS_MANIFEST

d6d0689

feat(price): addOracles by EC

9b6dbc5

turadg force-pushed the 6701-ec-addOracles branch from e972759 to 9b6dbc5 Compare February 12, 2023 23:21

turadg requested a review from Chris-Hibbert February 12, 2023 23:22

Chris-Hibbert reviewed Feb 12, 2023

View reviewed changes

Chris-Hibbert approved these changes Feb 12, 2023

View reviewed changes

turadg added the automerge:no-update (expert!) Automatically merge without updates label Feb 12, 2023

mergify bot merged commit bdb08a7 into master Feb 13, 2023

mergify bot deleted the 6701-ec-addOracles branch February 13, 2023 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6701 EC addOracles to a governed fluxAggregator #6975

6701 EC addOracles to a governed fluxAggregator #6975

turadg commented Feb 11, 2023

michaelfig commented Feb 12, 2023

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023 •

edited

Loading

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023

Chris-Hibbert Feb 12, 2023

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023

Chris-Hibbert left a comment

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023

	const trace = makeTracer('FluxAgg');
	const trace = makeTracer('FluxAgg, false');

6701 EC addOracles to a governed fluxAggregator #6975

6701 EC addOracles to a governed fluxAggregator #6975

Conversation

turadg commented Feb 11, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

michaelfig commented Feb 12, 2023

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

turadg Feb 12, 2023 • 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

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Feb 12, 2023 •

edited

Loading