feat: ChainAccountKit returns vows #9562

turadg · 2024-06-21T20:30:33Z

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

cloudflare-workers-and-pages · 2024-06-21T20:31:33Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`9bc7afb`
Status:	✅ Deploy successful!
Preview URL:	https://8ee90ae4.agoric-sdk.pages.dev
Branch Preview URL:	https://9449-chain-account-vows.agoric-sdk.pages.dev

View logs

michaelfig · 2024-06-23T02:58:23Z

My only question is: are you concerned that in a non-async function, a throw blah is synchronous, not reified as a return Promise.reject(blah)?

If you're fine with that, I will review this PR having understood that you're doing it in order to accommodate linting rules. My only request before I do, is that you update it with master to make the actual changes clearer to me.

Thanks!

turadg · 2024-06-23T17:15:12Z

are you concerned that in a non-async function, a throw blah is synchronous, not reified as a return Promise.reject(blah)?

Per #9449 these methods must always return Vows, so must not throw. IIUC returning a rejected promise is no better.

0xpatrickdev · 2024-06-23T17:46:13Z

packages/orchestration/src/exos/chain-account-kit.js

@@ -188,15 +187,27 @@ export const prepareChainAccountKit = (zone, { watch, when }) =>
            chainId: this.state.chainId,
            addressEncoding: 'bech32',
          });
+          return Promise.resolve(watch(undefined));


Why return anything here?

Consistent return type

I relaxed the lint rule for connectionHandler > onOpen | onClose children in 2863d1b. The connectionHandler is only used internally by the exo, and won't/can't be called by a consumer of orchestrate. Replying to network vat with an empty vow / promise also seems like a waste since it's not interested in our answer.

An alternative explored was adjusting the types:

diff --git a/packages/network/src/types.js b/packages/network/src/types.js index d40be9ad1..e70033ace 100644 --- a/packages/network/src/types.js +++ b/packages/network/src/types.js @@ -101,7 +101,7 @@ export {}; * localAddr: Endpoint, * remoteAddr: Endpoint, * c: Remote<ConnectionHandler>, - * ) => PromiseVow<void>} [onOpen] + * ) => PromiseVow<void> | void} [onOpen] * The connection has been opened * @property {( * connection: Remote<Connection>, @@ -114,7 +114,7 @@ export {}; * connection: Remote<Connection>, * reason?: CloseReason, * c?: Remote<ConnectionHandler>, - * ) => PromiseVow<void>} [onClose] + * ) => PromiseVow<void | void>} [onClose] * The connection has been closed * * @typedef {any | null} CloseReason The reason a connection was closed

0xpatrickdev · 2024-06-23T17:52:52Z

packages/orchestration/src/exos/chain-account-kit.js

          trace(`ICA Channel onReceive`, connection, bytes);
-          return '';
+          return Promise.resolve(watch(''));


Should we instead do this?

Suggested change

return Promise.resolve(watch(''));

return watch(Promise.resolve(''));

Also, I'm not sure we need to return anything here - return '' might be an artifact from satisfying this type guard:

agoric-sdk/packages/network/src/network.js

Line 1224 in 3027adf

.returns(Shape.Data),

There is no need for the onReceive handler. I removed it in d6bd228

packages/orchestration/src/exos/chain-account-kit.js

michaelfig · 2024-06-23T21:04:02Z

Per #9449 these methods must always return Vows, so must not throw. IIUC returning a rejected promise is no better.

Please use the wrapper pattern recommended by @mhofman in #9454 (comment)

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

0xpatrickdev · 2024-06-25T20:25:50Z

@michaelfig I've commandeered 🏴‍☠️ Turadg's PR if you could PTAL. When we come to agreement on the approach for connectionHandler, I will revise the PR body. Also tagging @dckc if he's interested to provide feedback.

dckc · 2024-06-25T22:17:56Z

... To conform to the new lint rules, this removes the async but returns Promise.resolve(watch( as an idiom for immediate PromiseVow.

is that still true? or is it overtaken by asVow?

dckc

a few non-critical suggestions

dckc · 2024-06-25T21:52:26Z

.eslintrc.cjs

-    selector: 'FunctionExpression[async=true]',
+    selector:
+      'FunctionExpression[async=true]:not(Property[key.name="connectionHandler"] > ObjectExpression > Property[key.name=/^(onOpen|onClose)$/] > FunctionExpression[async=true])',


I could sure use a comment to help read this.

Added a comment in e7b839a

dckc · 2024-06-25T21:58:13Z

packages/orchestration/src/exos/chain-account-kit.js

+            // TODO #9281 do not throw synchronously when returning a promise; return a rejected Vow
+            /// see https://github.com/Agoric/agoric-sdk/pull/9454#discussion_r1626898694


the addVow() call means this TODO is done now, isn't it?

Suggested change

// TODO #9281 do not throw synchronously when returning a promise; return a rejected Vow

/// see https://github.com/Agoric/agoric-sdk/pull/9454#discussion_r1626898694

dckc · 2024-06-25T21:59:47Z

packages/orchestration/src/exos/chain-account-kit.js

+          return asVow(() => {
+            throw new Error('not yet implemented');
+          });


orthogonal to this PR, but I think our preference is Error(...), not new Error(...)

Suggested change

return asVow(() => {

throw new Error('not yet implemented');

});

return asVow(() => {

throw Error('not yet implemented');

});

I'm struggling to confirm from Coding Style though.

dckc · 2024-06-25T22:00:29Z

packages/orchestration/src/exos/chain-account-kit.js

+          return asVow(() => {
+            throw new Error('not yet implemented');
+          });


or, for brevity:

Suggested change

return asVow(() => {

throw new Error('not yet implemented');

});

return asVow(() => assert.fail('not yet implemented'));

dckc · 2024-06-25T22:02:28Z

packages/orchestration/src/exos/chain-account-kit.js

+            const { connection } = this.state;
+            // TODO #9281 do not throw synchronously when returning a promise; return a rejected Vow
+            /// see https://github.com/Agoric/agoric-sdk/pull/9454#discussion_r1626898694
+            if (!connection) throw Fail`connection not available`;


I'm partial to doing that on one line, but I'm pretty sure others prefer...

Suggested change

if (!connection) throw Fail`connection not available`;

if (!connection) {

throw Fail`connection not available`;

}

I'm not sure why our lint tools don't require that. Maybe it's OK.

if (!connection) throw Fail`connection not available`;

doesn't seem too different from:

connection || Fail`connection not available`;

from a readability standpoint.

The latter I understand to be a preferred pattern, but it unfortunately does not satisfy ts-check.

I've implemented your other suggestions, thanks for the review!

Co-authored-by: 0xPatrick <[email protected]>

- ICA and ICQ channels only send outbound requests; they do not receive incoming requests they need to respond to - renames ConnectionHandlerI to OutboundConnectionHandlerI to better reflect the behavior

…ble rule - onOpen and onClose are event listeners that handle incoming notifications - these methods do not return values and are not expected to be resumable

error TS2741: Property 'account' is missing in type '{}' but required in type '{ account: OrchestrationAccount<any> | undefined; }'

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

turadg requested review from michaelfig and 0xpatrickdev June 21, 2024 20:30

turadg force-pushed the ta/heap-vows branch from 9a8962a to 6623af5 Compare June 22, 2024 13:40

Base automatically changed from ta/heap-vows to master June 22, 2024 14:11

turadg force-pushed the 9449-chain-account-vows branch from aea3535 to f2fa2dc Compare June 23, 2024 17:15

0xpatrickdev reviewed Jun 23, 2024

View reviewed changes

0xpatrickdev mentioned this pull request Jun 24, 2024

feat(vowTools): add asVow helper #9577

Merged

0xpatrickdev force-pushed the 9449-chain-account-vows branch from f2fa2dc to 2863d1b Compare June 25, 2024 19:43

0xpatrickdev requested a review from dckc June 25, 2024 20:26

dckc approved these changes Jun 25, 2024

View reviewed changes

turadg and others added 4 commits June 25, 2024 18:59

feat: ChainAccountKit returns vows

4383aec

Co-authored-by: 0xPatrick <[email protected]>

refactor(ConnectionHandler): remove unused, optional onReceive handler

465b415

- ICA and ICQ channels only send outbound requests; they do not receive incoming requests they need to respond to - renames ConnectionHandlerI to OutboundConnectionHandlerI to better reflect the behavior

feat(lint): exempt connectionHandler notification methods from resuma…

f409a8d

…ble rule - onOpen and onClose are event listeners that handle incoming notifications - these methods do not return values and are not expected to be resumable

fix(orchestrate): makeStateRecord type annotation

9bc7afb

error TS2741: Property 'account' is missing in type '{}' but required in type '{ account: OrchestrationAccount<any> | undefined; }'

0xpatrickdev force-pushed the 9449-chain-account-vows branch from e7b839a to 9bc7afb Compare June 25, 2024 23:28

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Jun 25, 2024

0xpatrickdev mentioned this pull request Jun 25, 2024

fix(orchestrate): makeStateRecord type annotation #9581

Closed

mergify bot merged commit 23e1921 into master Jun 26, 2024
85 checks passed

mergify bot deleted the 9449-chain-account-vows branch June 26, 2024 00:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ChainAccountKit returns vows #9562

feat: ChainAccountKit returns vows #9562

turadg commented Jun 21, 2024 •

edited by 0xpatrickdev

Loading

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

edited

Loading

michaelfig commented Jun 23, 2024

turadg commented Jun 23, 2024

0xpatrickdev Jun 23, 2024

turadg Jun 23, 2024

0xpatrickdev Jun 25, 2024

0xpatrickdev Jun 23, 2024

0xpatrickdev Jun 25, 2024 •

edited

Loading

michaelfig commented Jun 23, 2024

0xpatrickdev commented Jun 25, 2024

dckc commented Jun 25, 2024

dckc left a comment

dckc Jun 25, 2024

0xpatrickdev Jun 25, 2024

dckc Jun 25, 2024

dckc Jun 25, 2024

dckc Jun 25, 2024

dckc Jun 25, 2024

0xpatrickdev Jun 25, 2024

	return Promise.resolve(watch(''));
	return watch(Promise.resolve(''));

		// TODO #9281 do not throw synchronously when returning a promise; return a rejected Vow
		/// see https://github.com/Agoric/agoric-sdk/pull/9454#discussion_r1626898694

feat: ChainAccountKit returns vows #9562

feat: ChainAccountKit returns vows #9562

Conversation

turadg commented Jun 21, 2024 • edited by 0xpatrickdev Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

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

Deploying agoric-sdk with Cloudflare Pages

michaelfig commented Jun 23, 2024

turadg commented Jun 23, 2024

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

Choose a reason for hiding this comment

michaelfig commented Jun 23, 2024

0xpatrickdev commented Jun 25, 2024

dckc commented Jun 25, 2024

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

turadg commented Jun 21, 2024 •

edited by 0xpatrickdev

Loading

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

edited

Loading

0xpatrickdev Jun 25, 2024 •

edited

Loading