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

feat: `vowTools.allSettled` #10077

Merged

mergify merged 4 commits into master from pc/all-vows-settled

Sep 18, 2024

Member

0xpatrickdev commented Sep 12, 2024 •

edited

Loading

Description

adds vowTools helper that mimics the behavior of Promise.allSettled to support feat: zoeTools.localTransfer error paths #9902
tests unstorable results path which resulted in a type guard change

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Updates vow/README.md with information about VowTools

Testing Considerations

Includes unit tests

Upgrade Considerations

n/a, library code

0xpatrickdev requested a review from michaelfig

September 12, 2024 15:51

0xpatrickdev mentioned this pull request

feat: zoeTools.localTransfer error paths #9902

Merged

0xpatrickdev force-pushed the pc/all-vows-settled branch from e1b5f2e to 891b473 Compare

September 12, 2024 16:02

cloudflare-workers-and-pages bot commented Sep 12, 2024 •

edited

Loading

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`c3babe6`
Status:	✅ Deploy successful!
Preview URL:	https://d9a47cf3.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-all-vows-settled.agoric-sdk.pages.dev

0xpatrickdev commented

View reviewed changes

packages/vow/src/watch-utils.js

		@@ -119,27 +173,36 @@ export const prepareWatchUtils = (
		}
		return kit.vow;

Member Author

0xpatrickdev Sep 12, 2024

Is there a path for typing this, so we can keep the existing Vow<any[]> for allVows and something like Vow<({status: 'fulfilled', value: any} | {status: 'rejected', reason: Error})[]> for allVowsSettled?

h/t @Chris-Hibbert for the suggestion: https://github.com/Agoric/agoric-sdk/pull/9902/files#r1757395986

Member

michaelfig Sep 13, 2024

I've given this a shot above, but it is wholly untested. Hope it helps.

Member Author

0xpatrickdev Sep 13, 2024

Thanks, your suggestion worked! In hindsight, it was obvious 😅

michaelfig reviewed

View reviewed changes

packages/vow/src/watch-utils.js Outdated Show resolved Hide resolved

packages/vow/src/watch-utils.js Outdated

Comment on lines 134 to 135

		/**
		* @param {EVow<unknown>[]} vows

Member

michaelfig Sep 13, 2024

Pet peeve; the arguments are not vows, they're unknown specimens that might be vows. That's what EVow describes:

Suggested change

      
                    /**
          
                     * @param {EVow<unknown>[]} vows
          
                    /**
          
                     * @param {EVow<unknown>[]} specimens

Member

turadg Sep 13, 2024

+1. regardless of pet peeves, this is a code interpretability issue worth improving.

incidentally, since ERef is "a ref or eventually a ref" one is likely to read EVow as "a vow or eventually a vow" but it's really "ERef or Vow". We could consider some other names like EVRef or VowRef.

Member

michaelfig Sep 16, 2024

Thinking about this further, the type should actually be:

* @param {unknown[]} specimens

packages/vow/src/watch-utils.js Outdated Show resolved Hide resolved

packages/vow/src/watch-utils.js

		@@ -119,27 +173,36 @@ export const prepareWatchUtils = (
		}
		return kit.vow;

Member

michaelfig Sep 13, 2024

I've given this a shot above, but it is wholly untested. Hope it helps.

0xpatrickdev commented

View reviewed changes

packages/vow/test/watch-utils.test.js Outdated Show resolved Hide resolved

0xpatrickdev marked this pull request as draft

September 13, 2024 16:29

0xpatrickdev force-pushed the pc/all-vows-settled branch 3 times, most recently from 26a118a to baedae2 Compare

September 16, 2024 18:18

0xpatrickdev marked this pull request as ready for review

September 16, 2024 18:21

0xpatrickdev requested review from michaelfig and turadg

September 16, 2024 18:27

0xpatrickdev added the force:integration label

turadg requested changes

View reviewed changes

packages/vow/src/tools.js Outdated Show resolved Hide resolved

0xpatrickdev force-pushed the pc/all-vows-settled branch from baedae2 to 8c50684 Compare

September 16, 2024 23:34

0xpatrickdev requested a review from turadg

September 16, 2024 23:36

turadg approved these changes

View reviewed changes

packages/vow/README.md

@@ @@ -77,6 +77,67 @@ const { watch, makeVowKit } = prepareVowTools(vowZone); @@
               // Vows and resolvers you create can be saved in durable stores.
               ```
+              ## VowTools

Member

turadg Sep 16, 2024

👏 !

packages/vow/src/tools.js Outdated Show resolved Hide resolved

0xpatrickdev force-pushed the pc/all-vows-settled branch 2 times, most recently from 82c4902 to 2bf7f89 Compare

September 17, 2024 14:58

0xpatrickdev commented

View reviewed changes

packages/vow/README.md Outdated Show resolved Hide resolved

michaelfig reviewed

View reviewed changes

Member

michaelfig left a comment

Some clarifications for the README.md. I haven't looked further yet.

packages/vow/README.md Outdated Show resolved Hide resolved

packages/vow/README.md Outdated Show resolved Hide resolved

packages/vow/README.md Outdated Show resolved Hide resolved

packages/vow/README.md Outdated Show resolved Hide resolved

packages/vow/README.md Outdated Show resolved Hide resolved

0xpatrickdev removed the force:integration label

0xpatrickdev force-pushed the pc/all-vows-settled branch from eaca71b to 947b150 Compare

September 18, 2024 15:57

michaelfig changed the title ~~feat: vowTools.allVowsSettled~~ feat: vowTools.allSettled

michaelfig approved these changes

View reviewed changes

Member

michaelfig left a comment

LGTM! Well done.

0xpatrickdev and others added 3 commits

September 18, 2024 18:41


          feat: loosen watcher type guards to support non-passables

5135b9c

Co-authored-by: Michael FIG <[email protected]>


          refactor(vowTools): rename allVows to all, deprecate allVows

ac7fc7b


          feat: vowTools.allSettled

3949b10

- adds vowTools helper that mimics the behavior of Promise.allSettled


          docs: vowTools

c3babe6

0xpatrickdev force-pushed the pc/all-vows-settled branch from 947b150 to c3babe6 Compare

September 18, 2024 22:42

0xpatrickdev added the automerge:rebase label

mergify bot merged commit 6ee57ba into master

90 checks passed

mergify bot deleted the pc/all-vows-settled branch

September 18, 2024 23:15

mergify bot added a commit that referenced this pull request


          feat: zoeTools.localTransfer error paths (#9902)

3e9ff43

closes: #9925
refs: #9193

## Description
- This PR primarily hardens the `zoeTools` implementation for orchestrated async-flows by:
  - ensuring payments handled in `zoeTools.localTransfer` are recovered to the original seat in the event of failure or partial failure
  - providing a `zoeTools.withdrawFromSeat` retriable function to facilitate withdrawing funds from a LocalChainAccount to a User seat. Ensures all changes are rolled back in the event of partial failure.
   - using `asVow` instead of `retriable` since these operations are not idempotent
- Creates `src/fixtures/zoe-tools.contract.js` to facilitate testing error paths
- To facilitate the change, `vowTools.allVowsSettled` was implemented in `@agoric/vow`. See #10077 which this PR depends on

### Security Considerations
Improves safety around Payment handling in async-flow contracts in `@agoric/orchestration`

### Scaling Considerations
n/a

### Documentation Considerations
Updates jsdoc strings for `localTransfer` and `withdrawToSeat`. As maintainer notes for considerations around `asVow` and promptness.

### Testing Considerations
Includes tests with different failure paths previously unaccounted for.

### Upgrade Considerations
Unreleased code that will be part of an npm release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge:rebase