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

feat(base-zone):zone.watchPromise #9321

Merged
merged 5 commits into from
May 7, 2024
Merged

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented May 5, 2024

closes: #9231

Description

Make zone.watchPromise available for all zones, only overridden by the durable zone. This provides a way for zone.exo* makers and methods to use a watchPromise that is compatible with their backing zone.

Security Considerations

Doesn't do anything already possible via VatData.watchPromise or E.when.

Scaling Considerations

Nothing outside of VatData.watchPromise and E.when.

Documentation Considerations

Zones need to be documented more thoroughly at some point, probably via TSDocs.

Testing Considerations

Will be tested as part of asyncFlow.

Upgrade Considerations

The usual.

@michaelfig michaelfig added the tooling repo-wide infrastructure label May 5, 2024
@michaelfig michaelfig self-assigned this May 5, 2024
@michaelfig michaelfig requested a review from erights May 5, 2024 22:04
@michaelfig michaelfig marked this pull request as ready for review May 5, 2024 23:01
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Thank you so so much. This is much cleaner.

I look forward to simplifying asyncFlow using this!

Comment on lines 59 to 60
The `@agoric/vat-data/vow.js` module allows Zones to integrate Agoric's vat
upgrade mechanism. To create vow tools that deal with durable objects:
Copy link
Member

Choose a reason for hiding this comment

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

Is vat-data/vow.js this still useful, or should one now only import from @agoric/base-zone and @agoric/vow ?

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've added @agoric/vow/vat.js to take a direct dependency on @agoric/internal (to identify SwingSet vat-upgrade promise rejections). Unfortunately, if @agoric/vow becomes @endo/vow, that will create a circular dependency with the Agoric SDK.

Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Sigh. Not at the present time. I'd be happy to see this merged with this issue + a TODO and/or filed issue explaining it, and then to cross that bridge when we migrate it.

packages/vow/src/watch.js Show resolved Hide resolved
Copy link

cloudflare-workers-and-pages bot commented May 6, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5da473f
Status: ✅  Deploy successful!
Preview URL: https://41ebbcdb.agoric-sdk.pages.dev
Branch Preview URL: https://mfig-9231-zone-watch-promise.agoric-sdk.pages.dev

View logs

@michaelfig michaelfig force-pushed the mfig-9231-zone-watch-promise branch 2 times, most recently from 08be4b1 to 5da473f Compare May 6, 2024 19:07
@erights erights self-requested a review May 6, 2024 20:12
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelfig michaelfig added the automerge:no-update (expert!) Automatically merge without updates label May 7, 2024
@mergify mergify bot merged commit 459998d into master May 7, 2024
71 checks passed
@mergify mergify bot deleted the mfig-9231-zone-watch-promise branch May 7, 2024 01:13
@@ -79,6 +83,7 @@ export const makeVirtualZone = (baseLabel = 'virtualZone') => {
subZone: wrapProvider(makeSubZone),

makeOnce,
watchPromise,
Copy link
Member

Choose a reason for hiding this comment

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

We probably should put as a caveat that this implementation is not "virtual" in the sense that it will have a heap cost keeping the watcher in the heap. In theory this should use the liveslots provided watchPromise, but there seems to be a bug there where providing a virtual watcher is not in fact supported.

mergify bot added a commit that referenced this pull request May 7, 2024
refs: #9321

## Description

Update the preferred way of using SwingSet vat-compatible vows to import
`@agoric/vow/vat.js`.

Also, as a drive-by in `@agoric/vow`, avoid `import('./file.js').Type`
in favour of the `@import {Type} from './file.js'` Typescript 5.5
syntax.

### Security Considerations

Doesn't change any code semantics.

### Scaling Considerations

n/a

### Documentation Considerations

`@agoric/vat-data/vow.js` is still available, as an alias for
`@agoric/vow/vat.js`.

### Testing Considerations

n/a

### Upgrade Considerations

No change in code functionality.
erights added a commit that referenced this pull request May 14, 2024
Staged on #9097

closes: #XXXX
refs: #9231 #9321 #9329 #9097

## Description

Now that `watchPromise` is abstracted over zones and vows make use of
that, we can simplify the test cases a lot.

Should be a pure refactor with no externally observable effect.

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

none
### Testing Considerations

tests simpler. Otherwise, none
### Upgrade Considerations

none
erights added a commit that referenced this pull request May 16, 2024
Staged on #9097

closes: #XXXX
refs: #9231 #9321 #9329 #9097

## Description

Now that `watchPromise` is abstracted over zones and vows make use of
that, we can simplify the test cases a lot.

Should be a pure refactor with no externally observable effect.

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

none
### Testing Considerations

tests simpler. Otherwise, none
### Upgrade Considerations

none
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 tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need zone to abstract over watchPromise or vowTools
3 participants