Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: many typing improvements #9516

Merged
merged 2 commits into from
Jun 24, 2024
Merged

fix: many typing improvements #9516

merged 2 commits into from
Jun 24, 2024

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Jun 17, 2024

refs: #9441

Description

Finish adopting WellKnownVats in VatLoader as proposed by @turadg in #9441

Accommodate Guarded types in:

  • packages/vats: Producer<T>.resolve
  • packages/governance

packages/swingset-liveslots: KindFacet implies RemotableBrand

Cast the return type of BridgeHandler.register because it wasn't an async function, and there's no way I know using TypeScript to be able to transform the return type of a generic function to Promise<ReturnType<T>> without losing T's genericity.

Copy link

cloudflare-workers-and-pages bot commented Jun 18, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 777eb21
Status: ✅  Deploy successful!
Preview URL: https://abfcc71e.agoric-sdk.pages.dev
Branch Preview URL: https://mfig-loadvat.agoric-sdk.pages.dev

View logs

@michaelfig michaelfig changed the title fix(vats): improve typing of Producer<T> and work around generic BridgeHandler.register fix: many typing improvements Jun 18, 2024
@michaelfig michaelfig requested review from turadg and gibson042 June 18, 2024 16:26
@michaelfig michaelfig marked this pull request as ready for review June 18, 2024 16:27
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love to see all the red, especially of ts-expect-error.

I had a few questions before approving

packages/smart-wallet/test/supports.js Outdated Show resolved Hide resolved
packages/swingset-liveslots/src/vatDataTypes.d.ts Outdated Show resolved Hide resolved
/**
* Helper to type the registered scoped bridge correctly.
*
* FIXME: This is needed because `register` is not an async function, so
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting. it does seem like we would get it working without this but I agree it's not worth the time right now and this is an appropriate working solution

@@ -103,13 +103,16 @@ type ClientProvider = {
};

type Producer<T> = {
resolve: (v: ERef<T>) => void;
resolve: (v: ERef<T | import('@endo/exo').Guarded<T>>) => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't T itself by the Guarded type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This was a leftover attempt before I started typing KindFacet correctly. Reverted.

const vatLoader = name => {
/** @type {VatLoader} */
const vatLoader = async name => {
/** @typedef {Awaited<WellKnownVats[typeof name]>} VatRoot */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline typedef seems smelly, but I don't have a better solution. Consider naming it ThisVatRoot or something else to make clear it's just for this switch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted for ReturnedVat.

@@ -117,13 +117,15 @@ export const makeMockTestSpace = async log => {
produce.zoe.resolve(zoe);
produce.feeMintAccess.resolve(feeMintAccessP);

const vatLoader = name => {
/** @type {VatLoader} */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a little loose since the type says it can load any vat and this handles just two of them

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up restricting the loadable names.

Comment on lines 530 to 534
orchestration: ERef<
ReturnType<
typeof import('@agoric/orchestration/src/vat-orchestration.js').buildRootObject
>
>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vat-orchestration exports this

Suggested change
orchestration: ERef<
ReturnType<
typeof import('@agoric/orchestration/src/vat-orchestration.js').buildRootObject
>
>;
orchestration: OrchestrationVat;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, adapted.

priceAuthority: PriceAuthorityVat;
provisioning: ProvisioningVat;
transfer: TransferVat;
zoe: ERef<ReturnType<typeof import('../vat-zoe.js').buildRootObject>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we have vat-zoe export this as vat-orchestration does?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shall, so I did!

@michaelfig michaelfig self-assigned this Jun 23, 2024
@michaelfig michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Jun 23, 2024
@mergify mergify bot merged commit d941b39 into master Jun 24, 2024
85 checks passed
@mergify mergify bot deleted the mfig-loadVat branch June 24, 2024 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants