export types #6500

turadg · 2022-10-26T22:20:44Z

refs: #6343

Description

Emit types for agoric-sdk package builds like endojs/endo#1307

Security Considerations

n/a

Documentation Considerations

Export types are a documentation resource.

Testing Considerations

CI. And dry release?

mhofman

Nice start. Not an approval until I can get some clarifications on the various ts-expect-error.

I'd like to at least double check the need for some of these ts-expect-errors. As I've expressed before, I believe they are worse than casts as they can hide multiple errors unexpectedly. If possible narrow application of casts are better (while slightly harder to read unfortunately).

Also I see there was an attempt to switch to exported types, but it was reverted. If attempted again, let's avoid deep imports cross package.

I have some type fixes for swingset that I may now try to brush off :)

packages/internal/src/utils.js

mhofman · 2022-10-26T22:33:55Z

packages/smart-wallet/src/walletFactory.js

+// eslint-disable-next-line import/no-extraneous-dependencies
+import '@agoric/swingset-vat/src/types-ambient.js';


I don't suppose we can avoid relying on ambient types for swingset-vat. I know it does re-export most types explicitly at the top level.

mhofman · 2022-10-26T22:34:47Z

packages/SwingSet/src/kernel/state/kernelKeeper.js

@@ -158,7 +158,7 @@ const FIRST_METER_ID = 1n;

 /**
 * @param {HostStore} hostStorage
- * @param {KernelSlog} kernelSlog
+ * @param {KernelSlog | null} kernelSlog


Oh I have a stash with this fix and other swingset ones somewhere

mhofman · 2022-10-26T22:37:42Z

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

-/** @type {<T>(store: ERef<MapStore>, key: string, make: () => T) => Promise<T>} */
+/** @type {<T>(store: ERef<Map<string, T>>, key: string, make: () => T) => Promise<T>} */


I'm surprised we have a Map based store around

provideWhen seem to be used once with a ERef<MapStore> and once with a Map. The semantics of these Maps are different enough, which I believe does reflect on their signature. If provideWhen is in fact using the common subset of these 2, can we be explicit about this somehow? Since TS is structural typing only, maybe by defining an interface of the subset used? That would avoid a cast / expect-error in the place where provideWhen is used with the ERef<MapStore>.

Good idea. Done.

packages/vats/src/repl.js

packages/vats/src/vat-provisioning.js

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

packages/vats/src/types.js

mhofman · 2022-10-28T23:25:23Z

Nit on reviewing process: While I understand this is was still a draft PR, it's extremely difficult to track changes compared to the previously reviewed state if force pushes include both amendments and base changes. In general please do not amend, and stick to commits additions once reviews have started, and until the PR is accepted. New commits can be marked as fixup to indicate such amendments are to be made just before merging only. Furthermore when changing the base, please do 2 force push: one push adding commits on the old base as is, and one force push rebasing showing no changes (or only conflict resolution if warranted).

Another point specific to this PR: please include the main package name affected in the conventional commit prefix, e.g. chore(ERTP). Right now there are commits with duplicate names making it impossible to understand what's going on.

I really want type improvements, but right now this PR is extremely difficult to review.

turadg · 2022-10-28T23:48:42Z

I agree with the amendment policy for PRs marked R4R. I don't for PRs that aren't. This is something our eng org should agree on so let's discuss in standup.

Another point specific to this PR: please include the main package name affected in the conventional commit prefix, e.g. chore(ERTP).

I've opted against that because IIUC it creates redundant info in READMEs. e.g. the @agoric/ertp changelog says "chore(ertp): foo".

Right now there are commits with duplicate names making it impossible to understand what's going on.

I'm confused on why omission of package name makes it impossible to understand what's going on. If I had not separated the commits and instead, for example, squashed all the chore: misc types fixes then there would be no duplicates but I expect it would be harder to follow the changes.

I really want type improvements, but right now this PR is extremely difficult to review.

All the changes since your review of the draft were addressing your comments. So maybe it suffices to look over your comments and see whether they were addressed to your satisfaction.

mhofman

I'd like to do another pass, but here is my review so far.

I still have some concerns regarding casts and type compatibility.

I believe the conversion of the vats package to exported type introduces some mistaken re-exports, and could use a better layering for the main entrypoint.

packages/smart-wallet/package.json

mhofman · 2022-10-29T14:13:02Z

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

-/** @type {<T>(store: ERef<MapStore>, key: string, make: () => T) => Promise<T>} */
+/** @type {<T>(store: ERef<Map<string, T>>, key: string, make: () => T) => Promise<T>} */


provideWhen seem to be used once with a ERef<MapStore> and once with a Map. The semantics of these Maps are different enough, which I believe does reflect on their signature. If provideWhen is in fact using the common subset of these 2, can we be explicit about this somehow? Since TS is structural typing only, maybe by defining an interface of the subset used? That would avoid a cast / expect-error in the place where provideWhen is used with the ERef<MapStore>.

mhofman · 2022-10-29T14:19:49Z

packages/vats/src/core/boot.js

+export * from '../nameHub.js';
+export * from '../bridge.js';


These see, to be re-exporting non-type exports as well, is that intended?

If so please visually separate them. In general I prefer having runtime imports before type based ones.

I suspect the ./bridge.js import is for the BridgeManager type? In that case I believe the better approach would be to have a JSDoc @typedef in ../types.js and leverage the existing re-export from there. Same the nameHub.js's NameHub and NameAdmin.

packages/internal/src/utils.js

packages/SwingSet/tools/internal-types.js

packages/zoe/src/internal-types.js

mhofman · 2022-10-31T19:32:58Z

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

+    // @ts-expect-error cast
+    E.get(homeP).scratch,


While I mentioned above I'd prefer to figure out the provideWhen's param type to the compatible subtype, is there a reason to use // @ts-expect-error for casts instead of doing an actual cast ?

Do we consider casting to be too visually onerous?

Moot with 570cc99, but I do find casting in JSdoc to be hard to read. No strong feeling one way or the other.

mhofman · 2022-10-31T19:38:03Z

packages/vats/src/vat-provisioning.js

@@ -50,7 +51,7 @@ export function buildRootObject(_vatPowers) {
        // Update the notifier when the chainBundle resolves.
        const { notifier, updater } = makeNotifierKit();
        chainBundle.then(clientHome => {
-          updater(harden({ clientHome, clientAddress: address }));
+          updater.updateState(harden({ clientHome, clientAddress: address }));


Fix LGTM, but I should probably defer to someone more familiar with this code and notifiers. I believe this is the only behavioral difference in this PR.

Yep, the only intended behavior change. updater comes out of makeNotifierKit which just has updateState, fail and finish. The comment says "Update the notifier" so I'm sure it's not meant to be one of the latter two.

FYI @michaelfig who I think wrote this.

packages/vats/src/types.js

packages/vats/src/core/boot.js

mhofman · 2022-11-01T01:52:39Z

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

@@ -68,7 +68,7 @@ export const reserveThenDeposit = async (
  console.info('confirmed deposit for', debugName);
 };

-/** @type {<T>(store: ERef<Map<string, T>>, key: string, make: () => T) => Promise<T>} */
+/** @type {<T>(store: ERef<Pick<MapStore<string, T>, 'get' | 'set'>>, key: string, make: () => T) => Promise<T>} */
 const provideWhen = async (store, key, make) => {
  const found = await E(store).get(key);


I probably should have looked at the actual implementation before.

I was under the impression that MapStore threw when doing a get of an uninitialized value. That's what I meant earlier by different semantics, unfortunately the signature of get methods only varies on the return type between Map and MapStore, making them "compatible". Thinking more about this, it's probably a footgun that these objects have such a similar shape, yet such different semantics.

Anyway, I think provideWhen is faulty because of this, and I don't understand how the fallback path would have every worked with the MapStore variant.

Agree with your assessment. Is there a requested change in there? An issue to make or a thread to start?

I don't know. I don't think it'd be easy enough to fix now, but at least we should create an issue?

I think reverting the Pick and adding a ts-expect-error pointing to the issue?

I made an issue but then realized a simpler fix: bf9b05f

I fail to see how it's a fix?

mhofman · 2022-11-01T01:53:23Z

packages/SwingSet/src/types-external.js

@@ -351,7 +351,7 @@ export {};
 *
 * @typedef { string } BundleID
 * @typedef {*} BundleCap
- * @typedef { { moduleFormat: 'endoZipBase64', endoZipBase64: string, endoZipBase64Sha512 } } EndoZipBase64Bundle
+ * @typedef { { moduleFormat: 'endoZipBase64', endoZipBase64: string, endoZipBase64Sha512: string } } EndoZipBase64Bundle


Oh yeah that would do it!

mhofman · 2022-11-01T01:54:56Z

packages/vats/index.js

+export * from './src/nameHub.js';
+export * from './src/bridge.js';


I still believe this will cause formerly internal runtime helpers to now be visible on the root of this package, not only types.

See #6500 (comment)

Thanks for the thorough review!

I suspect the ./bridge.js import is for the BridgeManager type? In that case I believe the better approach would be to have a JSDoc @typedef in ../types.js and leverage the existing re-export from there. Same the nameHub.js's NameHub and NameAdmin.

makeNameHubAdmin is imported in three other modules, so arguably this top level is an improvement, but I'll err towards conservative and leave runtime exports refactoring for another day.

mhofman · 2022-11-01T01:59:01Z

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

@@ -108,6 +110,7 @@ export const makeInstallCache = async (
  };

  const wrapInstall = install => async (mPath, bPath, opts) => {
+    // @ts-expect-error xxx bundle types


I believe runtime changes are worth when they directly address issues found by typing. improvements

mhofman

I'm not sure I understand how the provideWhen type change is actually a fix.

mhofman · 2022-11-01T19:03:22Z

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

@@ -81,7 +81,7 @@ const provideWhen = async (store, key, make) => {

 /**
 *
- * @param {{ scratch: ERef<MapStore<string, CopyTagged>> }} homeP
+ * @param {{ scratch: ERef<Map<string, CopyTagged>> }} homeP


Does scratch actually have Map semantics? If not that'd be a very unobvious cast that hide the runtime differences.

Yes, it does

agoric-sdk/packages/solo/src/scratch.js

Lines 17 to 20 in bf9b05f

get: async keyP => {

const key = await keyP;

return map.get(key);

},

mhofman · 2022-11-01T19:05:12Z

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

@@ -68,7 +68,7 @@ export const reserveThenDeposit = async (
  console.info('confirmed deposit for', debugName);
 };

-/** @type {<T>(store: ERef<Map<string, T>>, key: string, make: () => T) => Promise<T>} */
+/** @type {<T>(store: ERef<Pick<MapStore<string, T>, 'get' | 'set'>>, key: string, make: () => T) => Promise<T>} */
 const provideWhen = async (store, key, make) => {
  const found = await E(store).get(key);


I fail to see how it's a fix?

mhofman · 2022-11-01T19:09:54Z

packages/SwingSet/src/vats/plugin-manager.js

@@ -21,9 +21,11 @@ const DEFAULT_WALKER = Far('walker', { walk: pluginRootP => pluginRootP });

 /**
 * @template T
- * @typedef {T} Device
+ * @typedef {'Device' & { __deviceType__: T }} Device


mhofman · 2022-11-01T19:12:00Z

packages/agoric-cli/src/init.js

 import '@agoric/swingset-vat/exported.js';
+import '@agoric/swingset-vat/src/vats/network/types.js';


Seeing this now, would it make sense to add import './src/vats/network/types.js'; to @agoric/swingset-vat/exported.js instead ?

Edit: maybe not since it's used directly in other places?

yeah, I considered that and opted for lesser change to what's exported. I figure it will be addressed in further ambients work

in the absence of a type signature, TypeScript infers an expected argument

mhofman

LGTM

dckc · 2022-11-02T17:12:02Z

I would have like to see more coordination around getting rid of @ts-check. It's a useful explicit tag, especially when moving code between repositories.

turadg · 2022-11-02T17:36:54Z

I would have like to see more coordination around getting rid of @ts-check. It's a useful explicit tag, especially when moving code between repositories.

@dckc it's an easy change to reverse. You want every JS file in a package to have @ts-check even if it's redundant with the package configuration?

dckc · 2022-11-02T17:53:54Z

You want every JS file in a package to have @ts-check even if it's redundant with the package configuration?

That would have been my preference, yes. At this point, I don't feel strongly that it's worth more churn in the git history. But it seems like a change to house style that's worth fairly widespread coordination.

turadg requested review from kriskowal and mhofman October 26, 2022 22:20

mhofman reviewed Oct 26, 2022

View reviewed changes

turadg force-pushed the 1254-export-types branch 2 times, most recently from fd324c8 to 8980a2d Compare October 28, 2022 23:19

turadg marked this pull request as ready for review October 28, 2022 23:19

mhofman self-requested a review October 28, 2022 23:34

turadg force-pushed the 1254-export-types branch from 8f791aa to 8980a2d Compare October 31, 2022 14:41

mhofman reviewed Oct 31, 2022

View reviewed changes

mhofman reviewed Nov 1, 2022

View reviewed changes

mhofman self-requested a review November 1, 2022 14:13

turadg mentioned this pull request Nov 1, 2022

delete #6517

Open

mhofman reviewed Nov 1, 2022

View reviewed changes

turadg added 15 commits November 1, 2022 12:25

fix(vat-provisioning): bad updater call

2f7d5f4

chore: enable noImplicitAny

6775bdf

build: emit types

bc0aa42

chore: enable checkJs

d1119f4

chore: misc types fixes

c988316

build: emit types

434e7ae

build: emit types

e5f6c2d

chore: misc types fixes

3d3620d

chore: emit types

ec7efa2

chore: vats/src/types as module

a39596e

chore: misc types fixes

9b7da70

chore: emit types

ab322c2

chore: dedupe ManualTimer type

4e2b987

build: emit types

10a6b2c

chore: misc types fixes

ffb5207

turadg added 10 commits November 1, 2022 12:29

chore: omit unused _vatPowers param

39ffa14

in the absence of a type signature, TypeScript infers an expected argument

chore: export bridge types

21ac961

chore: remove swingset-vat types-ambient imports

472b820

refactor: wrapInstall bundle validation

3181a4f

refactor: main index

ccc22fd

chore: export CommitteeElectorateCreatorFacet

12597aa

chore: improve Device types

7a1dcd5

chore: inline governance internalTypes

be1ede5

chore: TS ambient for network types

12c7971

chore: export FeeMintAccess type

078263a

turadg force-pushed the 1254-export-types branch from bf9b05f to 078263a Compare November 1, 2022 19:29

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

mhofman approved these changes Nov 1, 2022

View reviewed changes

turadg mentioned this pull request Nov 1, 2022

1254 runtime changes #6520

Merged

defer some runtime changes

135bf05

mergify bot merged commit aad309a into master Nov 1, 2022

mergify bot deleted the 1254-export-types branch November 1, 2022 22:21

turadg mentioned this pull request Nov 2, 2022

fix dev publish #6524

Merged

turadg mentioned this pull request Nov 3, 2022

use TypeScript Agoric/dapp-psm#40

Merged

		// eslint-disable-next-line import/no-extraneous-dependencies
		import '@agoric/swingset-vat/src/types-ambient.js';

		/** @type {<T>(store: ERef<MapStore>, key: string, make: () => T) => Promise<T>} */
		/** @type {<T>(store: ERef<Map<string, T>>, key: string, make: () => T) => Promise<T>} */

		export * from './src/nameHub.js';
		export * from './src/bridge.js';

	get: async keyP => {
	const key = await keyP;
	return map.get(key);
	},

		import '@agoric/swingset-vat/exported.js';
		import '@agoric/swingset-vat/src/vats/network/types.js';

export types #6500

export types #6500

Conversation

turadg commented Oct 26, 2022

Description

Security Considerations

Documentation Considerations

Testing Considerations

mhofman 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

mhofman commented Oct 28, 2022 • edited Loading

turadg commented Oct 28, 2022 • edited Loading

mhofman 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

turadg Nov 1, 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

mhofman 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

mhofman left a comment

Choose a reason for hiding this comment

dckc commented Nov 2, 2022

turadg commented Nov 2, 2022

dckc commented Nov 2, 2022

mhofman commented Oct 28, 2022 •

edited

Loading

turadg commented Oct 28, 2022 •

edited

Loading

turadg Nov 1, 2022 •

edited

Loading