misc Orch factoring improvements #9676

turadg · 2024-07-10T00:04:37Z

Description

Adapted from #9657 . The retriable helper and withOrchestration refactor landed already.

This has the rest:

orchestrateAll helper
separate module for flows
ZoeTools endowment (for withdrawFromSeat à la orchestration facade returns promises backed by vows #9449 )
makeOrchestrator abstraction

Security Considerations

orchestrateAll grants the same context to each orchFn. If they have to be different, the author can make multiple calls grouping what is the same.

Improves POLA of some Orchestration services by passing a makeOrchestrator.

Scaling Considerations

none

Documentation Considerations

none yet

Testing Considerations

CI

Upgrade Considerations

not yet deployed

cloudflare-workers-and-pages · 2024-07-10T00:05:27Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`ecb67b1`
Status:	✅ Deploy successful!
Preview URL:	https://5a41eef7.agoric-sdk.pages.dev
Branch Preview URL:	https://ta-orch-factoring.agoric-sdk.pages.dev

View logs

0xpatrickdev

LGTM. Holding approval til I better understand testing and the comment around seat recoverability.

0xpatrickdev · 2024-07-10T16:17:29Z

packages/orchestration/src/utils/start-helper.js

+  return {
+    ...facade,
+    chainHub,
+    vowTools,
+    zoeTools,
+    zone: zones.contract,
+  };


Tangential - wondering what parts of this we can make widely available in the bootstrap space. Thinking the benefits would be:

Smaller bundles for guest code + less code on chain

(marginal) less setup for devs since they can just grab a power

None because these all depend on a baggage from the host vat. We could put the withOrchestration function in bootstrap to save bundling, but the more general solution to deduping across bundles is to let bundles import from other bundles. That's on the roadmap.

We can consider less maintainable hacks if we run up against bundle size limits and have no other options that are more maintainable.

0xpatrickdev · 2024-07-10T16:30:26Z

packages/orchestration/src/examples/sendAnywhereFlows.js

+// in guest file (the orchestration functions)
+// the second argument is all the endowments provided
+
+export const orchestrationFns = harden({


I'm not crazy about moving this to a separate file. Did we feel sendAnywhere.contract.js got too big, or was there a different motivation?

Motivation is that it's a different runtime environment than the contract. Keeping the flows in a separate file will help prevent trying to access host values directly. Also it will let us improve linting. E.g. the "no async" lint rules need only apply to these files, not the contract.

Keeping the flows in a separate file will help prevent trying to access host values directly

Agree, but putting them at the top of the file also achieves the same

the "no async" lint rules

Think you meant to say no E, but I see your point. Noting this can be achieved intra-file with something like Property[key.name=/OfferHandler$/] or Property[key.name=/Handler$/] if we enforce the convention (sendIt -> sendItHandler)

0xpatrickdev · 2024-07-10T16:32:47Z

packages/orchestration/src/examples/sendAnywhereFlows.js

+  /**
+   * @param {Orchestrator} orch
+   * @param {object} ctx
+   * @param {{ account: OrchestrationAccount<any> }} ctx.contractState
+   * @param {any} ctx.localTransfer
+   * @param {any} ctx.findBrandInVBank
+   * @param {ZCFSeat} seat
+   * @param {{ chainName: string; destAddr: string }} offerArgs
+   */
+  async sendIt(
+    orch,
+    { contractState, localTransfer, findBrandInVBank },
+    seat,


Is there anything we can do to reduce boilerplate around this? (orch, ctx, seat params). The bulk of the noise in kitchen-sink.contract.js seems to be from these.

Maybe curry orch and ctx so sendIt takes the normal OfferHandler type?

because sendIt is a flow (guest fn) and not a regular function I think that's asking for trouble

0xpatrickdev · 2024-07-10T16:35:16Z

packages/orchestration/src/utils/zoe.js

+      const userSeat = await userSeatP;
+      atomicTransfer(zcf, srcSeat, tempSeat, give);
+      tempSeat.exit();
+      // TODO get the userSeat into baggage so it's at least recoverable


What's stopping us from doing this now?

there's no specific impediment. the TODO is because it's not done. Are you requesting that it be done before any of this lands?

I was more so curious, but if it's not a lot of effort it would be nice to see in this PR. If not, a ticket number so we remember to come back to it would be helpful

There's some design to figure out wrt subZone

0xpatrickdev · 2024-07-10T16:36:03Z

packages/orchestration/src/utils/zoe.js

+   * @param {AmountKeywordRecord} give
+   * @returns {Promise<void>}
+   */
+  const localTransfer = vowTools.retriable(


What's the best test to look at for this? sendAnywhere.test.ts should no longer have warnings about promises from async-flow?

Tangential: swapExample.test.ts has a test.skip we can hopefully restore soon.

yes, the sendAnywhere test. I'm not aware of those warnings but if the test passes then this function works

They look something like this on master and what we are trying to eliminate with #9449:

Warning for now: vow expected, not promise Promise { }

Warning for now: vow expected, not promise Promise { <pending> } (Error#1) Error#1: where warning happened at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:157:9) at eval (.../common/object-map.js:45:40) at Array.map (<anonymous>) at objectMap (.../common/object-map.js:44:20) at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:168:8) at performCall (.../async-flow/src/replay-membrane.js:196:12) at guestCallsHost (.../async-flow/src/replay-membrane.js:256:9) at In "makeEmptySeatKit" method of (zcf) [as makeEmptySeatKit] (.../async-flow/src/replay-membrane.js:525:8) at withdrawFromSeat (.../zoe/src/contractSupport/zoeHelpers.js:226:52) at sendItFn (.../orchestration/src/examples/sendAnywhere.contract.js:71:24) Warning for now: vow expected, not promise Promise { <pending> } (Error#2) Error#2: where warning happened at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:157:9) at eval (.../common/object-map.js:45:40) at Array.map (<anonymous>) at objectMap (.../common/object-map.js:44:20) at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:168:8) at performCall (.../async-flow/src/replay-membrane.js:196:12) at guestCallsHost (.../async-flow/src/replay-membrane.js:256:9) at In "makeEmptySeatKit" method of (zcf) [as makeEmptySeatKit] (.../async-flow/src/replay-membrane.js:525:8) at withdrawFromSeat (.../zoe/src/contractSupport/zoeHelpers.js:226:52) at sendItFn (.../orchestration/src/examples/sendAnywhere.contract.js:71:24) Warning for now: vow expected, not promise Promise { <pending> } (Error#3) Error#3: where warning happened at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:157:9) at eval (.../common/object-map.js:45:40) at Array.map (<anonymous>) at objectMap (.../common/object-map.js:44:20) at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:168:8) at performCall (.../async-flow/src/replay-membrane.js:196:12) at guestCallsHost (.../async-flow/src/replay-membrane.js:256:9) at In "makeEmptySeatKit" method of (zcf) [as makeEmptySeatKit] (.../async-flow/src/replay-membrane.js:525:8) at withdrawFromSeat (.../zoe/src/contractSupport/zoeHelpers.js:226:52) at sendItFn (.../orchestration/src/examples/sendAnywhere.contract.js:71:24)

But are no longer present in this PR 🎉 .

Hoping we can tighten the warning to an error, perhaps with the closing of #9449. We might also consider a parameter to asyncFlow tools to toggle the behavior while E support is being worked out.

0xpatrickdev · 2024-07-10T17:43:51Z

packages/orchestration/src/examples/sendAnywhereFlows.js

+// in guest file (the orchestration functions)
+// the second argument is all the endowments provided
+
+export const orchestrationFns = harden({


Keeping the flows in a separate file will help prevent trying to access host values directly

Agree, but putting them at the top of the file also achieves the same

the "no async" lint rules

Think you meant to say no E, but I see your point. Noting this can be achieved intra-file with something like Property[key.name=/OfferHandler$/] or Property[key.name=/Handler$/] if we enforce the convention (sendIt -> sendItHandler)

0xpatrickdev · 2024-07-10T17:45:49Z

packages/orchestration/src/utils/zoe.js

+      const userSeat = await userSeatP;
+      atomicTransfer(zcf, srcSeat, tempSeat, give);
+      tempSeat.exit();
+      // TODO get the userSeat into baggage so it's at least recoverable


I was more so curious, but if it's not a lot of effort it would be nice to see in this PR. If not, a ticket number so we remember to come back to it would be helpful

0xpatrickdev · 2024-07-10T17:53:02Z

packages/orchestration/src/utils/zoe.js

+   * @param {AmountKeywordRecord} give
+   * @returns {Promise<void>}
+   */
+  const localTransfer = vowTools.retriable(


They look something like this on master and what we are trying to eliminate with #9449:

Warning for now: vow expected, not promise Promise { }

Warning for now: vow expected, not promise Promise { <pending> } (Error#1) Error#1: where warning happened at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:157:9) at eval (.../common/object-map.js:45:40) at Array.map (<anonymous>) at objectMap (.../common/object-map.js:44:20) at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:168:8) at performCall (.../async-flow/src/replay-membrane.js:196:12) at guestCallsHost (.../async-flow/src/replay-membrane.js:256:9) at In "makeEmptySeatKit" method of (zcf) [as makeEmptySeatKit] (.../async-flow/src/replay-membrane.js:525:8) at withdrawFromSeat (.../zoe/src/contractSupport/zoeHelpers.js:226:52) at sendItFn (.../orchestration/src/examples/sendAnywhere.contract.js:71:24) Warning for now: vow expected, not promise Promise { <pending> } (Error#2) Error#2: where warning happened at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:157:9) at eval (.../common/object-map.js:45:40) at Array.map (<anonymous>) at objectMap (.../common/object-map.js:44:20) at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:168:8) at performCall (.../async-flow/src/replay-membrane.js:196:12) at guestCallsHost (.../async-flow/src/replay-membrane.js:256:9) at In "makeEmptySeatKit" method of (zcf) [as makeEmptySeatKit] (.../async-flow/src/replay-membrane.js:525:8) at withdrawFromSeat (.../zoe/src/contractSupport/zoeHelpers.js:226:52) at sendItFn (.../orchestration/src/examples/sendAnywhere.contract.js:71:24) Warning for now: vow expected, not promise Promise { <pending> } (Error#3) Error#3: where warning happened at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:157:9) at eval (.../common/object-map.js:45:40) at Array.map (<anonymous>) at objectMap (.../common/object-map.js:44:20) at tolerateHostPromiseToVow (.../async-flow/src/replay-membrane.js:168:8) at performCall (.../async-flow/src/replay-membrane.js:196:12) at guestCallsHost (.../async-flow/src/replay-membrane.js:256:9) at In "makeEmptySeatKit" method of (zcf) [as makeEmptySeatKit] (.../async-flow/src/replay-membrane.js:525:8) at withdrawFromSeat (.../zoe/src/contractSupport/zoeHelpers.js:226:52) at sendItFn (.../orchestration/src/examples/sendAnywhere.contract.js:71:24)

But are no longer present in this PR 🎉 .

Hoping we can tighten the warning to an error, perhaps with the closing of #9449. We might also consider a parameter to asyncFlow tools to toggle the behavior while E support is being worked out.

Co-Authored-By: Dean Tribble <dtribble@users.noreply.github.com>

turadg requested review from dtribble and 0xpatrickdev July 10, 2024 00:04

0xpatrickdev reviewed Jul 10, 2024

View reviewed changes

0xpatrickdev approved these changes Jul 10, 2024

View reviewed changes

turadg and others added 6 commits July 10, 2024 12:14

refactor: makeOrchestrator

800009c

Co-Authored-By: Dean Tribble <dtribble@users.noreply.github.com>

chore(types): retriable

ff94d3c

feat: ZoeTools endowment

9e61ddd

Co-Authored-By: Dean Tribble <dtribble@users.noreply.github.com>

feat: orchestrateAll

e9988c9

Co-Authored-By: Dean Tribble <dtribble@users.noreply.github.com>

refactor: use orchestrateAll

5c77188

Co-Authored-By: Dean Tribble <dtribble@users.noreply.github.com>

refactor: use localTransfer

ecb67b1

turadg force-pushed the ta/orch-factoring branch from 56c16b4 to ecb67b1 Compare July 10, 2024 19:37

turadg added the automerge:rebase Automatically rebase updates, then merge label Jul 10, 2024

mergify bot merged commit 0b6d5f3 into master Jul 10, 2024
86 checks passed

mergify bot deleted the ta/orch-factoring branch July 10, 2024 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc Orch factoring improvements #9676

misc Orch factoring improvements #9676

turadg commented Jul 10, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 10, 2024 •

edited

Loading

0xpatrickdev left a comment

0xpatrickdev Jul 10, 2024

turadg Jul 10, 2024 •

edited

Loading

0xpatrickdev Jul 10, 2024

turadg Jul 10, 2024

0xpatrickdev Jul 10, 2024

0xpatrickdev Jul 10, 2024

turadg Jul 10, 2024

0xpatrickdev Jul 10, 2024

turadg Jul 10, 2024

0xpatrickdev Jul 10, 2024

turadg Jul 10, 2024

0xpatrickdev Jul 10, 2024

turadg Jul 10, 2024

0xpatrickdev Jul 10, 2024 •

edited

Loading

0xpatrickdev Jul 10, 2024

0xpatrickdev Jul 10, 2024

0xpatrickdev Jul 10, 2024 •

edited

Loading

misc Orch factoring improvements #9676

misc Orch factoring improvements #9676

Conversation

turadg commented Jul 10, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Jul 10, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

0xpatrickdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

turadg commented Jul 10, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 10, 2024 •

edited

Loading

turadg Jul 10, 2024 •

edited

Loading

0xpatrickdev Jul 10, 2024 •

edited

Loading

0xpatrickdev Jul 10, 2024 •

edited

Loading