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

orchestration facade returns promises backed by vows #9449

Closed
14 tasks done
Tracked by #9281
turadg opened this issue Jun 4, 2024 · 5 comments · Fixed by #9685
Closed
14 tasks done
Tracked by #9281

orchestration facade returns promises backed by vows #9449

turadg opened this issue Jun 4, 2024 · 5 comments · Fixed by #9685
Assignees
Labels
asyncFlow related to membrane-based replay and upgrade of async functions

Comments

@turadg
Copy link
Member

turadg commented Jun 4, 2024

For each of these:

  • no async functions
  • no callWhen guards
  • methods that don't return immediately return Vows
  • methods that return vows don't throw

Files to convert

@dckc
Copy link
Member

dckc commented Jun 12, 2024

@erights and I went over the client code in sendAnywhere.contract.js. We found a few things and sketched fixes...

diff --git a/packages/orchestration/src/examples/sendAnywhere.contract.js b/packages/orchestration/src/examples/sendAnywhere.contract.js
index ed49472ce..d37ae1a49 100644
--- a/packages/orchestration/src/examples/sendAnywhere.contract.js
+++ b/packages/orchestration/src/examples/sendAnywhere.contract.js
@@ -71,7 +71,7 @@ export const start = async (zcf, privateArgs, baggage) => {
     ...orchPowers,
   });
 
-  let contractAccount;
+  const accountCell = {};
 
   const findBrandInVBank = async brand => {
     const assets = await E(
@@ -85,9 +85,9 @@ export const start = async (zcf, privateArgs, baggage) => {
   /** @type {OfferHandler} */
   const sendIt = orchestrate(
     'sendIt',
-    { zcf },
+    { zcf, findBrandInVBank, accountCell },
     // eslint-disable-next-line no-shadow -- this `zcf` is enclosed in a membrane
-    async (orch, { zcf }, seat, offerArgs) => {
+    async (orch, { zcf, findBrandInVBank, accountCell }, seat, offerArgs) => {
       mustMatch(
         offerArgs,
         harden({ chainName: M.scalar(), destAddr: M.string() }),
@@ -98,10 +98,11 @@ export const start = async (zcf, privateArgs, baggage) => {
       const { denom } = await findBrandInVBank(amt.brand);
       const chain = await orch.getChain(chainName);
 
-      // XXX ok to use a heap var crossing the membrane scope this way?
-      if (!contractAccount) {
+      let contractAccount = accountCell.account;
+      if (contractAccount === undefined) {
         const agoricChain = await orch.getChain('agoric');
         contractAccount = await agoricChain.makeAccount();
+        accountCell.account = contractAccount;
       }
 
       const info = await chain.getChainInfo();

@mhofman
Copy link
Member

mhofman commented Jun 12, 2024

+        accountCell.account = contractAccount;

The arguments must be durable compatible, which implies hardening. Mutating arguments like this is not supported, you'd need a method to get/set that internal state

@erights
Copy link
Member

erights commented Jun 12, 2024

you'd need a method to get/set that internal state

Yeah, after the call with @dckc, I think a “slight” rewrite of the cell puts it in a category similar to the exo state object:

let contractAcctInternal;
const accountCell = harden({
  get account() { return contractAcctInternal; },
  set account(newValue) { contractAcctInternal = newValue; },
});

This can be made less awkward with a helper. It is only similar to the state object in that the state object need additional mechanisms. But this form of endowment does not need any further mechanism than what we’d need to do to support state objects.

Note that this is only intended to go through the endowments path, like functions and zcf, that may not themselves be Passable. The endowment mechanism has to wrap them with durables on the host side, and with unwrapping emulators on the guest side.

@0xpatrickdev 0xpatrickdev self-assigned this Jun 17, 2024
@aj-agoric aj-agoric assigned dtribble and unassigned iomekam and 0xpatrickdev Jun 17, 2024
0xpatrickdev added a commit that referenced this issue Jun 24, 2024
- towards #9449
- deletes tests where paths are covered by other tests
- skips tests until functionality is tested elsewhere. see #9572 which blocks easily porting these to unit tests in @agoric/orchestration
mergify bot added a commit that referenced this issue Jun 25, 2024
refs: #9449

## Description

Disables `@agoric/orchestration` tests in `packages/boot` that interact directly with Vows. _`EV` does not currently support vows, and these tests block efforts related to #9449._

For tests that are skipped and not deleted, I propose testing these paths after the completion of #9572. 

### Testing Considerations

Skips tests instead deleting them. Taking on some tech debt until #9572
mergify bot added a commit that referenced this issue Jun 25, 2024
refs: #9449

## Description

- Add `asVow` helper to `VowTools` to ensure we always return vows, even in the event of early synchronous errors.
- Implement the helper in `ChainAccountKit` against #9562
mergify bot added a commit that referenced this issue Jun 25, 2024
refs: #9449

## Description

 Perform remote calls to agoricNames in membrane-friendly way. This is an interim approach until #9541, #9322, or #9519.
mergify bot pushed a commit that referenced this issue Jun 25, 2024
closes: #XXXX
refs: #9449 #9521 #9304 #9281

## Description

Changed async-flow to support endowments. Changed `orchestrate` to use `asyncFlow` with endowments. Changed `sendAnywhere` example orchestration contract to be more compatible with this new `orchestrate`.

The CI errors are all in the `orchestration` package. After some earlier iteration where orchestration failures indicated async-flow bugs, which I fixed, the remaining errors seem plausibly to be integration bugs on the orchestration side revealed by using this improved `orchestrate` function. If so, that satisfies the purpose of this PR -- to enable integration testing to reveal such errors. However, this leaves open the question of how to bring this PR to fruition despite these CI errors.

In that iteration, the majority of errors were due to host-side promises, which we expected. To proceed with integration testing, I temporarily turned that case into a warning, by wrapping the host-side promise with a host-side vow. This stopgap measure is obviously fragile under upgrade. It would cause may upgrades to fail

However, I have not investigated these CI errors enough to be at all confident that none of them are due to bugs in async-flow. For any of those, they should be fixed in this PR.

### Security Considerations
nothing new
### Scaling Considerations
none, given that total endowments are low cardinality. All these endowments are prepare-time per-function. There should not be any cardinality limit on the activations making use of these endowments. But like all other async-flow scaling issues, that remains to be tested.
### Documentation Considerations
The endowment rules and taxonomy is interesting, and should be documented.
### Testing Considerations
We get CI errors only from the `orchestration` package. Some of these may be the integration bugs we wanted this exercise to reveal. However, others may be async-flow bugs, which should have been caught by async-flow unit tests.

The warning stopgap I mentioned above [appears in CI](https://github.com/Agoric/agoric-sdk/actions/runs/9637015639/job/26575694851?pr=9566#step:12:648) as, for example
```
Warning for now: vow expected, not promise Promise { <pending> } (Error#1)
Error#1: where warning happened
  at makeGuestForHostVow (.../async-flow/src/replay-membrane.js:329:9)
  at eval (.../async-flow/src/convert.js:119:10)
  at innerConvert (.../async-flow/src/convert.js:63:8)
  at convertRecur (.../async-flow/src/convert.js:30:8)
  at convert (.../async-flow/src/convert.js:76:1)
  at performCall (.../async-flow/src/replay-membrane.js:137:1)
  at guestCallsHost (.../async-flow/src/replay-membrane.js:195:9)
  at In "getChain" method of (Orchestrator orchestrator) [as getChain] (.../async-flow/src/replay-membrane.js:282:8)
  at eval (.../orchestration/src/examples/unbondExample.contract.js:60:23)
  at eval (.../async-flow/src/async-flow.js:222:1)
  at Object.restart (.../async-flow/src/async-flow.js:222:30)
  at makeAsyncFlowKit (.../async-flow/src/async-flow.js:430:6)
  at asyncFlow_hostFlow (.../async-flow/src/async-flow.js:448:13)
  at orcFn (.../orchestration/src/facade.js:124:15)
  at eval (.../pass-style/src/make-far.js:224:31)
```

The relevant lines are
```
  at In "getChain" method of (Orchestrator orchestrator) [as getChain] (.../async-flow/src/replay-membrane.js:282:8)
  at eval (.../orchestration/src/examples/unbondExample.contract.js:60:23)
```
where the first line indicates what method or method guard provided the inappropriate promise
```js
  getChain: M.callWhen(M.string()).returns(ChainInfoShape),
```

and the second line indicates where the guest code called it
```js
const omni = await orch.getChain('omniflixhub');
```

### Upgrade Considerations

The orchestration code in question cannot be truly upgrade safe until we see no more of these "vow expected, not promise" warnings. Even then, we should expect that async-flow as of this PR is ready for lots of testing, but not yet ready to run on the main chain with durable state expected to survive real upgrades.
mergify bot added a commit that referenced this issue Jun 26, 2024
refs: #9449

## Description

- More returning of vows, using `asVow` helper
- Adjusts **resumable** custom lint rules to ignore `onOpen` and `onClose` when they are properties of `connectionHandler`
- removes unused `onReceive` handler or connectionHandler for `chainAccountKit` and `icqConnectionKit`

### Security Considerations
none
### Scaling Considerations
none

### Documentation Considerations
none

### Testing Considerations
CI for now

### Upgrade Considerations
not yet deployed
@0xpatrickdev
Copy link
Member

0xpatrickdev commented Jun 28, 2024

With #9591, it seems the only remaining items are:

  • TS Types in the Facades. See 4149dd8 for more details
  • Timer Utils - good candidate for retriable
  • withdrawFromSeat - not idempotent (i.e. not retriable) - how should we handle this?
  • chainHub - currently uses heapVowTools

mergify bot added a commit that referenced this issue Jun 30, 2024
closes: #XXXX
refs: #9449
refs: #9308

## Description

Changes in this PR are related to #9449, primarily for `local-orchestration-account.js` and `cosmos-orchestration-account.js`, `service.js`, and `orchestrator.js`. Attenuated versions of these kit are what the caller of `...getChain('agoric' | 'cosmos').makeAccount()` receives. 

This PR also includes a change to `smart-wallet` so it understands how to unwrap **offerResult**s that are **Vow**s. This  uses **heapVowTools** which means it does  _**not**_  cover resumability in the scenario of `smart-wallet` upgrading - only the contracts that rely on it.

Other small changes included changes:
- `PromiseToVow` and `VowifyAll` type helpers, so we can still reference the idealized API spec in code that returns Vows
- fix/refactor of `getTimeoutTimestampNS` logic that removes an unnecessary network call when the caller supplies values
- lint rule: adds a warning when importing `heapVowE` in resumable code
- resumable lint rule changes to **error**, now that all warnings are resolved

### Security Considerations
N/A

### Scaling Considerations
N/A

### Documentation Considerations
N/A

### Testing Considerations

Adds a test boot and unit tests for a rejected Delegate wallet offer, which is helpful for ensuring errors are properly propagated through the vow chain.

Upgrade tests would be helpful here, but we haven't started on these yet. I don't think we need those to land this PR, although they are certainly critical before release. Would love to pair with someone on this and learn more about how we do that. 

### Upgrade Considerations

This PR contains a change to **smart-wallet**.
gibson042 pushed a commit that referenced this issue Jul 2, 2024
refs: #9449

## Description

- Add `asVow` helper to `VowTools` to ensure we always return vows, even in the event of early synchronous errors.
- Implement the helper in `ChainAccountKit` against #9562
mergify bot added a commit that referenced this issue Jul 9, 2024
refs: #9449

## Description

This makes ChainHub return durable vows. The underlying lookup functions are not durable but they made so by the `retriable` helper.

So this also introduces a stub version of #9541 pulled from @dtribble 's #9657

### Security Considerations
none

### Scaling Considerations
none

### Documentation Considerations
none

### Testing Considerations
CI

### Upgrade Considerations
not yet upgradable. Communicated by https://github.com/Agoric/agoric-sdk/blob/a267ba20db0eeb2082c287ca7b69e37d6f272eb9/packages/vow/src/tools.js#L37-L38
@turadg turadg assigned turadg and unassigned 0xpatrickdev Jul 10, 2024
mergify bot added a commit that referenced this issue Jul 10, 2024
refs: #9449

## 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 #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
@mergify mergify bot closed this as completed in 3915e4c Jul 11, 2024
@mergify mergify bot closed this as completed in #9685 Jul 11, 2024
This was referenced Jul 12, 2024
mergify bot added a commit that referenced this issue Jul 14, 2024
follow up on #9449 

## Description
#9685 unwrapped an `asVow()` to get the promise failure to propagate. This reverts that now that #9704 removed the need for it.

### Security Considerations
none

### Scaling Considerations
none

### Documentation Considerations
none

### Testing Considerations
CI

### Upgrade Considerations
not yet deployed
mergify bot added a commit that referenced this issue Jul 16, 2024
follow up on #9449 

## Description

- removes the temporary accommodation in which `orchestrate` returns a Promise instead of a Vow.
- removes `holder` from ResolvedContinuingInvitation b/c the `invitationMakers` provide the ocaps
- fixes `orchestrate` typedef to infer the return type from its arguments


### Security Considerations
none

### Scaling Considerations
none

### Documentation Considerations
none

### Testing Considerations
CI


### Upgrade Considerations
not yet deployed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
asyncFlow related to membrane-based replay and upgrade of async functions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants