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.

Sign up for GitHub

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

Jump to bottom

typecheck vats package #8596

Merged
merged 7 commits into from
Dec 4, 2023
Merged

typecheck vats package #8596

merged 7 commits into from
Dec 4, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Dec 2, 2023

refs: #8445

Description

#8488 had a hard to find bug that would have been easier with typechecking. The proposal wasn't being type-checked because in @agoric/vats the typecheck is opt-in.

This changes it to opt-out. I confirmed with yarn type-coverage that it doesn't lose any, and in fact it goes up.

This also provides some type annotations that might also have helped detect the bug.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Self-documenting

Testing Considerations

CI

Upgrade Considerations

n/a

@turadg turadg changed the title typescript vats package typecheck vats package Dec 2, 2023
Chris-Hibbert
Chris-Hibbert reviewed Dec 2, 2023
View reviewed changes
packages/zoe/src/zoeService/installationStorage.js
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/prefer-ts-expect-error -- accomodate different type search depths */
Copy link
Contributor

Choose a reason for hiding this comment

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

How much type-checking coverage are we losing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is just to allow using ts-ignore instead of ts-expect-error

dckc
dckc approved these changes Dec 2, 2023
View reviewed changes
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

works for me

packages/governance/src/contractHelper.js
@@ -152,7 +152,8 @@ const facetHelpers = (zcf, paramManager) => {
*
* @see {makeDurableGovernorFacet}
*
* @param {{ [methodName: string]: (context?: unknown, ...rest: unknown[]) => unknown}} limitedCreatorFacet
* @template {{ [methodName: string]: (context?: unknown, ...rest: unknown[]) => unknown}} LCF
* @param {LCF} limitedCreatorFacet
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be an endless amount of refinement to do for the types for governance.

I wonder if really complex types is like really complex documentation: it suggests searching for a simpler design.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. The design we shipped with was somewhat rushed. Making it durable is also rushed (and incomplete). When making it durable we could simplify, or do that in a next phase.

packages/vats/index.js
@@ -1,4 +1,3 @@
// @ts-check
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why this should be taken out.
A comment would be ideal.

Copy link
Member

Choose a reason for hiding this comment

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

oh. the tsconfig is opt-out now.

I'm uneasy about making such signals implicit. But I suppose it's better than having the next person forget make a new file and forget to include it.

packages/vats/package.json
"atLeast": 90.86
"atLeast": 90.88
Copy link
Member

Choose a reason for hiding this comment

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

👏

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Dec 2, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2023

@turadg - This PR appears to be stuck. It's had a merge label for > 24 hours

turadg
turadg commented Dec 4, 2023
View reviewed changes
packages/swingset-liveslots/src/watchedPromises.js
function providePromiseWatcher(
kindHandle,
fulfillHandler = x => x,
rejectHandler = x => {
throw x;
Copy link
Member Author

Choose a reason for hiding this comment

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

@FUDCo removing this throw make sense to you? I think when when a rejectHandler throws that ends up as an unhandled promise rejection

Copy link
Contributor

Choose a reason for hiding this comment

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

At first glance I thought you were right, but on further inspection I'm not sure that's so (though I'm not deeply confident of that conclusion). In particular, it looks to me like there's a call chain from startVat that can result in somebody catching the exception.

On the other hand, I think the default fulfillHandler is questionable, since from the way it's invoked it doesn't look to me like there's ever going to be anybody positioned to receive the return value.

@mergify mergify bot merged commit 1b5e57f into master Dec 4, 2023
67 checks passed
@mergify mergify bot deleted the ta/typecheck-scripts branch December 4, 2023 21:38
@mhofman mhofman mentioned this pull request Jun 14, 2024
Merged
mergify bot pushed a commit that referenced this pull request Jun 14, 2024
@mhofman
fix(liveslots): promise watcher to cause unhandled rejection if no ha…
a19a964 
…ndler (#9507)

refs: #8596

## Description

Reverts a behavioral change questioned in #8596 (comment), which was indeed not appropriate.

If the reject handler is missing, it should result in an unhandled rejection.

### Security Considerations
None

### Scaling Considerations
None

### Documentation Considerations
None

### Testing Considerations
No coverage

### Upgrade Considerations
Maintains currently deployed behavior on mainnet
mhofman added a commit that referenced this pull request Jun 20, 2024
@mhofman
fix(liveslots): promise watcher to cause unhandled rejection if no ha…
424e490 
…ndler (#9507)

refs: #8596

## Description

Reverts a behavioral change questioned in #8596 (comment), which was indeed not appropriate.

If the reject handler is missing, it should result in an unhandled rejection.

### Security Considerations
None

### Scaling Considerations
None

### Documentation Considerations
None

### Testing Considerations
No coverage

### Upgrade Considerations
Maintains currently deployed behavior on mainnet
mhofman added a commit that referenced this pull request Jun 22, 2024
@mhofman
fix(liveslots): promise watcher to cause unhandled rejection if no ha…
0de0c62 
…ndler (#9507)

refs: #8596

## Description

Reverts a behavioral change questioned in #8596 (comment), which was indeed not appropriate.

If the reject handler is missing, it should result in an unhandled rejection.

### Security Considerations
None

### Scaling Considerations
None

### Documentation Considerations
None

### Testing Considerations
No coverage

### Upgrade Considerations
Maintains currently deployed behavior on mainnet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FUDCo FUDCo FUDCo left review comments

@Chris-Hibbert Chris-Hibbert Chris-Hibbert left review comments

@dckc dckc dckc approved these changes

Assignees
No one assigned
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@turadg @dckc @FUDCo @Chris-Hibbert