6701 EC removeOracles #6977

turadg · 2023-02-11T23:48:51Z

closes: #6701
stacks on #6975

Description

Lets EC disable an oracle in the pool

Security Considerations

After being disabled, pushPrice no longer works. getStatus does. I figure that's good for checking if it's disabled.

Scaling Considerations

The durable oracleKit is removed from the oracles map but it's not "deleted" in that someone may be holding a reference to its oracle facet from an offer result.

Documentation Considerations

The new feature will be documented in Econ Gov GUI and its docs.

Testing Considerations

Uses the test-oracle-integration test to do both governance and pushes through the smart wallet. Some of on-chain paths aren't tested. The disabling is low enough that I think the wallet tests suffice.

packages/inter-protocol/src/price/priceOracleKit.js

Chris-Hibbert · 2023-02-12T21:48:10Z

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

+      message: 'pushPrice for disabled oracle',
+    },
+  );
+});


We should test that aggregation continues to work.

What's the minimum number of sources that are required in order to calculate an aggregation? Does the disabled source count toward that total?

We should test that aggregation continues to work.

Ok, will do.

What's the minimum number of sources that are required in order to calculate an aggregation?

It's specified in minSubmissionCount of the config when creating a feed. It gets read here,

agoric-sdk/packages/inter-protocol/src/price/roundsManager.js

Lines 363 to 367 in adc0836

if (

details.get(roundId).submissions.length <

details.get(roundId).minSubmissions

) {

return [false, 0];

Does the disabled source count toward that total?

It doesn't change anything. It's just as if the oracle wallet never pushed a price.

Chris-Hibbert · 2023-02-12T21:48:37Z

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

@@ -316,7 +316,8 @@ test.serial('errors', async t => {
  );
 });

-test.serial('govern addOracle', async t => {
+// test both addOracles and removeOracles in same test to reuse the lengthy the EC setup


Suggested change

// test both addOracles and removeOracles in same test to reuse the lengthy the EC setup

// test both addOracles and removeOracles in same test to reuse the lengthy EC setup

durned rebase. done

turadg · 2023-02-13T18:57:35Z

packages/inter-protocol/src/price/priceOracleKit.js

@@ -0,0 +1,124 @@
+import { Fail } from '@agoric/assert';


this is really a rename 1023f04

dckc

looks pretty thorough

part of me feels obliged to reproduce your results using the CLI and smoke test script(s), but I think the whole team is going to be doing a bunch of that very soon

dckc · 2023-02-13T19:03:46Z

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

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


this will only work for the invitation purse, right? call it invitationPurseBalance?

Good idea. This was removed and reappeared from a faulty rebase so I'll just remove it.

dckc · 2023-02-13T19:04:28Z

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

+ * @param {string} voterAcceptanceOID
+ * @returns {Promise<import('@agoric/smart-wallet/src/invitations').ContinuingInvitationSpec>}
+ */
+export const voteForOpenQuestion = async (


dckc · 2023-02-13T19:05:50Z

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

@@ -311,22 +281,35 @@ test('govern offerFilter', async t => {
  // 44 tested above
  t.is(currentState.offerToUsedInvitation[46].value[0].description, 'Voter0');

+  // Call for a vote ////////////////////////////////


consider t.log('Call for a vote')?

I'm really on the fence about that. Is it useful log output? It won't interleave with debugging output.

I'm opting to punt on this change until there's a clear best practice.

Is it useful log output?

I like it when the t.log() output tells a story...

It won't interleave with debugging output.

I think it does with failures of t.is() and t.deepEqual() and such. But yeah, it's out of sync with console.log().

Punting is fair. Thanks for considering it.

dckc · 2023-02-13T19:06:46Z

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

-    previousOffer: 46,
-    invitationMakerName: 'makeVoteInvitation',
-    invitationArgs: harden([yesPosition, questionHandle]),
+    previousOffer: 44,


Help me understand why previousOffer is changing?

This is the differ getting confused. This invitation is for VoteOnPauseOffers which needs 44, instead of makeVoteInvitation which needed 46.

But I've been replacing numbers with strings so I'll do that here too.

dckc · 2023-02-13T19:10:17Z

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

+    removeOracle: async oracleId => {
+      trace('deleteOracle', oracleId);
+      const kit = oracles.get(oracleId);
+      kit.admin.disable();


I wonder why disable rather than revoke. Can it be enabled again?

(thanks for adding the JS standard revoke() thing to https://github.com/dckc/awesome-ocap/wiki/CareTaker )

that was the idea. right now there's no enable() method but the state is a boolean so it could be toggled.

dckc · 2023-02-13T19:13:56Z

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

+  t.like(await E(creatorFacet).getRoundData(2), {
+    roundId: 2n,
+    // median of two is their average
+    answer: 250n,


dckc · 2023-02-13T19:14:31Z

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

@@ -316,7 +316,8 @@ test.serial('errors', async t => {
  );
 });

-test.serial('govern addOracle', async t => {
+// test both addOracles and removeOracles in same test to reuse the lengthy the EC setup


turadg requested review from dckc and Chris-Hibbert February 11, 2023 23:48

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

turadg force-pushed the 6701-ec-removeOracles branch from da17baf to 8f93647 Compare February 12, 2023 00:24

This was referenced Feb 12, 2023

6701 ec oracles list #6963

Closed

6707 pause feed #6982

Closed

Chris-Hibbert reviewed Feb 12, 2023

View reviewed changes

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

Base automatically changed from 6701-ec-addOracles to master February 13, 2023 00:21

turadg force-pushed the 6701-ec-removeOracles branch from 8f93647 to bcf33fe Compare February 13, 2023 02:07

turadg requested a review from Chris-Hibbert February 13, 2023 02:07

turadg force-pushed the 6701-ec-removeOracles branch from bcf33fe to 70e196e Compare February 13, 2023 18:55

turadg commented Feb 13, 2023

View reviewed changes

dckc approved these changes Feb 13, 2023

View reviewed changes

turadg added 5 commits February 13, 2023 11:41

chore(types): remove unavailable type

f0c0b2d

refactor: mv priceOracleKit

3728ece

test: extract voteForOpenQuestion

61c9c0f

feat(price): removeOracles by EC

8720d22

test(smartWallet): simplify

9b6e2cd

turadg force-pushed the 6701-ec-removeOracles branch from 70e196e to 9b6e2cd Compare February 13, 2023 19:42

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

mergify bot merged commit 943b3a4 into master Feb 13, 2023

mergify bot deleted the 6701-ec-removeOracles branch February 13, 2023 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6701 EC removeOracles #6977

6701 EC removeOracles #6977

turadg commented Feb 11, 2023 •

edited by dckc

Loading

Chris-Hibbert Feb 12, 2023

turadg Feb 12, 2023

Chris-Hibbert Feb 12, 2023

dckc Feb 13, 2023

turadg Feb 13, 2023

turadg Feb 13, 2023

dckc left a comment

dckc Feb 13, 2023

turadg Feb 13, 2023

dckc Feb 13, 2023

dckc Feb 13, 2023

turadg Feb 13, 2023

dckc Feb 13, 2023

dckc Feb 13, 2023

turadg Feb 13, 2023

dckc Feb 13, 2023

turadg Feb 13, 2023

dckc Feb 13, 2023

dckc Feb 13, 2023

	if (
	details.get(roundId).submissions.length <
	details.get(roundId).minSubmissions
	) {
	return [false, 0];

	// test both addOracles and removeOracles in same test to reuse the lengthy the EC setup
	// test both addOracles and removeOracles in same test to reuse the lengthy EC setup

6701 EC removeOracles #6977

6701 EC removeOracles #6977

Conversation

turadg commented Feb 11, 2023 • edited by dckc Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

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

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

turadg commented Feb 11, 2023 •

edited by dckc

Loading