7300 remove pipeTopicToStorage #7389

turadg · 2023-04-12T02:04:40Z

closes: #7300

Description

Fully removes pipeTopicToStorage. To do so required updating these contracts:

auctioneer
fluxAggregator
smartWallet

~~chain-main.js also imported it and I didn't want to touch that too much so moved the definition within the module.~~ (removed separately)

That freed up removing it from @agoric/notifer.

This also moves the remaining exports of stored-topics.js to Zoe contract support, since they were to support "PublicTopic" and that's a contract level convention.

There are also testing and types improvements along the way.

Security Considerations

--

Scaling Considerations

Eliminiates use of non-scalable pipeTopicToStorage in contracts.

Documentation Considerations

Breaking changes, indicated with ! commits.

Testing Considerations

CI

packages/cosmic-swingset/src/chain-main.js

turadg · 2023-04-12T02:06:12Z

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

@@ -425,6 +424,7 @@ export const prepareVaultManagerKit = (
            // about the installation and terms of the liqudation contract. We could
            // have another notifier for state downstream of governance changes, but
            // that doesn't seem to be cost-effective.
+            // @ts-expect-error FIXME never defined


@Chris-Hibbert this seems like a genuine bug, but I can't tell the impact

I think replaceable liquidator contracts wasn't preserved in the move to auctions. If I'm right, the impact is just some vestigial code and a little bit of unused storage. Orthogonal to this PR.

Dan's right. The comment above and variable should go away. There doesn't appear to be any code that touches it.

ok removing

turadg · 2023-04-12T02:08:02Z

packages/zoe/src/contractSupport/recorder.js

@@ -189,7 +201,9 @@ export const prepareRecorderKit = (baggage, marshaller) => {
 };

 /**
- * To be called at most once per baggage.
+ * Convenience wrapper for DurablePublishKit and Recorder kinds.


@gibson042 I thought I had taken this out per our thread but maybe it reappeared in rebase.

In any case, it has proven handy for tests and I think it's okay in contracts with the caveat. WDYT?

turadg · 2023-04-12T02:10:19Z

packages/inter-protocol/src/auction/auctionBook.js

 */

 /**
 * @param {Baggage} baggage
 * @param {ZCF} zcf
+ * @param {import('@agoric/zoe/src/contractSupport/recorder.js').MakeRecorderKit} makeRecorderKit


I'm not happy about these deep imports but I think the real solution is to have a new package for contract support. such that this would be,

Suggested change

* @param {import('@agoric/zoe/src/contractSupport/recorder.js').MakeRecorderKit} makeRecorderKit

* @param {import('@agoric/contracts').MakeRecorderKit} makeRecorderKit

queued up: #7392

dckc

looks good. I'm not entirely confident I could maintain this code, so this might be more of an "I don't see any critical problems" review and you might want other eyes.

The notion of a breaking refactor hurts my head... but whatever... the breaking change is signaled.

dckc · 2023-04-12T02:10:20Z

packages/smart-wallet/src/smartWallet.js

- *   updatePublishKit: PublishKit<UpdateRecord>,
- *   currentPublishKit: PublishKit<CurrentWalletRecord>,
+ *   updateRecorderKit: import('@agoric/zoe/src/contractSupport/recorder.js').RecorderKit<UpdateRecord>,
+ *   currentRecorderKit: import('@agoric/zoe/src/contractSupport/recorder.js').RecorderKit<CurrentWalletRecord>,


recorder.js is a zoe concern? hm.

(I see later your comment about an @agoric/contracts package. makes sense)

dckc · 2023-04-12T02:12:01Z

packages/smart-wallet/src/smartWallet.js

          return harden({
            current: {
              description: 'Current state of wallet',
-              subscriber: currentPublishKit.subscriber,
-              storagePath: memoizedPath(currentStorageNode),


👏 getting rid of memoizedPat seems like a nice simplification

dckc · 2023-04-12T02:25:56Z

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

+        // XXX how to handle errors here?
+        E(latestRoundPublisher).write({


void seems reasonable.

That is: rely on whatever unhandled rejection policy is used throughout the platform. Current state of the art seems to make them reasonably visible to node operators: #4939 / #4471. See also logging issues.

As to RPC clients: if writing latest round info fails, is writing an error message likely to succeed?

And if it's a transient problem, we can expect it to self-heal, since rounds happen on a regular cadence.

dckc · 2023-04-12T02:29:09Z

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

@@ -177,6 +177,7 @@ test.serial('invitations', async t => {
   * @returns {Promise<[{description: string, instance: Instance}]>}
   */
  const getInvitationFor = async (desc, len, balances) => {
+    await eventLoopIteration();


I often feel obliged to say why I'm adding await eventLoopIteration(). I wonder about an alias such as await vstorageWritesVisible().

dckc · 2023-04-12T02:33:17Z

packages/zoe/src/contractSupport/durability.js

- *
- * @param {import('@agoric/vat-data').Baggage} baggage
- */
-export const makeStorageNodePathProvider = baggage => {


a breaking chore seems a little funky. I wonder if this is really a fix... if makeStorageNodePathProvider broke some rule(s) and hence was buggy. not critical

looking at "closes #7300", I think #7300 makes more sense as a performance / scalability bug. So this is indeed a fix.

dckc · 2023-04-12T02:35:35Z

packages/inter-protocol/src/auction/auctionBook.js

 */

 /**
 * @param {Baggage} baggage
 * @param {ZCF} zcf
+ * @param {import('@agoric/zoe/src/contractSupport/recorder.js').MakeRecorderKit} makeRecorderKit


dckc · 2023-04-12T02:42:07Z

packages/inter-protocol/test/auction/test-auctionBook.js

 import { setup } from '@agoric/zoe/test/unitTests/setupBasicMints.js';
-import { makePublishKit } from '@agoric/notifier';
-import { makeBoard } from '@agoric/vats/src/lib-board.js';


makeBoard is no longer used? Oh... I see it moved up. I wonder what's missing in our toolkit that results in these spurious diffs.

what's missing in our toolkit

Linting for import ordering. When some import is no longer used there is a lint error, and I fix it up with TypeScript's "Organize imports" function. If we can agree on import ordering rule I'd use that instead (by doing Organize imports and then running the linter with --fix.)

packages/cosmic-swingset/src/chain-main.js

dckc · 2023-04-12T02:46:37Z

packages/inter-protocol/test/supports.js

@@ -232,7 +232,7 @@ export const subscriptionKey = subscription => {

 /**
 *
- * @param {ERef<{getPublicTopics: () => import('@agoric/notifier').TopicsRecord}>} hasTopics
+ * @param {ERef<{getPublicTopics: () => import('@agoric/zoe/src/contractSupport').TopicsRecord}>} hasTopics


you're leaving off /index.js on purpose, I trust.

yes b/c TS still automatically resolves /index.js (Node style)

dckc · 2023-04-12T02:50:27Z

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

@@ -425,6 +424,7 @@ export const prepareVaultManagerKit = (
            // about the installation and terms of the liqudation contract. We could
            // have another notifier for state downstream of governance changes, but
            // that doesn't seem to be cost-effective.
+            // @ts-expect-error FIXME never defined


I think replaceable liquidator contracts wasn't preserved in the move to auctions. If I'm right, the impact is just some vestigial code and a little bit of unused storage. Orthogonal to this PR.

Chris-Hibbert · 2023-04-12T16:55:56Z

packages/zoe/src/contractSupport/recorder.js

+/**
+ * For use in tests
+ */
+export const prepareMockRecorderKitMakers = () => {


test-only code doesn't usually belong under .../src. Can this be a helper published from @agoric/zoe/tools?

I'll move it in #7392

Chris-Hibbert · 2023-04-12T17:17:03Z

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

@@ -425,6 +424,7 @@ export const prepareVaultManagerKit = (
            // about the installation and terms of the liqudation contract. We could
            // have another notifier for state downstream of governance changes, but
            // that doesn't seem to be cost-effective.
+            // @ts-expect-error FIXME never defined


Dan's right. The comment above and variable should go away. There doesn't appear to be any code that touches it.

turadg requested review from dckc and Chris-Hibbert April 12, 2023 02:04

turadg changed the title ~~7300 complete~~ 7300 remove pipeTopicToStorage Apr 12, 2023

turadg commented Apr 12, 2023

View reviewed changes

dckc approved these changes Apr 12, 2023

View reviewed changes

Chris-Hibbert approved these changes Apr 12, 2023

View reviewed changes

mhofman mentioned this pull request Apr 12, 2023

refactor(cosmic-swingset): remove notifier from bundle publish #7396

Merged

turadg force-pushed the 7300-complete branch from ad607c0 to e3d835e Compare April 12, 2023 21:13

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

turadg force-pushed the 7300-complete branch from e3d835e to 1235eb3 Compare April 12, 2023 22:14

turadg mentioned this pull request Apr 12, 2023

enforce consistent import ordering #7403

Open

turadg added 10 commits April 12, 2023 17:53

refactor(recorder): DRY prepareRecorderKitMakers

2cdc02f

fix(wallet): pipeTopicToStorage with Recorder kit

31b79b7

fix(price): pipeTopicToStorage with Recorder kit

5269d2f

chore!: rm obsolete makeStorageNodePathProvider

dc0a4a5

chore(auction): fix BookDataNotification types

af06442

fix(auction): pipeTopicToStorage with Recorder kit

2f460a0

chore!: remove deprecated pipeTopicToStorage

6a6108a

refactor!: move PublicTopic to Zoe contractSupport

c51ea3d

chore(types): fix and validate VaultManager types

a56da69

chore(vaults): rm vestigal liquidator config

88740f7

turadg force-pushed the 7300-complete branch from 4c98096 to 88740f7 Compare April 13, 2023 00:53

Merge branch 'master' into 7300-complete

cde25d0

mergify bot merged commit 0e49c36 into master Apr 13, 2023

mergify bot deleted the 7300-complete branch April 13, 2023 01:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7300 remove pipeTopicToStorage #7389

7300 remove pipeTopicToStorage #7389

turadg commented Apr 12, 2023 •

edited

Loading

turadg Apr 12, 2023

dckc Apr 12, 2023

Chris-Hibbert Apr 12, 2023

turadg Apr 12, 2023

turadg Apr 12, 2023

turadg Apr 12, 2023

dckc Apr 12, 2023

turadg Apr 12, 2023

dckc left a comment

dckc Apr 12, 2023

dckc Apr 12, 2023

dckc Apr 12, 2023

dckc Apr 12, 2023

dckc Apr 12, 2023

dckc Apr 12, 2023

dckc Apr 12, 2023

dckc Apr 12, 2023

turadg Apr 12, 2023

turadg Apr 12, 2023

dckc Apr 12, 2023

turadg Apr 12, 2023 •

edited

Loading

dckc Apr 12, 2023

Chris-Hibbert Apr 12, 2023

turadg Apr 12, 2023

Chris-Hibbert Apr 12, 2023

	* @param {import('@agoric/zoe/src/contractSupport/recorder.js').MakeRecorderKit} makeRecorderKit
	* @param {import('@agoric/contracts').MakeRecorderKit} makeRecorderKit

		// XXX how to handle errors here?
		E(latestRoundPublisher).write({

7300 remove pipeTopicToStorage #7389

7300 remove pipeTopicToStorage #7389

Conversation

turadg commented Apr 12, 2023 • edited 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

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

turadg Apr 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

turadg commented Apr 12, 2023 •

edited

Loading

turadg Apr 12, 2023 •

edited

Loading