9449 membrane types by turadg · Pull Request #9685 · Agoric/agoric-sdk

turadg · 2024-07-10T21:28:40Z

closes: #9449

Description

Address all the FIXMEs about types and vows in Orchestration. To do so I moved some stuff down to async-flow and zoe packages. I also made some small fixes in smart-wallet.

I believe this ticks the last box of #9449.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

This adds type tests and I used type-coverage to verify no regressions.

Upgrade Considerations

orchestration and async-flow not yet deployed.

The changes in zoe and smart-wallet are just typedefs so safe to deploy anytime.

cloudflare-workers-and-pages · 2024-07-10T21:28:42Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`6b41829`
Status:	✅ Deploy successful!
Preview URL:	https://c7aaa4c8.agoric-sdk.pages.dev
Branch Preview URL:	https://9449-membrane-vows.agoric-sdk.pages.dev

View logs

erights

Only reviewed asyncFlow changes so far. More soon...

erights · 2024-07-11T01:39:27Z

packages/async-flow/src/replay-membrane.js

@@ -154,14 +154,9 @@ export const makeReplayMembrane = ({
    const passStyle = passStyleOf(h);


I love that we can now stop tolerating host promises and instead reject them with errors!

However, this leaves the doc-comment above and the function name tolerate* stale. Please revise to be consistent either with consistent rejection, or at least as the function which decides whether to tolerate or to reject. I don't have a name to suggest.

Ok to fully remove tolerateHostPromiseToVow? I was trying to be surgical because I didn't know if you needed it to be there.

Removal is good

erights · 2024-07-11T01:43:50Z

packages/async-flow/src/types.d.ts

+export type GuestOf<F extends HostAsyncFuncWrapper> = F extends (
+  ...args: infer A
+) => Vow<infer R>
+  ? (...args: A) => Promise<R>
+  : F;


Wow!

Thanks for translating all those type declarations into better .d.ts types.

tsc did the work! yarn prepack

TIL! Good to know.

packages/async-flow/src/types.d.ts

0xpatrickdev

Nice work 👏

async-flow/src/types.d.ts#L118-L252 is foreign to me but seems like its mostly porting things from js to ts

0xpatrickdev · 2024-07-11T01:41:00Z

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

+            // XXX makeAccount returns a Promise for an exo but reserves being able to return a vow
+            // so we use heapVowE to shorten the promise path
+            // eslint-disable-next-line no-restricted-syntax -- will run in one turn
+            allVows([lcaP, heapVowE(lcaP).getAddress()]),


I'm surprised we need to reach for heapVowE here. Original E + pipelining should work, since lcaP is a Promise and not a Vow. There's a test demonstrating the behavior mentioned in: #9591 (comment)

~~Maybe we need to improve the allVows type?~~ ah E doesn't understand PromiseVow. Still, seems like it's maybe a type issue and we shouldn't need heapVowE

I tried to explain this in the comment. The value actually being returned is not a vow, but the return signature says it could be. @michaelfig wanted to reserve that option for the future. So this code should comply. (i.e. not to accelerate Hyrum's Law).

That makes sense, thanks

0xpatrickdev · 2024-07-11T01:57:49Z

packages/orchestration/src/orchestration-api.ts

   * in the {@link CurrentWalletRecord} in vstorage.
   */
-  getPublicTopics: () => Promise<TopicsRecord>;
+  getPublicTopics: () => Promise<Record<string, ResolvedPublicTopic<unknown>>>;


Nice. Consider exporting a ResolvedTopicsRecord from zoe/contractSupport/topics.js

packages/async-flow/src/types.d.ts

0xpatrickdev · 2024-07-11T02:12:02Z

packages/async-flow/src/types.d.ts

+  | 'Done';
+
+/**
+ * `T` defaults to `any`, not `Passable`, because unwrapped guests include


Realize this is an existing comment, but consider an @internal directive

when I added it there was a jsdoc lint error the tag should be empty

What does @internal mean/do?

https://typedoc.org/tags/internal/

#9690 fixes the lint error. I'm still not sure whether this should be @internal.

0xpatrickdev · 2024-07-11T02:14:20Z

packages/async-flow/src/types.d.ts

+type HostInterface<T> = {
+  [K in keyof T]: HostOf<T[K]>;
+};
+


Nice work on these. Do we also need a GuestInterface?

When we define an interface it should be in the view of the guest.

0xpatrickdev · 2024-07-11T02:24:51Z

packages/async-flow/src/replay-membrane.js

-        return watch(h);
+        const e = Error('where promise detected');
+        console.error('vow expected, not promise', h, e);
+        throw e;


0xpatrickdev · 2024-07-11T02:28:08Z

packages/zoe/src/contractSupport/topics.js

 */

+/**
+ * A {PublicTopic} in which the `storagePath` is a string.


Maybe add a why to one of the Resolved* types

Suggested change

* A {PublicTopic} in which the `storagePath` is a string.

* A {PublicTopic} in which the `storagePath` is a string.

* Useful when working with Vows and async-flow.

good idea. we can use more "what this is for"

packages/smart-wallet/src/types.d.ts

erights · 2024-07-11T17:00:49Z

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

 });

-/** @type {{ [name: string]: [description: string, valueShape: Pattern] }} */
+/** @type {{ [name: string]: [description: string, valueShape: Matcher] }} */


Why change the type from Pattern to Matcher? Do you intend to exclude other patterns that are not themselves matchers, even if they contain matchers?

Likewise for other similar changes

Why change the type from Pattern to Matcher?

because makeRecordKit accepts only a Matcher for the valueShape:

agoric-sdk/packages/zoe/src/contractSupport/recorder.js

Lines 148 to 149 in 4d54c56

* @param {TypedMatcher<T>} [valueShape]

* @returns {RecorderKit<T>}

Do you intend to exclude other patterns that are not themselves matchers, even if they contain matchers?

For now.

agoric-sdk/packages/zoe/src/contractSupport/recorder.js

Lines 261 to 271 in 4d54c56

/**

* Stop-gap until https://github.com/Agoric/agoric-sdk/issues/6160

* explictly specify the type that the Pattern will verify through a match.

*

* This is a Pattern but since that's `any`, including in the typedef turns the

* whole thing to `any`.

*

* @template T

* @typedef {import('@endo/patterns').Matcher & { validatedType?: T }} TypedMatcher

*/

Pattern is no longer any (thanks to you IIRC)

@typedef {Exclude<Passable, Error | Promise>} Pattern

Yes. From endojs/endo#2238

Ah yes, we can fix that now: 6b41829

erights · 2024-07-11T18:05:38Z

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

 });

-/** @type {{ [name: string]: [description: string, valueShape: Pattern] }} */
+/** @type {{ [name: string]: [description: string, valueShape: Matcher] }} */


pattern vs matcher

now in packages/async-flow

turadg · 2024-07-11T19:29:56Z

@Mergifyio requeue main

mergify · 2024-07-11T19:29:58Z

requeue main

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

turadg · 2024-07-11T19:34:37Z

@Mergifyio queue main

mergify · 2024-07-11T19:34:40Z

queue main

🛑 The pull request has been removed from the queue `main`

Details

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

turadg · 2024-07-11T20:41:17Z

@Mergifyio requeue main

mergify · 2024-07-11T20:41:20Z

requeue main

☑️ This pull request is already queued

turadg · 2024-07-11T23:13:56Z

This is failing only on getting-started link-cli because of a git status --porcelain porcelain check finding endo-sha.txt left over from an earlier run of CI. I'm going to bypass integration to land this.

mhofman

There is one behavioral change in this PR, and it is problematic

mhofman · 2024-07-12T01:11:17Z

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

+          const { remoteChainInfo, connectionInfo } = this.state;
+          const stakingDenom = remoteChainInfo.stakingTokens?.[0]?.denom;
+          if (!stakingDenom) {
+            return asVow(Fail`chain info lacks staking denom`);


This will not do what you expect, and throw an error instead of returning a rejected vow

should have been this?

Suggested change

return asVow(Fail`chain info lacks staking denom`);

return asVow(() => Fail`chain info lacks staking denom`);

That would work but I'm not sure why this PR removed the surrounding asVow in the first place.

I the assume bug is that Fail throws immediately here instead of propoagating as a vow?

What's a good way to catch this in a test? When reviewing this, I made similar edits to cosmos-orch-acct and didn't see any of these tests break:

agoric-sdk/packages/orchestration/test/examples/stake-ica.contract.test.ts

Lines 246 to 261 in 24528f1

await t.throwsAsync(E(account).getBalances(), {

message: 'not yet implemented',

});

await t.throwsAsync(E(account).send(mockChainAddress, mockAmountArg), {

message: 'not yet implemented',

});

// XXX consider, positioning amount + address args the same for .send and .transfer

await t.throwsAsync(E(account).transfer(mockAmountArg, mockChainAddress), {

message: 'not yet implemented',

});

await t.throwsAsync(E(account).transferSteps(mockAmountArg, null as any), {

message: 'not yet implemented',

});

await t.throwsAsync(E(account).withdrawRewards(), {

message: 'Not Implemented. Try using withdrawReward.',

});

The above does not go through the membrane, though. Would the membrane catch this? On my short list to test.

didn't see any of these tests break

yeah, we need breaking tests. Addressing in #9696

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

turadg force-pushed the 9449-membrane-vows branch from be1b9e8 to 7aa206d Compare July 11, 2024 00:56

turadg changed the base branch from master to pc/9673-sync-continuing-offer July 11, 2024 00:56

turadg requested review from 0xpatrickdev and erights July 11, 2024 01:00

turadg marked this pull request as ready for review July 11, 2024 01:00

Base automatically changed from pc/9673-sync-continuing-offer to master July 11, 2024 01:18

chore(types): update coverage measurement

884a94b

turadg force-pushed the 9449-membrane-vows branch from 7aa206d to 7bf6edf Compare July 11, 2024 01:24

erights reviewed Jul 11, 2024

View reviewed changes

0xpatrickdev approved these changes Jul 11, 2024

View reviewed changes

turadg added the force:integration Force integration tests to run on PR label Jul 11, 2024

erights reviewed Jul 11, 2024

View reviewed changes

turadg requested a review from erights July 11, 2024 18:38

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

turadg added 14 commits July 11, 2024 19:15

feat: error on membrane promise

94dfab1

test(types): retriable

6005984

refactor: types to .d.ts

d91d11d

feat(types): HostOf, GuestOf

35380af

chore: update type-coverage

da4380d

refactor(types): use HostOf

541900f

chore(types): rm Orchestration membrane helpers

df5d174

now in packages/async-flow

fix: use heapVowE for call on remote possible vow

9e8a29a

chore(types): produce stakeOsmo

704bad6

feat(types): LocalAccountMethods for agoric Orch account

b4fdee6

chore(types): LocalTransfer

96b5c6e

chore(types): annotate Orchestration account impls

4ad9fb5

chore(types): offerWatcher

beeb7da

fix(types): PublicSubscribers

91ef352

turadg and others added 4 commits July 11, 2024 19:15

chore(types): Orchestration continuing offers

4be4842

chore: update type-coverage

43a8f5a

chore(facade): say _which_ params are missing

2405c8f

chore(facade): harden API surfaces

f10c9c1

turadg force-pushed the 9449-membrane-vows branch from 86e05ae to f10c9c1 Compare July 11, 2024 19:15

turadg removed the force:integration Force integration tests to run on PR label Jul 11, 2024

fix(types): TypedMatcher --> TypedPattern

6b41829

turadg added the bypass:integration Prevent integration tests from running on PR label Jul 11, 2024

mergify bot merged commit 3915e4c into master Jul 11, 2024

mergify bot deleted the 9449-membrane-vows branch July 11, 2024 23:14

turadg mentioned this pull request Jul 12, 2024

retry helper to durably watch idempotent promises #9541

Closed

mhofman reviewed Jul 12, 2024

View reviewed changes

turadg mentioned this pull request Jul 12, 2024

fix vow failure handling #9696

Merged

		@@ -154,14 +154,9 @@ export const makeReplayMembrane = ({
		const passStyle = passStyleOf(h);

	* A {PublicTopic} in which the `storagePath` is a string.
	* A {PublicTopic} in which the `storagePath` is a string.
	* Useful when working with Vows and async-flow.

	* @param {TypedMatcher<T>} [valueShape]
	* @returns {RecorderKit<T>}


	/**
	* Stop-gap until https://github.com/Agoric/agoric-sdk/issues/6160
	* explictly specify the type that the Pattern will verify through a match.
	*
	* This is a Pattern but since that's `any`, including in the typedef turns the
	* whole thing to `any`.
	*
	* @template T
	* @typedef {import('@endo/patterns').Matcher & { validatedType?: T }} TypedMatcher
	*/

	return asVow(Fail`chain info lacks staking denom`);
	return asVow(() => Fail`chain info lacks staking denom`);

	await t.throwsAsync(E(account).getBalances(), {
	message: 'not yet implemented',
	});
	await t.throwsAsync(E(account).send(mockChainAddress, mockAmountArg), {
	message: 'not yet implemented',
	});
	// XXX consider, positioning amount + address args the same for .send and .transfer
	await t.throwsAsync(E(account).transfer(mockAmountArg, mockChainAddress), {
	message: 'not yet implemented',
	});
	await t.throwsAsync(E(account).transferSteps(mockAmountArg, null as any), {
	message: 'not yet implemented',
	});
	await t.throwsAsync(E(account).withdrawRewards(), {
	message: 'Not Implemented. Try using withdrawReward.',
	});

Comments

Conversation

turadg commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Uh oh!

cloudflare-workers-and-pages bot commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying agoric-sdk with Cloudflare Pages

Uh oh!

erights left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xpatrickdev left a comment

Choose a reason for hiding this comment

Uh oh!

0xpatrickdev Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turadg Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

0xpatrickdev Jul 11, 2024 •

edited

Loading

turadg Jul 11, 2024 •

edited

Loading

mergify bot commented Jul 11, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `main`