explicit exports of Zoe service and contract facet types #11075

turadg · 2025-03-04T03:01:06Z

Description

While working to move Fast USDC (#10811) I kept running into problems with types resolving. I started working on the tech debt of #6343 and slipped down that rabbit hole.

This makes the ambient type declarations of ZoeService and ZCF modules into explicit exports and updates a ton of code to cope. Review by commit!

Some collateral improvements along the way:

governance type improvements
return types of Zoe helpers
Typesscript 5.8 release
cosmic-swingset conflicting dependencies

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

I don't think we have docs on type imports

Testing Considerations

CI

Upgrade Considerations

This will break modules importing the types-ambient.js files that are gone. Those were deep imports so not part of the package semver.

dckc · 2025-03-04T05:44:06Z

This will break modules importing the types-ambient.js files that are gone. Those were deep imports so not part of the package semver.

My impression is that anything that another package can import is part of the package API. Help me find the definition of semver you're working with?

dckc · 2025-03-04T05:48:24Z

This looks like progress on some other related issues:

Perhaps some of those should be closed as dups / not planned at some point.

dckc

I skimmed each of the commits; the changes all look straightforward / rote with no runtime impact (except 1 possible case).

I'm not quite clear on the DX impact on getPublicFacet; I should look into that more.

dckc · 2025-03-04T05:08:04Z

packages/SwingSet/package.json

-    "atLeast": 76.28
+    "atLeast": 76.25


What should I look for when reviewing changes in typeCoverage floor?

Ideally it goes up. It should never go down too much, per reviewer's judgement

Is there a dashboard?

packages/zoe/src/contracts/loan/types-ambient.js

packages/zoe/tsconfig.json

dckc · 2025-03-04T05:16:54Z

packages/orchestration/src/types.ts

-// For ZCF which is only available ambiently
-/// <reference types="@agoric/zoe/src/contractFacet/types-ambient" />
+import type { ZCF } from '@agoric/zoe';


packages/zoe/src/contractFacet/types.ts

dckc · 2025-03-04T05:31:34Z

packages/governance/test/swingsetTests/committeeBinary/vat-voter.js

+      /** @type {any} */
      const electorateInstance = await E(zoe).getInstance(invitation);


did getInstance get hit by the same thing that affected getPublicFacet?

dckc · 2025-03-04T05:34:12Z

packages/ERTP/src/amountMath.js

 * @param {V} value
 * @returns {MathHelpers<V>}
 */
 export const assertValueGetHelpers = value =>
  // @ts-expect-error cast
  helpers[assertValueGetAssetKind(value)];

-/** @type {(allegedBrand: Brand, brand?: Brand) => void} */
+/**
+ * @type {(allegedBrand: Brand<any>, brand?: Brand<any>) => void}


Brand's type parameter is no longer optional? If I'm reading that right, that's an awfully big DX regression.

it still is but the default is AssetKind, not any. This change was needed only here. I didn't look deeply into why

dckc · 2025-03-04T05:36:12Z

packages/internal/src/config.js

@@ -31,9 +31,10 @@ export const BridgeId = /** @type {const} */ ({
 });
 harden(BridgeId);

+/** @satisfies {Record<string, BridgeId>} */
 export const CosmosInitKeyToBridgeId = {


harden this while we're here?

slippery slope, I suppose. This PR is already pretty big. And I think so far I haven't see any changes that have runtime impact.

dckc · 2025-03-04T05:38:05Z

packages/zoe/src/contractFacet/types.ts

+// XXX runtime import of types
+import './internal-types.js';


how could this have runtime impact? it's a .ts file; don't we provide a companion .js file that's empty?

good point. I should say "side-effect" import is more accurate.

dckc · 2025-03-04T05:38:52Z

packages/zoe/src/zoeService/types.ts

-  bundleID: BundleID,
+  bundleID: string,


this looks like an unfortunate regression... or ... was BundleID previously any here?

changed because the type couldn't resolve. But I think it wasn't serving much value. The property is named bundleId. The type was always string.

Is there anything more specific about a BundIeID than string? Maybe b1${string}

the type couldn't resolve.

That's the sort of thing I had in mind when asking whether it was previously any.

https://github.com/Agoric/agoric-sdk/blob/master/packages/SwingSet/src/types-external.js#L169 doesn't have any non-trivial JSDoc, but https://github.com/Agoric/agoric-sdk/blob/master/packages/SwingSet/docs/bundles.md#bundleids has a couple paragraphs of documentation.

I'm pretty sure there's a b0- thing too, though the swingset docs seem to exclude it.

I think some sort of // XXX reconnect to BundleID type docs marker is worthwhile. Not critical.

b1-${string}.

My comment was written before I saw Dan's. I was merely pointing out the dash in the pattern, not responding to the possibility of b0-. I think b0- is used in different places, but I'd check with @kriskowal to be sure.

Chris-Hibbert · 2025-03-04T18:43:15Z

Can we say this closes #8645?

cloudflare-workers-and-pages · 2025-03-04T19:09:44Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`2198f64`
Status:	✅ Deploy successful!
Preview URL:	https://8b8891e0.agoric-sdk.pages.dev
Branch Preview URL:	https://6343-unambient-zoe.agoric-sdk.pages.dev

View logs

turadg · 2025-03-04T19:11:14Z

Can we say this closes #8645?

I don't think this touches that

dckc

#10811 is sufficiently important that this is worth landing even if it results in a few bumps in the road.

Chris-Hibbert · 2025-03-04T18:46:28Z

packages/SwingSet/package.json

-    "atLeast": 76.28
+    "atLeast": 76.25


Is there a dashboard?

Chris-Hibbert · 2025-03-04T19:10:23Z

packages/governance/src/types.js

@@ -688,7 +689,7 @@ export {};
 *
 * @param {ERef<ZoeService>} zoe
 * @param {import('@agoric/zoe/src/zoeService/utils.js').Instance<(zcf: ZCF<GovernanceTerms<{}>>) => {}>} allegedGoverned
- * @param {Instance} allegedGovernor
+ * @param {Instance<(zcf) => {publicFacet: GovernorPublic}>} allegedGovernor


I'm pretty sure the allegedGovernor (here and in AssertContractElectorate) is an instance. Doesn't this say it's a function? The first use in both funct is

const allegedGovernorPF = E(zoe).getPublicFacet(allegedGovernor);

This also says it's an Instance. What I added is a contract start fn type parameter so downstream uses know what is the result of starting the instance

Chris-Hibbert · 2025-03-04T19:11:05Z

packages/governance/src/validators.js

@@ -27,6 +27,7 @@ const assertContractGovernance = async (
    await Promise.all([allegedGovernedTermsP, realGovernedP]);

  assert(
+    // @ts-expect-error XXX types


Do you have a theory?

just tech debt

Chris-Hibbert · 2025-03-04T19:13:13Z

packages/vats/src/core/types-ambient.d.ts

@@ -232,6 +232,7 @@ type WellKnownContracts = {
  provisionPool: typeof import('@agoric/inter-protocol/src/provisionPool.js').start;
  priceAggregator: typeof import('@agoric/inter-protocol/src/price/fluxAggregatorContract.js').start;
  reserve: typeof import('@agoric/inter-protocol/src/reserve/assetReserve.js').start;
+  reserveGovernor: typeof import('@agoric/governance/src/contractGovernor.js').start;


redundant with contractGovernor?

no because this is another entry in the space. it just happens to have the same type as contractGovernor

Chris-Hibbert · 2025-03-04T19:20:55Z

packages/zoe/src/zoeService/types.ts

-  bundleID: BundleID,
+  bundleID: string,


b1-${string}.

Chris-Hibbert · 2025-03-04T19:27:39Z

packages/orchestration/src/examples/stake-bld.contract.js

@@ -112,7 +113,8 @@ export const start = async (zcf, privateArgs, baggage) => {
            trace('makeStakeBldInvitation', give);
            const { holder } = await makeLocalAccountKit();
            /** @type {Record<string, Payment<'nat'>>} */
-            // @ts-expect-error XXX PaymentPKeywordRecord throught deeplyFulfilled will be a PaymnentKeywordRecord
+            // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- fails only through some build paths
+            // @ts-ignore XXX PaymentPKeywordRecord throught deeplyFulfilled will be a PaymnentKeywordRecord


Let's fix the typos. This is really hard to read as is.

Suggested change

// @ts-ignore XXX PaymentPKeywordRecord throught deeplyFulfilled will be a PaymnentKeywordRecord

// @ts-ignore XXX PaymentPKeywordRecord, though after deeplyFulfilled it will be a PaymentKeywordRecord

if I have to do another push I'll include this

b1-${string}

Good suggestion. I did try that and a bunch of places have full wide string or use a different literal like id (in a test). So out of scope

Is there a dashboard?

nope

Where do these numbers come from? What do they get compared to?

try git grep typeCoverage

mergify · 2025-03-04T20:05:07Z

This pull request has been removed from the queue for the following reason: pull request manually updated.

The pull request #11075 has been manually updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

turadg · 2025-03-04T20:08:20Z

@mergify requeue

mergify · 2025-03-04T20:08:23Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

turadg requested review from dckc and Chris-Hibbert March 4, 2025 03:01

turadg requested a review from a team as a code owner March 4, 2025 03:01

dckc reviewed Mar 4, 2025

View reviewed changes

Agoric deleted a comment from dckc Mar 4, 2025

dckc approved these changes Mar 4, 2025

View reviewed changes

turadg added 8 commits March 4, 2025 11:21

chore(deps): repeated dependencies

ba70595

chore(deps): bump typescript to release

f6ddc77

chore(deps): bump typescript-eslint

32b831e

chore(types): scripts/update-typeCoverage-floor.sh

52fc2fe

chore(types)!: unambient contractFacet types

c0671e7

chore(types): explicit zoe contractFacet imports

6d00a32

feat(types): zoeService explicit exports

ac2ddae

feat(types): generic Instance

9a0359a

turadg force-pushed the 6343-unambient-zoe branch from 801823c to 6b67182 Compare March 4, 2025 19:30

turadg added the automerge:rebase Automatically rebase updates, then merge label Mar 4, 2025

Chris-Hibbert reviewed Mar 4, 2025

View reviewed changes

turadg added 7 commits March 4, 2025 12:04

chore(types): zoeService explicit imports

7d25041

chore(types): fallout

177d73b

chore(types): more explicit imports

b471e59

chore(types): more explicit imports

b5712f1

chore(types): restore zoe globals

903f0ea

chore(types): remove contractFacet internal types

13a49e3

chore(types): scripts/update-typeCoverage-floor.sh

2198f64

turadg force-pushed the 6343-unambient-zoe branch from 6b67182 to 2198f64 Compare March 4, 2025 20:04

mergify bot merged commit 3d98aac into master Mar 4, 2025
84 checks passed

mergify bot deleted the 6343-unambient-zoe branch March 4, 2025 21:15

		/** @type {any} */
		const electorateInstance = await E(zoe).getInstance(invitation);

	// @ts-ignore XXX PaymentPKeywordRecord throught deeplyFulfilled will be a PaymnentKeywordRecord
	// @ts-ignore XXX PaymentPKeywordRecord, though after deeplyFulfilled it will be a PaymentKeywordRecord

explicit exports of Zoe service and contract facet types #11075

explicit exports of Zoe service and contract facet types #11075

Conversation

turadg commented Mar 4, 2025 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

dckc commented Mar 4, 2025

dckc commented Mar 4, 2025

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 Mar 4, 2025 • 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

Chris-Hibbert commented Mar 4, 2025

cloudflare-workers-and-pages bot commented Mar 4, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

turadg commented Mar 4, 2025

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

turadg Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 4, 2025

turadg commented Mar 4, 2025

mergify bot commented Mar 4, 2025

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

turadg commented Mar 4, 2025 •

edited

Loading

turadg Mar 4, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Mar 4, 2025 •

edited

Loading

turadg Mar 4, 2025 •

edited

Loading