static types improvements #6256

turadg · 2022-09-17T15:28:19Z

Description

Various commits to increase type coverage. One yielded this bug detection #6262

One new types feature is that the Installation type carries through to offer args validation and offer results, so we no longer need to cast (or guess) those.

That's the first several commits. This also goes further and tries to make progress on ( #4560 ) by preventing node_modules diving for a few packages. This required making common ERTP types available as explicit exports instead of ambient. I expect that's the only controversial change: 428b274. If there is an impasse, I'm open to splitting that work to a separate PR.

Security Considerations

n/a, static types

Documentation Considerations

--

Testing Considerations

Types check

misc type fix

mhofman

I am not ok with further leaking the internal file structure of our packages through explicit type exports. Let's make those available through the root of the package, which requires the use of @typedefs

mhofman · 2022-09-20T21:42:30Z

packages/inter-protocol/src/psm/types.js

@@ -1,2 +1 @@
-// @ts-check


backtrack previous commit?

mhofman · 2022-09-20T21:47:42Z

packages/solo/src/chain-cosmos-sdk.js

@@ -728,7 +729,7 @@ ${chainID} chain does not yet know of address ${clientAddr}${adviseEgress(
   *
   * It then delivers the mailbox to inbound.  There are no optimisations.
   *
-   * @param {number=} lastMailboxUpdate
+   * @param {UpdateCount=} lastMailboxUpdate


Suggested change

* @param {UpdateCount=} lastMailboxUpdate

* @param {UpdateCount} [lastMailboxUpdate]

done in #6287

mhofman · 2022-09-20T21:50:10Z

packages/solo/src/chain-cosmos-sdk.js

@@ -81,7 +81,7 @@ const makeTempFile = async (prefix, contents) => {
        if (err) {
          return reject(err);
        }
-        return resolve();
+        return resolve(undefined);


Confused by the need to undefined here. No-one is expecting a specific result type, so void should be valid, no?

Without it,

Expected 1 argument, but got 0. 'new Promise()' needs a JSDoc hint to produce a 'resolve' that can be called without arguments.

Ugh right it defaults to any, which somehow isn't compatible with void in the return value case. This is really a time where the lack of an easy way to specify template types in JSDoc is annoying. Unfortunately instantiation expressions wouldn't really help either, and short of a "cast" on the promise itself, there isn't a good way.

In this case I guess an explicit undefined is ok.

mhofman · 2022-09-20T21:59:12Z

packages/ERTP/src/types-module.d.ts

+ * @file Generated from types.js, then find-replace to export all
+ *
+ * This file exists because types.js is ambient and some modules need the types
+ * to be explictly imported. Changing types.js to a module breaks too much.
+ */


In #4560 (comment), I was imagining a different approach:

Add an export {} at the end of types.js to make it a module explicitly exporting types.

Add an ambient.js which does /* @typedef {import('./types.js').Foo} Foo */

The reason for 1. is to be able to do an export * from './src/types.js in the index.js entrypoint of a package, so that types can be consumed by other packages without having to go for deep imports (aka import('@agoric/ertp').Amount)
The reason for 2. is to not break existing ambient type usages.

That way we can also over time prune the number of types in ambient.js and don't need to keep 2 type definitions in sync.

Also while I am ok with TS syntax, I believe we still haven't settled the question of using it. Regardless the point above makes it a requirement that any public types declared in a .ts file have to be re-exported by a JSDoc @typedef to end up as an exported type at the root of the package.

mhofman · 2022-09-20T22:05:44Z

packages/ui-components/src/components/NatPurseSelector.jsx

@@ -9,9 +9,9 @@ import { stringifyValue } from '../display/index.js';
 * components represent initial state as null/undefined.
 *
 * @param {Array<PursesJSONState> | null} purses Unfiltered purses. This may be null to simplify use in UIs.
- * @param {Brand} [optBrand] - optional brand to filter for
+ * @param {import('@agoric/ertp/src/types-module').Brand} [optBrand] - optional brand to filter for


This is exactly what I want to avoid. We should have

Suggested change

* @param {import('@agoric/ertp/src/types-module').Brand} [optBrand] - optional brand to filter for

* @param {import('@agoric/ertp').Brand} [optBrand] - optional brand to filter for

I agree that's the goal. I just didn't want to have to remake all the ambients. But I accept that as a requirement for explicit package exports.

Chris-Hibbert · 2022-09-20T21:16:20Z

packages/ERTP/src/amountMath.js

   * @param {Brand<K>} brand
-   * @param {K} [assetKind]
+   * @param {K} [assetKind='nat']


Does it make sense for both line 247 and 249 to specify defaults?

I think so. One is JSDoc telling the consumer what the default is and the other the runtime making it so.

The runtime making it happen is on line 254. As I read it (I'm admittedly not the expert here) line 247 says that the AssetKind type doesn't require that its parameter be specified, and if it's omitted from the types, it'll assume 'nat'. Line 249 says that the call to makeEmpty doesn't require a second arg, with the same default. Don't those have the same impact?

If a call appears without the second arg, both typescript and javascript should use 'nat'. If the arg is provided, both should draw the same conclusion. If there's an explicit declaration that conflicts with the code, TS should complain. Won't all the same things happen if line 247 has the default and 249 doesn't?

Sorry, I misread the discussion as being about line 254 (the runtime default).

You're right, 249 shouldn't have a default. I'll fix in this in #6287

Chris-Hibbert · 2022-09-20T21:19:37Z

packages/zoe/src/zoeService/types.js

+ * @template {object} [OA=any] offer args
+ * @template {object} [OR=any] offer result


Can these be Args and Result rather than OA and OR?

Yes of course. Do you think we should adopt that style? So far we've been using all caps to denote a generic type. Usually one letter. I'm more inclined to A for args and R for results.

If we want to change to words, I'd prefer that be done separately in a repo-wide change.

OA and OR seem strictly worse than either alternative. When a single letter is used, I much prefer to also have some part of the commentary indicate what it's mnemonic for, as is the case here, but I think I saw cases where that was dropped. Do some of the formats for specifying typescript not allow room for comments on individual parameters or template values? If that's the case, I'd prefer clearer names.

It's often the case that the role of the templated parameters is opaque from just the type declarations. If I have to specify them for offers, invitations, or start functions, it's really helpful to get an indication of what the missing declaration is for.

I think the reason generics are usually initialisms is to distinguish from types, which are StudlyCaps.

I take the point that it's helpful to have the parameter described, and when using the TS syntax there's no documentation string. So I agree when there are multiple type slots and there isn't a documentation string that we should use the more descriptive name, even it it might be confused with a typedef.

Done in #6287

packages/zoe/test/unitTests/zcf/test-zcf.js

Chris-Hibbert · 2022-09-20T21:26:11Z

packages/governance/src/types.js

@@ -279,7 +279,7 @@

 /**
 * @typedef { ElectorateCreatorFacet & {
- *   getVoterInvitations: () => Promise<Invitation>[]
+ *   getVoterInvitations: () => Promise<Invitation<{}, {voter: import('../test/swingsetTests/contractGovernor/vat-voter.js').EVatVoter}>>[]


types shouldn't depend on tests, right?

Agreed. I just couldn't find this defined in src. Maybe I found this but couldn't reach it by typedef,

agoric-sdk/packages/governance/src/committee.js

Lines 78 to 83 in 2057268

voter: makeHeapFarInstance(`voter${index}`, VoterI, {

castBallotFor(questionHandle, positions) {

const { voteCap } = allQuestions.get(questionHandle);

return E(voteCap).submitVote(voterHandle, positions, 1n);

},

}),

I'll leave this open until the import doesn't go from src to test

Done in #6287

packages/zoe/src/types.js

Chris-Hibbert · 2022-09-20T21:54:31Z

packages/zoe/src/zoeService/types.js

@@ -49,7 +49,7 @@
 * code breaks.
 * @property {GetConfiguration} getConfiguration
 * @property {GetBundleIDFromInstallation} getBundleIDFromInstallation
- * @property {GetProposalShapeForInvitation} getProposalShapeForInvitation
+ * @property {(invitationHandle: InvitationHandle) => Pattern | undefined} getProposalShapeForInvitation


GetProposalShapeForInvitation is still defined in zoeService/internal-types.js, and used in a few places. If it doesn't describe this method, what is it for?

This was to get around an import depth problem. GetProposalShapeForInvitation is defined in internal-types and here we're describing ZoeService which is external. So the type couldn't be resolved. IMO we shouldn't complicate things with callbacks defs like that but I made the minimum necessary change

since ZoeService is external, the type should be externally visible.

we shouldn't complicate things with callbacks defs like that

say more, please? Is the argument that it's better/clearer to type out all the details even when a set of parameters and return values are common across multiple calls? Is this intended as a broad prescription?

My argument, which would apply broadly, is that the shared parameter list and return value is better to repeat so that we don't unnecessarily couple the different definitions. TypeScript types structurally, so when two things share the same structure they are as equal as when they share a typedef.

There is value in the developer mental model to things being the same, but in that case we should include the function name as well and say "this method on this object is the same as it is over there. That is best accomplished by a union with a picked subset of the other object.

e.g.,

type SomeService = { help: () => string; doSomething: (str: string) => void; } type SomeServiceInternal = Pick<SomeService, 'doSomething'> & { doAdminThing: () => AdminFacet; }

Chris-Hibbert · 2022-09-20T21:57:41Z

packages/solo/src/chain-cosmos-sdk.js

@@ -759,6 +760,7 @@ ${chainID} chain does not yet know of address ${clientAddr}${adviseEgress(
    // Reset the backoff period.
    retryBackoff = randomizeDelay(INITIAL_SEND_RETRY_DELAY_MS);
  };
+  /** @param {Error} [e]  */


academic question: Why is this helpful here? We don't often declare the types of errors, and there doesn't appear to be anything special going on here.

The special thing going on is = (e = undefined) => { which gives a type hint to TS so that e is not any, it's undefined. Then passing errors to failedSend doesn't satisfy the constraint. This annotation indicates it can be Error or undefined

Chris-Hibbert · 2022-09-20T21:59:08Z

packages/zoe/test/unitTests/contracts/test-priceAggregatorChainlink.js

@@ -73,6 +77,7 @@ test.before(
      /** @type {OracleHandler} */
      const oracleHandler = Far('OracleHandler', {
        async onQuery({ increment }, _fee) {
+          // @ts-expect-error xxx


You usually say more. Oversight?

moving fast. I think I can remove it

Chris-Hibbert · 2022-09-20T22:00:40Z

packages/SwingSet/tools/manual-timer.js

@@ -53,7 +53,7 @@ const setup = () => {
 * kernel. You can make time pass by calling `advanceTo(when)`.
 *
 * @param {ManualTimerOptions} [options]
- * @returns {ManualTimer}
+ * @returns {TimerService & { advanceTo: (when: Timestamp) => void; }}


is ManualTimer too hard to import here, or not correct, or what?

It's not correct, due to the incomplete Timer migration. ManualTimer is now supposed to have tick and tickN instead of advanceTo.

Chris-Hibbert · 2022-09-20T22:07:27Z

packages/ERTP/src/types-module.d.ts

+ * @file Generated from types.js, then find-replace to export all
+ *
+ * This file exists because types.js is ambient and some modules need the types
+ * to be explictly imported. Changing types.js to a module breaks too much.


Do you have a tool that automatically converted, or was in done manually?

What's the plan for maintenance? If we will have to manually keep them in sync, that should be mentioned here and there, even if you plan to keep up with it yourself.

It was yarn lint:types with some find replace. This file should say exactly how to reproduce it 👍

turadg mentioned this pull request Sep 19, 2022

device timer bug #6262

Draft

turadg force-pushed the ta/more-ts-check branch 2 times, most recently from 618e7a4 to f800d07 Compare September 19, 2022 19:19

turadg added 6 commits September 20, 2022 09:17

chore: ts-check types.js

4169857

chore: fix types

52aa261

misc type fix

chore: parameterize offer()

cf27bc4

chore: fix offer/invitation types

c46e789

chore: Invitation type result first

ab55fb8

chore(types): never allow offer args to arg-less invitations

89ee478

turadg force-pushed the ta/more-ts-check branch from f800d07 to a29d3a3 Compare September 20, 2022 16:31

turadg added 7 commits September 20, 2022 11:12

chore: ts-check most files that have typedefs

cafb4e9

refactor(types): improvements

5349bf5

refactor(solo): runtime validation

34c702b

feat(ertp): types module

428b274

chore(types): maxNodeModuleJsDepth=0 and disable ambient

7e677cd

chore(types): maxNodeModuleJsDepth=0 and disable ambient

503219b

chore(agoric-cli): maxNodeModuleJsDepth=0 and disable ambient types

2057268

turadg force-pushed the ta/more-ts-check branch from a29d3a3 to 2057268 Compare September 20, 2022 19:46

turadg requested review from Chris-Hibbert and mhofman September 20, 2022 20:24

turadg marked this pull request as ready for review September 20, 2022 20:24

turadg requested a review from dtribble as a code owner September 20, 2022 20:24

mhofman requested changes Sep 20, 2022

View reviewed changes

Chris-Hibbert requested changes Sep 20, 2022

View reviewed changes

turadg mentioned this pull request Sep 20, 2022

static type improvements to contracts #6287

Merged

turadg marked this pull request as draft September 21, 2022 14:55

turadg closed this Nov 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

static types improvements #6256

static types improvements #6256

turadg commented Sep 17, 2022 •

edited

Loading

mhofman left a comment

mhofman Sep 20, 2022

mhofman Sep 20, 2022

turadg Sep 21, 2022

mhofman Sep 20, 2022

turadg Sep 20, 2022

mhofman Sep 20, 2022

mhofman Sep 20, 2022

mhofman Sep 20, 2022

turadg Sep 20, 2022

Chris-Hibbert Sep 20, 2022

turadg Sep 20, 2022

Chris-Hibbert Sep 21, 2022

turadg Sep 21, 2022

Chris-Hibbert Sep 20, 2022

turadg Sep 20, 2022

Chris-Hibbert Sep 20, 2022

turadg Sep 20, 2022

turadg Sep 21, 2022

Chris-Hibbert Sep 20, 2022

turadg Sep 20, 2022

turadg Sep 21, 2022

Chris-Hibbert Sep 20, 2022

turadg Sep 20, 2022

Chris-Hibbert Sep 21, 2022

turadg Sep 21, 2022

Chris-Hibbert Sep 20, 2022

turadg Sep 20, 2022

Chris-Hibbert Sep 20, 2022

turadg Sep 21, 2022

Chris-Hibbert Sep 20, 2022

turadg Sep 20, 2022

Chris-Hibbert Sep 20, 2022

turadg Sep 20, 2022

	* @param {UpdateCount=} lastMailboxUpdate
	* @param {UpdateCount} [lastMailboxUpdate]

	* @param {import('@agoric/ertp/src/types-module').Brand} [optBrand] - optional brand to filter for
	* @param {import('@agoric/ertp').Brand} [optBrand] - optional brand to filter for

		* @template {object} [OA=any] offer args
		* @template {object} [OR=any] offer result

	voter: makeHeapFarInstance(`voter${index}`, VoterI, {
	castBallotFor(questionHandle, positions) {
	const { voteCap } = allQuestions.get(questionHandle);
	return E(voteCap).submitVote(voterHandle, positions, 1n);
	},
	}),

static types improvements #6256

static types improvements #6256

Conversation

turadg commented Sep 17, 2022 • edited Loading

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

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

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 Sep 17, 2022 •

edited

Loading