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

Need zone to abstract over watchPromise or vowTools #9231

Closed
erights opened this issue Apr 14, 2024 · 2 comments · Fixed by #9321
Closed

Need zone to abstract over watchPromise or vowTools #9231

erights opened this issue Apr 14, 2024 · 2 comments · Fixed by #9321
Assignees
Labels
bug Something isn't working

Comments

@erights
Copy link
Member

erights commented Apr 14, 2024

At

const testFirstPlay = async (t, zone, vowTools) => {
I found I had to pass in the vowTools to the test cases that should work across zones because

  • the prepareVowTools from @agoric/vow works with the heap and virtual zones, but not the durable zone, and therefore does not support recovery from upgrade.
  • the prepareVowTools from @agoric/vat-data/vow.js works with the durable zone and supports recovery from upgrade, but does not work with the heap or virtual zones.

I raised the with @michaelfig and, IIRC, we agreed that

  • the underlying problem was the difference between the watchPromise exported by liveSlots (supporting durability and upgrade) vs the watchPromiseShim in @agoric/vow intended for cases where the "real" watchPromise is not available
  • it might be reasonable to extend the Zone type to provide a watchPromise method that works with that zone. Then, code like the test cases in test-async-flow.js need only be parameterized over zone, and no longer additionally parameterized over vowTools.

This would address the issues @mhofman raises at #9097 (comment)

@erights erights added the bug Something isn't working label Apr 14, 2024
@erights
Copy link
Member Author

erights commented Apr 14, 2024

See also #9097 (comment)

@erights
Copy link
Member Author

erights commented Apr 15, 2024

@michaelfig , ok to assign this to the two of us?

@erights erights changed the title Need zone to abstract over watchPromise or vowTools Need zone to abstract over watchPromise or vowTools Apr 15, 2024
mergify bot added a commit that referenced this issue May 7, 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.
@mergify mergify bot closed this as completed in #9321 May 7, 2024
erights added a commit that referenced this issue 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 issue 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
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants