contract start types #7436

turadg · 2023-04-17T18:59:11Z

Description

No ticket. Nerd-sniped in #7426 (comment)

Tighter definition for startInstance, resulting in more installation types required in bootstrap.

That then brought up some governance types inconsistencies. Those have an issue so I punted with a comment.

There were two runtime changes to vaultFactory types which I separated.

Review by commit recommended.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

--

Testing Considerations

CI

dckc

I was reviewing by commit and then a force-push made me wonder if I should wait.

dckc · 2023-04-17T19:08:55Z

packages/zoe/src/zoeService/utils.d.ts

+  zcf?: ZCF,
+  privateArgs?: {},
+  baggage?: Baggage,
+) => ERef<{ creatorFacet?: {}; publicFacet?: {} }>;


does {} mean any in this context?

here it's specifically an empty object, vs the any several lines down

dckc · 2023-04-17T19:14:43Z

packages/zoe/src/zoeService/utils.d.ts

-  : SF extends (zcf: { getTerms: () => {} }) => unknown
-  ? { terms: any }
-  : {};
+  : never;

 type StartResult<S> = S extends (...args: any) => Promise<infer U>


infer U is magic I have yet to learn. That makes this an "I don't see any critical problems" review and not a "I'm prepared to maintain this code" review.

It's a sort of syntactic sugar for extracting what matches in the condition https://www.typescriptlang.org/docs/handbook/2/conditional-types.html#inferring-within-conditional-types

packages/governance/src/types-ambient.js

packages/inter-protocol/src/proposals/econ-behaviors.js

dckc

I need to understand the change in vats/src/core/types.js better.

dckc · 2023-04-17T19:51:21Z

packages/vats/src/core/types.js

+ *       centralSupply: Promise<Installation<import('@agoric/vats/src/centralSupply.js').start>>,
+ *       committee: Promise<Installation<import('@agoric/governance/src/committee.js').start>>,
+ *       contractGovernor: Promise<Installation<import('@agoric/governance/src/contractGovernor.js').start>>,
+ *       econCommitteeCharter: Promise<Installation<import('@agoric/inter-protocol/src/econCommitteeCharter.js').start>>,
+ *       feeDistributor: Promise<Installation<import('@agoric/inter-protocol/src/feeDistributor.js').start>>,
 *       mintHolder: Promise<Installation<import('@agoric/vats/src/mintHolder.js').prepare>>,
+ *       psm: Promise<Installation<import('@agoric/inter-protocol/src/psm/psm.js').start>>,
+ *       reserve: Promise<Installation<import('@agoric/inter-protocol/src/reserve/assetReserve.js').start>>,
+ *       VaultFactory: Promise<Installation<import('@agoric/inter-protocol/src/vaultFactory/vaultFactory.js').start>>,


This looks like a significant regression. I need to understand the motivation better.

There was some attempt to layer EconomyBootstrapSpace on top rather than putting that stuff here.

agoric-sdk/packages/inter-protocol/src/proposals/econ-behaviors.js

Lines 86 to 87 in 7c17a16

* }>} EconomyBootstrapSpace

*/

I'll put these wherever they go. I just command-clicked installation in the param destructuring to find out where the defs were coming from. I'll try EconomyBootstrapSpace

Now I remember why I put them here: the keys are from WellKnownName which is the key of the installation property Record.

Changing that is out of scope, imo. Acceptable?

oic how this works now: econCommitteeCharter is not just any old installation... got it.

dckc · 2023-04-17T19:52:10Z

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

-          return harden({});
+          return harden([]);


amended 9c4e487 after review to prevent drifts in the future

Chris-Hibbert

I don't see any problems with this, but, echoing @dckc, I can't approve this without understanding the answers to his questions.

Chris-Hibbert · 2023-04-17T21:16:46Z

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

@@ -302,3 +292,43 @@ test('initialPrice', /** @param {ExecutionContext} t */ async t => {
  const q400 = await E(pa).quoteGiven(make(collateral.brand, 400n), debt.brand);
  t.deepEqual(q400.quoteAmount.value[0].amountOut, make(debt.brand, 14200n));
 });
+
+test.skip('types', async t => {


If it's intended that this be checked in with test.skip, it should have a comment saying why.

the comment on 303-305 works for me.

I guess moving it up could have made it easier to see.

I put in into a separate (non-Ava) test file to avoid the confusion. Doing that also let me use .ts syntax for better test coverage.

I'm still confused. I infer from the latest comment that the .ts file was intended to replace this test, but the test is still here.

I don't understand how a skipped test helps to declare types, or what other function it might serve.

Does the .ts file add code to our system, or does it only impact type declarations?

dckc

looks good

Chris-Hibbert · 2023-04-19T16:33:21Z

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

@@ -302,3 +292,43 @@ test('initialPrice', /** @param {ExecutionContext} t */ async t => {
  const q400 = await E(pa).quoteGiven(make(collateral.brand, 400n), debt.brand);
  t.deepEqual(q400.quoteAmount.value[0].amountOut, make(debt.brand, 14200n));
 });
+
+test.skip('types', async t => {


I'm still confused. I infer from the latest comment that the .ts file was intended to replace this test, but the test is still here.

I don't understand how a skipped test helps to declare types, or what other function it might serve.

Does the .ts file add code to our system, or does it only impact type declarations?

Chris-Hibbert · 2023-04-19T16:36:12Z

packages/zoe/test/types.test-d.ts

+ * @file uses .ts syntax to be able to declare types (e.g. of kit.creatorFacet as {})
+ * because "there is no JavaScript syntax for passing a a type argument"
+ * https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html


I'm confused about the contribution of this file. I thought .ts fles were for type declarations, but this looks like imperative code, though not working code. What is its impact? What types are being declared or implied? Does it run as code during tests, or is it parsed and discarded, or something else?

I'm still confused. I infer from the latest comment that the .ts file was intended to replace this test, but the test is still here.

Sorry about that. I must have messed up a rebase. I've amended and push ed with the test.skip('types') removed.

I don't understand how a skipped test helps to declare types, or what other function it might serve.

The intent was to have some types tests co-located with the runtime tests for a particular specimen (e.g. scaledPriceAuthority). But it's better to keep purely types tests out of Ava tests so it doesn't report about skipping. Indeed, the thing being tested (type check) isn't being skipped at all.

Does the .ts file add code to our system, or does it only impact type declarations?

It's only read by the tsc checker run by yarn lint:types.

I thought .ts fles were for type declarations, but this looks like imperative code, though not working code.

.d.ts is for type declarations. .ts is transpiled to .js so it's for any code.. The test-d.ts is meant to convey that while it's a .ts it's for testing type defs. That's a convention of the tsd package.

What is its impact?

It just gets included in the tsc type check. So if something here starts erroring (or stops, because the ts-expect-error is not finding one) then it'll cause lint:types (and thus CI) to fail.

Does it run as code during tests, or is it parsed and discarded, or something else?

Ava won't run it because it doesn't match the pattern Ava looks for. It's parsed only by tsc. When the package gets built the tsc config will transpile it, but to a similarly ignored test file.

Thanks for the explanations. Would it make sense to add any of this as a comment to the file?

It would have to be duplicated for each file. I've put it in the wiki: https://github.com/Agoric/agoric-sdk/wiki/Static-types-testing

Chris-Hibbert · 2023-04-19T16:38:25Z

yarn.lock

+"@jest/schemas@^29.4.3":
+  version "29.4.3"
+  resolved "https://registry.yarnpkg.com/@jest/schemas/-/schemas-29.4.3.tgz#39cf1b8469afc40b6f5a2baaa146e332c4151788"
+  integrity sha512-VLYKXQmtmuEz6IxJsrZwzG9NvtkQsWNnWMsKxqWNu3+CnfzJQhp0WDDKWLVV9hLKr0l3SLLFRqcYHjhtyuDVxg==
+  dependencies:
+    "@sinclair/typebox" "^0.25.16"
+


it's unusual for check-ins to contain yarn.lock. Is it intentional here? What would it take to review it for correctness?

The commit added a devDpendency upon tsd. Almost any change in dependencies will change yarn.lock. Because that's automated there's not much to review. The important thing to review is the package.json changes. For yarn.lock you might want to verify there are no duplicates or that it hasn't grown dramatically. If there are duplicates they can be fixed with npx yarn-deduplicate.

turadg requested review from dckc and Chris-Hibbert April 17, 2023 18:59

turadg force-pushed the ta/start-types branch from fd35648 to dd6d85a Compare April 17, 2023 19:25

dckc reviewed Apr 17, 2023

View reviewed changes

turadg force-pushed the ta/start-types branch from dd6d85a to 7712dcb Compare April 17, 2023 19:43

dckc reviewed Apr 17, 2023

View reviewed changes

Chris-Hibbert reviewed Apr 17, 2023

View reviewed changes

turadg requested review from dckc and Chris-Hibbert April 18, 2023 22:22

dckc approved these changes Apr 18, 2023

View reviewed changes

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

turadg force-pushed the ta/start-types branch from 9ea223a to 3cf1b63 Compare April 18, 2023 23:01

chore(types): define ContractStartFunction

98b1700

turadg force-pushed the ta/start-types branch from 3cf1b63 to 68d4170 Compare April 19, 2023 16:25

Chris-Hibbert reviewed Apr 19, 2023

View reviewed changes

turadg added 6 commits April 19, 2023 09:50

test(types): startInstance type

eb5ee80

fix(types): fix some governance types

a681988

chore(types): define more installations

dc7af51

refactor: export GovernorFacetShape

ec8a5ad

fix(vaults): runtime types for governance

e91661d

chore(types): fix TypedMatcher for makeRecorderKit

ef2fff7

turadg force-pushed the ta/start-types branch from 68d4170 to ef2fff7 Compare April 19, 2023 16:50

mergify bot merged commit f3bb795 into master Apr 19, 2023

mergify bot deleted the ta/start-types branch April 19, 2023 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contract start types #7436

contract start types #7436

turadg commented Apr 17, 2023

dckc left a comment

dckc Apr 17, 2023

turadg Apr 18, 2023

dckc Apr 17, 2023

turadg Apr 18, 2023

dckc left a comment

dckc Apr 17, 2023

turadg Apr 17, 2023

turadg Apr 18, 2023

dckc Apr 18, 2023

dckc Apr 18, 2023

dckc Apr 17, 2023

turadg Apr 19, 2023

Chris-Hibbert left a comment

Chris-Hibbert Apr 17, 2023

dckc Apr 17, 2023

turadg Apr 18, 2023

Chris-Hibbert Apr 19, 2023

dckc left a comment

Chris-Hibbert Apr 19, 2023

Chris-Hibbert Apr 19, 2023

turadg Apr 19, 2023

Chris-Hibbert Apr 19, 2023

turadg Apr 19, 2023

Chris-Hibbert Apr 19, 2023

turadg Apr 19, 2023

contract start types #7436

contract start types #7436

Conversation

turadg commented Apr 17, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

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

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

Chris-Hibbert 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

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