chore(orchestration): endowments are membrane-friendly #9591

0xpatrickdev · 2024-06-26T16:58:33Z

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 offerResults that are Vows. 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.

cloudflare-workers-and-pages · 2024-06-26T19:39:04Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`100e0a3`
Status:	✅ Deploy successful!
Preview URL:	https://24abe6ef.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-local-orch-account-vows.agoric-sdk.pages.dev

View logs

0xpatrickdev · 2024-06-27T01:33:19Z

packages/boot/tools/supports.ts

+              switch (message['@type']) {
+                case '/cosmos.staking.v1beta1.MsgDelegate': {
+                  if (message.amount.amount === '504') {
+                    throw Error('simulated packet timeout');


What's the best way to communicate a failure here? (bridge in bootstrap test)

packages/orchestration/src/exos/local-orchestration-account.js

0xpatrickdev · 2024-06-27T01:41:20Z

packages/boot/test/bootstrapTests/lca.test.ts

+      // FIXME should receive "simulated packet timeout"
+      message:
+        /syscall.callNow failed: device.invoke failed, see logs for details/,
+    },


This newly added test, despite the broken error propagation through runUtils, passes on current master (but is failing here)

Temporarily fixed via d77582b

dtribble

Excellent work. There was one or two issues that I think really need fixing (and some comments here that should be for later work).

dtribble · 2024-06-27T23:30:19Z

packages/orchestration/src/exos/cosmos-orchestration-account.js

+    NetworkShape.Vow$(M.arrayOf(ChainAmountShape)),
+  ),
+  withdrawRewards: M.call().returns(
+    NetworkShape.Vow$(M.arrayOf(ChainAmountShape)),


Use of NetworkShape.Vow$ seems odd, given that there is also VowShape. Can VowShape have an optional argument, e.g., VowShape(NetworkShape.... That's pretty clunky, but then there's just one way to express.

Is there any doc on what Vow$ is?

I created this ticket to clean up interface guards for watchers: #9611

TLDR; agree we should be importing something less verbose from @agoric/vow. We also might just go with VowShape instead of Vow$ since the unwrapped shape will likely never be reached. I can't find a link atm but this topic came up in a recent PR review cc @turadg

dtribble · 2024-06-27T23:31:42Z

packages/orchestration/src/exos/cosmos-orchestration-account.js

@@ -16,9 +16,10 @@ import {
 } from '@agoric/cosmic-proto/cosmos/staking/v1beta1/tx.js';
 import { Any } from '@agoric/cosmic-proto/google/protobuf/any.js';
 import { makeTracer } from '@agoric/internal';
+import { Shape as NetworkShape } from '@agoric/network';


Having to rename Shape seems bad. network should not export just Shape.

dtribble · 2024-06-27T23:33:35Z

packages/orchestration/src/exos/cosmos-orchestration-account.js

@@ -129,7 +130,7 @@ export const prepareCosmosOrchestrationAccountKit = (
      undelegateWatcher: M.interface('undelegateWatcher', {
        onFulfilled: M.call(M.string())
          .optional(M.arrayOf(M.undefined())) // empty context
-          .returns(M.promise()),
+          .returns(NetworkShape.Vow$(M.promise())),


I cannot parse this pattern at all.

dtribble · 2024-06-27T23:35:38Z

packages/orchestration/src/exos/cosmos-orchestration-account.js

-        Delegate: M.callWhen(ChainAddressShape, AmountArgShape).returns(
-          InvitationShape,
+        Delegate: M.call(ChainAddressShape, AmountArgShape).returns(
+          M.promise(),


shouldn't this be explicitly a promise for void?

Does the promise pattern include Vows? Otherwise shouldn't this be a Vow?

For the invitationMakers interfaces, the return promise unwraps to an InvitationShape, not a VowShape. The offerHandler of the Invitation is what needs to be a Vow.

shouldn't this be explicitly a promise for void?

That's not expressible as a pattern. It's not pass-invariant.

(Nit: in typescript void does not signify undefined but rather a kind of don't care. I'll ignore that below.)

In is indeed not expressible as a pattern, but in this case not because of pass invariance. Patterns (as opposed to guards) can only enforce what they can immediately check, and a pattern cannot immediately check what the promise will eventually fulfill to.

The original, by using an M.callWhen method guard with a returns(InvitationShape) does enforce that Delegate returns a promise for something that matches InvitationShape, because it uses the callWhen delay to validate the promise's fulfillment before passing it through. Likewise, within an M.callWhen you can say M.await(FooShape) as a parameter guard. If the argument is a promise, this will enforce that the promise fulfills to something that matches FooShape before passing that fulfillment along as the argument to the raw method.

dtribble · 2024-06-27T23:41:17Z

packages/orchestration/src/exos/cosmos-orchestration-account.js

-        async getBalances() {
-          throw Error('not yet implemented');
+        getBalances() {
+          return asVow(() => Fail`not yet implemented`);


This should have a reference to the associated bug to implement it, and probably TODO in the error message?

Good idea, done: #9610

dtribble · 2024-06-28T00:08:22Z

packages/orchestration/src/exos/orchestrator.js

@@ -111,27 +112,23 @@ export const prepareOrchestratorKit = (
        },
      },
      orchestrator: {
-        /** @type {Orchestrator['getChain']} */
+        /** @type {PromiseToVow<Orchestrator['getChain']>} */


Too much more and we are gonna see renamed to P2V :)

dtribble · 2024-06-28T00:08:59Z

packages/orchestration/src/exos/orchestrator.js

-                chainHub.getChainInfo('agoric'),
-                this.facets.makeLocalChainFacadeWatcher,
-              ),
+            // @ts-expect-error Type 'Vow<Voidless>' is not assignable to type 'Vow<Chain<any>>'.


Should there be a TODO/bug to fix these types?

Yes, I added cb5f043 which clearly spells out the compromises I made on types. I suspect the PromiseVow added here doesn't get us all the way with generics needed for getChain.

For the ticket, I am attributing it is part of the remaining work for #9449.

dtribble · 2024-06-28T00:11:41Z

packages/boot/test/bootstrapTests/lca.test.ts

+  });
+
+  await t.throwsAsync(
+    wd.executeOffer({


what is the expected error and why? Seems like somethign we shoudl check the details of, given that there are so many layers that could fail

The expected error was commented out at the end of the function (this is the typical place to assert error message values in an ava test).

Inspection of the error message is disabled due to this: https://github.com/Agoric/agoric-sdk/pull/9591/files#r1655705785 - still not quite sure the past way to propagate an error through this bridge device.

The test still serves value since the rejection is properly propagated - just the message is missing.

I have confidence about the layering via tests in bootstrapTests/orchestration.test.ts, which show some "ABCI code: 5: error handling packet: see events for details", as well as unit tests for local-orch-acct.

dtribble · 2024-06-28T00:16:43Z

packages/orchestration/src/exos/local-orchestration-account.js

            E(this.state.account).getBalance(brand),
+            this.facets.getBalanceWatcher,
+            { denom },


any particular reason why you pass denom in an embedded record rather than as a sole argument? Consistency? I check reflexively because it is more allocations :)

but perhaps clearer

Context is a single argument, so I’ve been treating it like an opts bag for consistency/readability.

This is good feedback, though. These changes worth considering as we get closer to completion cc @turadg

... These changes worth considering as we get closer to completion ...

I hope any plans to re-consider code we've written are represented by open issues.

Not worth a ticket so updated here: 69a6e21

dtribble · 2024-06-28T00:19:48Z

packages/orchestration/src/exos/local-orchestration-account.js

        },
        getBalances() {
-          throw new Error('not yet implemented');
+          return asVow(() => Fail`not yet implemented`);


Isn't there a helper already (as with Promise.reject) for making and already-rejected vow?

Not to my knowledge. I think we could also do :

watch(Promise.reject(Fail`...`))

But asVow seems concise enough to me here

dckc

time for a meeting; sharing what I've got so far

dckc · 2024-06-28T16:59:45Z

.eslintrc.cjs

+  {
+    selector: "Identifier[name='heapVowE']",
+    message:
+      'heapVowE shortens vows to promises; instead use `E` from `@endo/far`',


should be "instead use E with watch()"?

dckc · 2024-06-28T18:14:18Z

packages/orchestration/src/internal.ts

@@ -0,0 +1,5 @@
+import type { Vow } from '@agoric/vow';
+
+export type PromiseToVow<T> = T extends (...args: infer A) => Promise<infer R>


I'd like to see docs for PromiseToVow and type tests - at least 1 positive and 1 negative.

The name seems long. Maybe it doesn't need to appear in many places?

Please see tests added here: 2dcc26f. As always, thanks for careful review. Something was missed in the original implementation.

"Missed" is maybe harsh. It would return never if the target was not a Promise. The caller shouldn't be using it in the first place.

Now, it passes through all values and only transforms Promises to Vows.

dckc · 2024-06-28T18:20:17Z

packages/orchestration/src/exos/local-orchestration-account.js

  IBCTransferOptionsShape,
 } from '../typeGuards.js';


aside: we call them ThingShape but put them in files called typeGuards. In E, they were called Guards. (likewise guards in Monte).

Trying it out just to look at it: DenomGuard vs. DenomShape. Hm. Not much of an improvement.

But our dev audience might already be familiar with the notion by the name "guard":

The term is used with specific meaning in APL, Haskell, Clean, Erlang, occam, Promela, OCaml, Swift,[2] Python from version 3.10, and Scala programming languages.
-- Guard (computer science) - Wikipedia

dckc · 2024-06-28T18:21:45Z

packages/orchestration/src/exos/local-orchestration-account.js

-  executeTx: M.callWhen(M.arrayOf(M.record())).returns(M.arrayOf(M.record())),
+  delegate: M.call(M.string(), AmountShape).returns(VowShape),
+  undelegate: M.call(M.string(), AmountShape).returns(VowShape),
+  withdraw: M.call(AmountShape).returns(NetworkShape.Vow$(PaymentShape)),


Network is a distraction here.

I suggest doing something like const { Vow$ } = NetworkShape; above.

dckc · 2024-06-28T18:33:37Z

packages/orchestration/src/exos/local-orchestration-account.js

+         *   JsonSafe<
+         *     TypedJson<'/cosmos.staking.v1beta1.MsgUndelegateResponse'>


note to self: is this distinction also expressed in a test?

dckc · 2024-06-28T18:46:37Z

packages/orchestration/src/exos/local-orchestration-account.js

+                delegatorAddress: this.state.address.address,
+              }),
+            ]),
+            this.facets.extractFirstResultWatcher,


Is including Watcher in the name worthwhile? It's starting to feel like extractFirstResultWatcherBehaviorDefinitionCodeSpecification.

I agree its verbose. This seems to be the pattern adopted elsewhere, so I ran with it. Technically it's a "watcher handler"

dckc · 2024-06-28T18:48:44Z

packages/orchestration/src/exos/local-orchestration-account.js

        },
-        /** @type {OrchestrationAccount<any>['transferSteps']} */
+        /** @type {PromiseToVow<OrchestrationAccount<any>['transferSteps']>} */


This usage is a good basis for a type test; that is: in the test, use these names as motivation.

dckc · 2024-06-28T18:49:14Z

packages/orchestration/src/utils/orchestrationAccount.js

+  getBalance: M.call(M.any()).returns(NetworkShape.Vow$(CoinShape)),
+  getBalances: M.call().returns(NetworkShape.Vow$(M.arrayOf(CoinShape))),


again, Network is a distraction here

dckc · 2024-06-28T18:51:22Z

packages/orchestration/src/exos/local-orchestration-account.js

-            return this.facets.holder.delegate(validatorAddress, ertpAmount);
+            return watch(


note to self: is a test for this change included in this PR?

If not, what's our plan for testing it?

the 1st line of the commit message exceeds conventional norms

I suggest changing feat(orchestration): to chore:. The feature that I would expect to see in release notes is local-orchestration-account.

dckc · 2024-06-28T18:56:08Z

packages/orchestration/src/exos/local-chain-facade.js

@@ -54,8 +54,8 @@ export const prepareLocalChainFacade = (
      makeAccount() {
        const { localChainInfo } = this.state;
        const lcaP = E(localchain).makeAccount();
-        // FIXME use watch() from vowTools
-        return heapVowTools.watch(allVows([lcaP, E(lcaP).getAddress()]), {
+        // @ts-expect-error E does not understand Vow pipelining


That seems like a devex issue that our customers may encounter.

IOU an issue. Bonus points if you beat me to it.
cc @Jovonni @mitdralla

E does not understand Vow pipelining

I thought this was the case, but it doesn't seem to be. I was tricked by the linter at one point and this comment is not in the final revision. I can try to squash it out of the commit, but not sure it's worth the effort

On further analysis, the original comment does appear to be true. In this particular instance, E(localchain).makeAccount is a Promise, not a Vow, so pipelining works.

Here's a test that demonstrates the behavior: d7bf05d

- allows us to reference the idealized API which typically returns Promises in code that returns Vows

…s are resumable

…outTimestamp` is provided

…m shared `orchestrationAccountMethods` are resumable

- adds VowifyAll helper for satisfies types test

* Make smart wallet use `when` to accept a Vow for the result of an offer NOTE: this enables upgrade of the contract, NOT upgrade of the smart-wallet refs: #9308 Co-authored-by: Dean Tribble <tribble@agoric.com>

adds tests that demonstrate: 1. allVows supporting pipelining if the operand is a Promise 2. allVows (original E?) does not support pipelining if the operand is a Vow

0xpatrickdev commented Jun 27, 2024

View reviewed changes

packages/orchestration/src/exos/local-orchestration-account.js Show resolved Hide resolved

0xpatrickdev commented Jun 27, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/local-orch-account-vows branch 3 times, most recently from b09aaf9 to fedb393 Compare June 27, 2024 20:53

0xpatrickdev mentioned this pull request Jun 27, 2024

add support for Vows to smart-wallet #9308

Closed

0xpatrickdev marked this pull request as ready for review June 27, 2024 22:20

dtribble approved these changes Jun 28, 2024

View reviewed changes

0xpatrickdev added the force:integration Force integration tests to run on PR label Jun 28, 2024

0xpatrickdev force-pushed the pc/local-orch-account-vows branch 2 times, most recently from cb5f043 to 4149dd8 Compare June 28, 2024 16:33

0xpatrickdev mentioned this pull request Jun 28, 2024

orchestration facade returns promises backed by vows #9449

Closed

dckc reviewed Jun 28, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/local-orch-account-vows branch from 4149dd8 to 2dcc26f Compare June 28, 2024 21:25

0xpatrickdev removed the force:integration Force integration tests to run on PR label Jun 28, 2024

0xpatrickdev force-pushed the pc/local-orch-account-vows branch from 69a6e21 to 2bece3a Compare June 29, 2024 01:55

0xpatrickdev changed the title ~~feat(orchestration): endowments are membrane-friendly~~ chore(orchestration): endowments are membrane-friendly Jun 29, 2024

0xpatrickdev added the force:integration Force integration tests to run on PR label Jun 29, 2024

dtribble added the automerge:rebase Automatically rebase updates, then merge label Jun 30, 2024

0xpatrickdev added 9 commits June 30, 2024 02:10

lint: warn when importing heapVowE in resumable code

f76fb10

types(orchestration): PromiseToVow helper

9404b43

- allows us to reference the idealized API which typically returns Promises in code that returns Vows

chore(orchestration): ensure local-orchestration-account is resumable

e8bbaec

chore(orchestration): ensure local-orchestration-account offerHandler…

042766f

…s are resumable

fix(orchestration): do not call getTimeoutTimestampNS if `opts.time…

c162426

…outTimestamp` is provided

chore(orchestration): ensure cosmos-orchestration-account methods fro…

ef193ac

…m shared `orchestrationAccountMethods` are resumable

chore(orchestration): ensure chain facades are membrane friendly

c9263dd

test(boot): local orch account offer handler rejection

7e3fdfe

chore: cosmos-orchestration-account is resumable

fc2f5ea

- adds VowifyAll helper for satisfies types test

0xpatrickdev and others added 10 commits June 30, 2024 02:10

chore(orchestration): orchestration service is resumable

612c0ff

chore(orchestration): orchestrator is resumable

166c6b9

feat(smart-wallet): accept Vow for offerResult

933ab29

* Make smart wallet use `when` to accept a Vow for the result of an offer NOTE: this enables upgrade of the contract, NOT upgrade of the smart-wallet refs: #9308 Co-authored-by: Dean Tribble <tribble@agoric.com>

fix(orchestration): disable type casts in skipped boot tests

6f6b891

lint: tighten resumable restricted syntax to error

26d4508

chore(orchestration): document TODOs with ticket numbers

3660944

test(vow): allVows pipelining behavior

d7bf05d

adds tests that demonstrate: 1. allVows supporting pipelining if the operand is a Promise 2. allVows (original E?) does not support pipelining if the operand is a Vow

chore(orchestration): reduce noise around Vow$ shape

ff47e1c

test(types): PromiseToVow behavior

194ac48

chore: limit unnecessary allocation to watcher ctx

100e0a3

0xpatrickdev force-pushed the pc/local-orch-account-vows branch from 2bece3a to 100e0a3 Compare June 30, 2024 02:10

mergify bot merged commit efd3370 into master Jun 30, 2024
74 checks passed

mergify bot deleted the pc/local-orch-account-vows branch June 30, 2024 02:26

This was referenced Jul 1, 2024

feat(orchestration): return vows #9530

Closed

feat(undelegate): use Timestamp instead of Date #9646

Merged

0xpatrickdev mentioned this pull request Jul 11, 2024

9449 membrane types #9685

Merged

0xpatrickdev mentioned this pull request Jul 25, 2024

upgrade testing supports #9770

Merged

		@@ -0,0 +1,5 @@
		import type { Vow } from '@agoric/vow';

		export type PromiseToVow<T> = T extends (...args: infer A) => Promise<infer R>

		* JsonSafe<
		* TypedJson<'/cosmos.staking.v1beta1.MsgUndelegateResponse'>

		getBalance: M.call(M.any()).returns(NetworkShape.Vow$(CoinShape)),
		getBalances: M.call().returns(NetworkShape.Vow$(M.arrayOf(CoinShape))),

		return this.facets.holder.delegate(validatorAddress, ertpAmount);
		return watch(

chore(orchestration): endowments are membrane-friendly #9591

chore(orchestration): endowments are membrane-friendly #9591

Conversation

0xpatrickdev commented Jun 26, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Jun 26, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

0xpatrickdev Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

0xpatrickdev Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

0xpatrickdev Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

dtribble left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a 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

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 Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

0xpatrickdev Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

0xpatrickdev commented Jun 26, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jun 26, 2024 •

edited

Loading

0xpatrickdev Jun 27, 2024 •

edited

Loading

0xpatrickdev Jun 27, 2024 •

edited

Loading

0xpatrickdev Jun 27, 2024 •

edited

Loading

0xpatrickdev Jun 28, 2024 •

edited

Loading

0xpatrickdev Jun 28, 2024 •

edited

Loading