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(vows): improve handling of ephemeral values #9620

Merged
merged 7 commits into from
Jul 1, 2024

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Jun 30, 2024

refs: #9308

Description

  • adds asPromise helper to VowTools for unwrapping Vow|Promise, ensuring proper handling of ephemeral promises
  • updates watch-utils to better handle values that are not storable durably, such as promises

Testing Considerations

This does not include tests that simulate an upgrade - only a heap zone is used in the included tests. See #9631

Upgrade Considerations

This PR includes changes we'd like to be in the initial release of vows.

Copy link

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

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: ff92211
Status: ✅  Deploy successful!
Preview URL: https://45e89b2b.agoric-sdk.pages.dev
Branch Preview URL: https://pc-watch-utils-as-promise.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev marked this pull request as ready for review July 1, 2024 00:21
* @param {Watcher<T, TResult1, TResult2, C>} [watcher]
* @param {C} [watcherArgs]
* @returns {Promise<TResult1 | TResult2>}
*/
Copy link
Member Author

@0xpatrickdev 0xpatrickdev Jul 1, 2024

Choose a reason for hiding this comment

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

This is identical to the type for watch, minus the return line. We might consider extracting that logic and extending it, and/or using something like the PromiseToVow helper from @agoric/orchestration that should maybe live here instead:

/**
* Converts a function type that returns a Promise to a function type that
* returns a Vow. If the input is not a function returning a Promise, it
* preserves the original type.
*
* @template T - The type to transform
*/
export type PromiseToVow<T> = T extends (
...args: infer Args
) => Promise<infer R>
? (...args: Args) => Vow<R>
: T extends (...args: infer Args) => infer R
? (...args: Args) => R
: T;

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 lines 14-15 above be removed since they're redundant?

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Some F2-practice for you, and just a few minor things. 😉

packages/vow/src/vow.js Outdated Show resolved Hide resolved
packages/vow/src/vow.js Outdated Show resolved Hide resolved
packages/vow/src/vow.js Show resolved Hide resolved
packages/vow/src/vow.js Show resolved Hide resolved
packages/vow/src/watch-utils.js Outdated Show resolved Hide resolved
packages/vow/src/watch-utils.js Outdated Show resolved Hide resolved
packages/vow/src/watch-utils.js Outdated Show resolved Hide resolved
packages/vow/src/watch-utils.js Outdated Show resolved Hide resolved
packages/vow/src/watch-utils.js Outdated Show resolved Hide resolved
packages/vow/src/watch-utils.js Outdated Show resolved Hide resolved
@0xpatrickdev 0xpatrickdev force-pushed the pc/watch-utils-as-promise branch 2 times, most recently from ac14194 to 80e9d83 Compare July 1, 2024 21:49
@0xpatrickdev 0xpatrickdev changed the title feat(vowTools): asPromise helper for unwrapping Vow|Promise feat(vows): improve handling of ephemeral values Jul 1, 2024
@0xpatrickdev 0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Jul 1, 2024
@mergify mergify bot merged commit 8edf902 into master Jul 1, 2024
85 checks passed
@mergify mergify bot deleted the pc/watch-utils-as-promise branch July 1, 2024 23:22
gibson042 pushed a commit that referenced this pull request Jul 2, 2024
refs: #9308

## Description
- adds `asPromise` helper to `VowTools` for unwrapping `Vow|Promise`, ensuring proper handling of ephemeral promises
- updates watch-utils to better handle values that are not storable durably, such as promises

### Testing Considerations
This does not include tests that simulate an upgrade - only a heap zone is used in the included tests. See #9631

### Upgrade Considerations
This PR includes changes we'd like to be in the initial release of vows.
gibson042 added a commit that referenced this pull request Jul 2, 2024
refs: #9308

## Description
- adds `asPromise` helper to `VowTools` for unwrapping `Vow|Promise`, ensuring proper handling of ephemeral promises
- updates watch-utils to better handle values that are not storable durably, such as promises

### Testing Considerations
This does not include tests that simulate an upgrade - only a heap zone is used in the included tests. See #9631

### Upgrade Considerations
This PR includes changes we'd like to be in the initial release of vows.
gibson042 pushed a commit that referenced this pull request Jul 2, 2024
refs: #9308

## Description
- adds `asPromise` helper to `VowTools` for unwrapping `Vow|Promise`, ensuring proper handling of ephemeral promises
- updates watch-utils to better handle values that are not storable durably, such as promises

### Testing Considerations
This does not include tests that simulate an upgrade - only a heap zone is used in the included tests. See #9631

### Upgrade Considerations
This PR includes changes we'd like to be in the initial release of vows.
gibson042 added a commit that referenced this pull request Jul 2, 2024
git rebase-todo
```
# Branch pc/as-vow
label base-pc-as-vow
pick b6b5f5f feat(vowTools): add asVow helper
pick 0cdcd5f feat(vowTools): asVow should not wrap a vow as a vow
label pc-as-vow
reset base-pc-as-vow
merge -C 0ad10c6 pc-as-vow # feat(vowTools): add asVow helper (#9577)

# Branch pc/watch-utils-as-promise
label base-pc-watch-utils-as-promise
pick bf430a1 feat(watchUtils): add asPromise helper
# To resolve conflicts in the following commit:
# * In packages/vow/src/types.js, keep inbound changes and (after that) the
#   definition of VowTools.
# * In packages/vow/test/watch-utils.test.js, keep only inbound changes and
#   limit them to start at "asPromise converts a vow to a promise" (i.e.,
#   just the additions of
#   https://github.com/Agoric/agoric-sdk/pull/9620/files#diff-fd7cabcc7d1097036e2981c3262aa20612bec349f6e418d53324fc33b1b26946 ).
pick c940d5c feat(vowTools): asPromise helper for unwrapping vows
pick 8c27c67 feat(watchUtils): handle non-storables
pick 3d5a3f3 feat(types): EVow
pick 1c0b964 refactor: don't re-use index
pick 274df18 fix(vow): clearer stored/non-stored values
pick ff92211 refactor: 'extra' field for future properties
label pc-watch-utils-as-promise
reset base-pc-watch-utils-as-promise
merge -C 8edf902 pc-watch-utils-as-promise # feat(vows): improve handling of ephemeral values (#9620)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants