5346 replace uses of makeNotifierKit with makeNotifierFromSubscriber #5704

turadg · 2022-06-30T23:19:30Z

Description

This with #5739 will enable #5386

This one removes makeNotifierKit from VaultFactory without changing its API. Next will update the external API to provide Subscriber, for off-chain reads.

Review recommendation: by commit

Security Considerations

--

Documentation Considerations

--

Testing Considerations

Tests needed updates for refactors, but there should be no change in behavior.
Also test locally agoric start --reset to ensure sim-chain hasn't broken on making storageNode and marshaller no longer optional.

packages/run-protocol/test/vaultFactory/test-vaultFactory.js

packages/notifier/src/storesub.js

gibson042 · 2022-07-08T21:42:35Z

packages/notifier/src/storesub.js

+      if (!storageNode) {
+        return {};
+      }
+      return E(storageNode).getStoreKey();


I'm surprised by this divergence between storageNode being falsy vs. fulfilling with a falsy value, but it probably won't come up in practice.

do you mean you'd expect this to return falsy when storageNode is falsy? agreed. I tried returning null but ran into a type error.

I'll leave this conversation open to come back to it. Let me know if you're talking about somethign else

That, and also that a falsy storageNode is accepted while a storageNode that resolves to a falsy value (e.g., Promise.resolve(undefined)) results in an exception. I think this also intersects my concern about allowing invocation without a valid storage node, which seems to unnecessarily complicate things (e.g., note how the output from getStoreKey is meaningless in such a configuration).

I agree with the perspective. In production, the storageNode and marshaller should always be there so it would be fine to require it. In tests, they aren't always. I think it would be better to solve that, but pragmatically I'm not sure it's worth it.

If I can't remove the kludge in the tests, I'll at least remove it from @agoric/notifier and make the contracts deal with it.

In production, the storageNode and marshaller should always be there

We have the sim chain, which while perhaps not production, is supported for smart contract development work. In the sim chain, there's no chainStorage.

Thanks, that's the thing I was trying to remember. Since it's a "sim" chain, would it be reasonable to simulate chainStorage? E.g. in-memory stoageNode, identity marshaller.

oh. yeah. in-memory chainStorage would be great.

identity doesn't work as a marshaller, cuz the output has to be a string and the input is not constrained that way. But I think you can always use a vanilla output from makeMarshal().

Made it required and updated bootstrap to always provide it cbaa70b, falling back to a no-op "NullStorageNode" when ChainStorage isn't available.

packages/notifier/src/storesub.js

packages/notifier/src/types.js

gibson042 · 2022-07-08T22:13:09Z

packages/notifier/test/test-stored-subscription.js

+  publisher.publish(initialValue);
+  const storesub = makeStoredSubscriber(subscriber, storage);
+
+  t.deepEqual(await E(storesub).getStoreKey(), await E(storage).getStoreKey());


This may be highlighting a gap in makeFakeStorage; I would expect the key to be defined as part of its initialization rather than dynamic like this.

packages/notifier/test/test-stored-subscription.js

dckc · 2022-07-12T20:31:07Z

use case: list of vaults by risk

cc @btulloh

dckc · 2022-07-12T21:29:06Z

Where is the code that makes a published.vault.23423423432 thingy? Or is that yet to be done?

turadg · 2022-07-12T21:45:55Z

Where is the code that makes a published.vault.23423423432 thingy? Or is that yet to be done?

YTBD. Right now just trying to refactor existing behavior to use makeNotifierFromSubscriber. After that I can come back and put in the proper storage nodes and add tests.

dckc

Looks pretty good... at least: I don't see any serious problems.

I'm a little fuzzy on the details; I'm relying somewhat on test coverage, static typing, and discussion with @gibson042 and co.

packages/run-protocol/src/vaultFactory/vault.js

dckc · 2022-07-18T20:03:30Z

packages/run-protocol/src/vaultFactory/vaultManager.js

-  getNotifier: ({ state }) => state.assetNotifier,
+  getNotifier: ({ state }) => makeNotifierFromSubscriber(state.assetSubscriber),


Perhaps add a "we make a new notifier on each call that has no memory of previous states" comment?

The difference between these is subtle. I was a about to ask why we make a new one on each call when I remembered the whole prefix-lossy concept.

dckc · 2022-07-18T20:04:54Z

packages/run-protocol/src/vaultFactory/vaultManager.js

-  getNotifier: ({ state }) => state.assetNotifier,
+  getNotifier: ({ state }) => state.assetSubscriber,


is this an internal API? rename it to getSubscriber?

looks like it got renamed later

dckc · 2022-07-18T20:07:31Z

packages/run-protocol/test/vaultFactory/test-liquidator.js

@@ -662,7 +666,8 @@ test('update liquidator', async t => {
    }),
  );
  await eventLoopIteration();
-  govNotify = await d.managerNotified();
+  // ??? why is AT_NEXT necessary now


I don't know; I wonder if I'm sufficiently up-to-speed to review this...

looks like that went away in a later commit

dckc · 2022-07-18T20:08:48Z

packages/run-protocol/src/vaultFactory/vaultKit.js

 */
 export const makeVaultKit = (vault, assetSubscriber) => {
  const { holder, helper } = makeVaultHolder(vault);
  const vaultKit = harden({
    publicNotifiers: {
      vault: holder.getNotifier(),
-      asset: assetSubscriber,
+      asset: makeNotifierFromSubscriber(assetSubscriber),


we make this notifier just once? not a new prefix-lossy one on demand?

right, this is interim. the contract shouldn't be making Notifiers at all because that costs state tracking. Change in next PR.

dckc · 2022-07-18T20:09:43Z

packages/run-protocol/test/vaultFactory/vault-contract-wrapper.js

@@ -50,7 +50,7 @@ export async function start(zcf, privateArgs) {

  const { zcfSeat: stage } = zcf.makeEmptySeatKit();

-  const { notifier: managerNotifier } = makeNotifierKit();
+  const { subscriber: assetSubscriber } = makePublishKit();


we throw away the publisher? how does it every get updated?

It doesn't, this is the fake contract

dckc · 2022-07-18T20:10:53Z

packages/run-protocol/test/vaultFactory/test-liquidator.js

+      locked: aeth.makeEmpty(),
+      updateCount: undefined,
+    },
+    // ??? why is AT_NEXT necessary now


again, good question

dckc · 2022-07-18T20:15:29Z

packages/zoe/src/contractSupport/priceAuthority.js

- * @property {ERef<Notifier<Timestamp>>} notifier
+ * @property {ERef<Notifier<unknown>>} notifier


This is a relaxation of a type constraint? Any particular reason?

It was wrong

dckc · 2022-07-18T20:20:28Z

packages/vats/src/core/chain-behaviors.js

-    chainStorageP.resolve(undefined);
+    chainStorageP.resolve(null);


why change undefined to null?

You were considering an in-memory chainStorage; that didn't work out?

Because when there's no chain storage available, it's defined to be missing. I think that's better expressed as null.

turadg requested a review from gibson042 June 30, 2022 23:19

turadg mentioned this pull request Jun 30, 2022

fix(notifier): Make makeNotifierFromAsyncIterable lossy #5695

Closed

turadg commented Jun 30, 2022

View reviewed changes

packages/run-protocol/test/vaultFactory/test-vaultFactory.js Outdated Show resolved Hide resolved

dckc mentioned this pull request Jul 6, 2022

Consume metrics into DB/Prometheus for Econ Stress Test #5681

Closed

turadg force-pushed the ta/vault-off-chain branch from 662351c to 6594619 Compare July 8, 2022 21:21

turadg commented Jul 8, 2022

View reviewed changes

packages/notifier/src/storesub.js Outdated Show resolved Hide resolved

gibson042 reviewed Jul 8, 2022

View reviewed changes

turadg force-pushed the ta/vault-off-chain branch from 6594619 to 9591dfe Compare July 8, 2022 22:44

This was referenced Jul 8, 2022

feat(notifier): Add makeNotifierFromSubscriber #5737

Merged

refactor(notifier)!: no initial state for makePublishKit #5742

Merged

turadg force-pushed the ta/vault-off-chain branch from 3c29431 to fb4b156 Compare July 12, 2022 21:32

turadg force-pushed the ta/vault-off-chain branch from fb4b156 to 9e13972 Compare July 13, 2022 20:54

turadg requested a review from gibson042 July 13, 2022 20:58

turadg changed the title ~~ta/vault off chain~~ 5346 replace uses of makeNotifierKit with makeNotifierFromSubsciber Jul 13, 2022

turadg changed the base branch from master to gh-5413-makeNotifierFromSubscriber July 13, 2022 22:35

Base automatically changed from gh-5413-makeNotifierFromSubscriber to master July 14, 2022 00:18

turadg force-pushed the ta/vault-off-chain branch from 9e13972 to 3f195cc Compare July 14, 2022 15:07

turadg requested a review from dckc July 14, 2022 15:07

turadg marked this pull request as ready for review July 14, 2022 15:07

turadg requested a review from dtribble as a code owner July 14, 2022 15:08

turadg marked this pull request as draft July 14, 2022 15:09

turadg mentioned this pull request Jul 14, 2022

feat(notifier): makeStoredPublishKit #5766

Merged

turadg force-pushed the ta/vault-off-chain branch 2 times, most recently from d7c5b6c to 6e2ebd5 Compare July 18, 2022 15:57

turadg marked this pull request as ready for review July 18, 2022 15:58

turadg force-pushed the ta/vault-off-chain branch 2 times, most recently from b33b192 to cbaa70b Compare July 18, 2022 17:39

turadg added the automerge:rebase Automatically rebase updates, then merge label Jul 18, 2022

turadg changed the title ~~5346 replace uses of makeNotifierKit with makeNotifierFromSubsciber~~ 5346 replace uses of makeNotifierKit with makeNotifierFromSubscriber Jul 18, 2022

turadg removed request for dtribble and gibson042 July 18, 2022 18:36

dckc approved these changes Jul 18, 2022

View reviewed changes

turadg added 10 commits July 18, 2022 20:26

feat(vaultManager): asset state in off-chain storage

9276893

refactor(collateralManager): getNotifier --> getSubscriber

105d2a5

refactor(vaultManager): getNotifier --> getAssetSubscriber

e0619cc

refactor(vaultHolder): getNotifier --> getSubscriber

405a69f

refactor(amm): pool getNotifier → getSubscriber

6d024a7

refactor(runStake): getAssetNotifier → getAssetSubscriber

7c2dc83

refactor(runStakeKit): getNotifier → getSubscriber

9df44e7

refactor(inter): require storageNode and marshaller

61dfd3f

refactor(vaultFactory): validate privateArgs defined

9999436

refactor: when StorageNode required make inert at least

ec40f84

turadg force-pushed the ta/vault-off-chain branch from cbaa70b to ec40f84 Compare July 18, 2022 20:26

mergify bot merged commit 00e3c5c into master Jul 18, 2022

mergify bot deleted the ta/vault-off-chain branch July 18, 2022 21:17

turadg mentioned this pull request Jul 18, 2022

5386 off chain reads #5780

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

5346 replace uses of makeNotifierKit with makeNotifierFromSubscriber #5704

5346 replace uses of makeNotifierKit with makeNotifierFromSubscriber #5704

turadg commented Jun 30, 2022 •

edited

Loading

gibson042 Jul 8, 2022

turadg Jul 8, 2022

gibson042 Jul 12, 2022

turadg Jul 14, 2022

dckc Jul 14, 2022

turadg Jul 14, 2022

dckc Jul 14, 2022

turadg Jul 18, 2022

gibson042 Jul 8, 2022

dckc commented Jul 12, 2022

dckc commented Jul 12, 2022

turadg commented Jul 12, 2022

dckc left a comment

dckc Jul 18, 2022

dckc Jul 18, 2022

dckc Jul 18, 2022

dckc Jul 18, 2022

dckc Jul 18, 2022

dckc Jul 18, 2022

turadg Jul 18, 2022

dckc Jul 18, 2022

turadg Jul 18, 2022

dckc Jul 18, 2022

dckc Jul 18, 2022

turadg Jul 18, 2022

dckc Jul 18, 2022

turadg Jul 18, 2022

		getNotifier: ({ state }) => state.assetNotifier,
		getNotifier: ({ state }) => makeNotifierFromSubscriber(state.assetSubscriber),

		getNotifier: ({ state }) => state.assetNotifier,
		getNotifier: ({ state }) => state.assetSubscriber,

		* @property {ERef<Notifier<Timestamp>>} notifier
		* @property {ERef<Notifier<unknown>>} notifier

		chainStorageP.resolve(undefined);
		chainStorageP.resolve(null);

5346 replace uses of makeNotifierKit with makeNotifierFromSubscriber #5704

5346 replace uses of makeNotifierKit with makeNotifierFromSubscriber #5704

Conversation

turadg commented Jun 30, 2022 • edited Loading

Description

Security 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Jul 12, 2022

dckc commented Jul 12, 2022

turadg commented Jul 12, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Jun 30, 2022 •

edited

Loading