fix(exo): Extend InterfaceGuard to support symbol-keyed methods #1740

gibson042 · 2023-08-26T19:49:05Z

Description

Extend InterfaceGuard with a symbolMethodGuards field whose value is a CopyMap<symbol, MethodGuard> when the interface has symbol-keyed methods such as Symbol.asyncIterator. The current implementation expresses it as completely optional and does not create it when there are no symbol-keyed methods, but we could also set it to an empty CopyMap in that case.

Security Considerations

None AFAICT.

Scaling Considerations

None AFAICT, even if we always add the CopyMap.

Documentation Considerations

None.

Testing Considerations

Just unit tests.

Upgrade Considerations

None special; the new functionality should be fully backwards compatible.

erights

First quick reaction: Awesome choice to use a CopyMap rather than an array of pairs! In thinking about this issue, that "natural" choice never occurred to me.

(Good you're also fixing CopyMap patterns, just in time for the first production use of CopyMaps!)

erights · 2023-08-27T03:53:31Z

expresses it as completely optional and does not create it when there are no symbol-keyed methods, but we could also set it to an empty CopyMap in that case.

Since it arrives late, after an earlier representation shipped, and optional is coherent. I think it should remain optional. But if it does happen to be present with an empty map, that should be fine too. I don't see any reason to insist on a canonical representation for this case. (I think you're already doing that, in which case, no change suggested.)

erights

LGTM, thanks! For the sake of agoric-sdk, it's great that we can get this into the next endo release.

erights · 2023-08-27T04:13:26Z

packages/exo/README.md

@@ -10,13 +10,19 @@ When an exo is defined with an InterfaceGuard, the exo is augmented by default w

 ```js
 // `GET_INTERFACE_GUARD` holds the name of the meta-method
-import { GET_INTERFACE_GUARD } from `@endo/exo`;
+import { GET_INTERFACE_GUARD } from '@endo/exo';
+import { getCopyMapKeys } from '@endo/patterns';

 ...
   const interfaceGuard = await E(exo)[GET_INTERFACE_GUARD]();
   // `methodNames` omits names of automatically added meta-methods like
   // the value of `GET_INTERFACE_GUARD`.
   // Others may also be omitted if `interfaceGuard.partial`


Attn @michaelfig

My mistake! In anything that saw the light of day, it was never called interfaceGuard.partial. It became interfaceGuard.sloppy, but I forgot to rename this occurrence.

But @michaelfig is in process with a PR that will make changes in this area, so leave that to him. For your PR, no change suggested.

erights · 2023-08-27T04:16:48Z

packages/exo/README.md

+   const methodNames = Reflect.ownKeys(interfaceGuard.methodGuards)
+     .concat(
+       interfaceGuard.symbolMethodGuards
+         ? getCopyMapKeys(interfaceGuard.symbolMethodGuards)
+         : []
+     );


I like the following style a bit better, but up to you.

Suggested change

const methodNames = Reflect.ownKeys(interfaceGuard.methodGuards)

.concat(

interfaceGuard.symbolMethodGuards

? getCopyMapKeys(interfaceGuard.symbolMethodGuards)

: []

);

const methodNames = [

...Reflect.ownKeys(interfaceGuard.methodGuards),

...interfaceGuard.symbolMethodGuards

? getCopyMapKeys(interfaceGuard.symbolMethodGuards)

: [],

];

erights · 2023-08-27T04:21:19Z

packages/exo/src/exo-tools.js

+    methodGuards = harden({
+      ...mg,
+      ...(symbolMethodGuards &&
+        fromEntries(getCopyMapEntries(symbolMethodGuards))),
+    });


Nice!

Given this, my preference to use [...x, ...y] above is stronger. Now I will request that change ;)

erights · 2023-08-27T04:24:38Z

packages/exo/test/test-heap-classes.js

+  t.is(UpCounterI.symbolMethodGuards, undefined);
+
+  const symbolic = Symbol.for('symbolic');
+  const FooI = M.interface('Foo', {


Please also add a test with an exo implementing FooI and then call the symbol-named method to test that the raw method is guarded by the methodGuard.

erights · 2023-08-27T04:26:09Z

packages/patterns/src/patterns/patternMatchers.js

+    klass: 'Interface',
+    interfaceName: M.string(),
+    methodGuards: M.recordOf(M.string(), MethodGuardShape),
+    sloppy: M.boolean(),


Just be aware that as of @michaelfig 's upcoming PR, sloppy will move into that optional section, and another will appear there as well. No change suggested.

erights · 2023-08-27T04:29:11Z

packages/patterns/src/types.js

+ *   methodGuards: { [K in keyof T]: K extends symbol ? never : T[K] },
+ *   symbolMethodGuards?: CopyMap<symbol, MethodGuard>,


Nice typing of methodGuards. I wondered how to do that.

Could you use the same trick to type symbolMethodGuard with the opposing side of that conditional?

Attn @turadg

I think doing so will help @turadg 's efforts to statically inter TS types from patterns and guards.

I got it to type the CopyMap K as a symbol-keyed entry of T, but don't know how to establish the pair relationship. It will be open for followup.

erights · 2023-08-27T04:31:55Z

packages/patterns/src/patterns/patternMatchers.js

+    ...(symbolMethodGuardsEntries.length
+      ? { symbolMethodGuards: makeCopyMap(symbolMethodGuardsEntries) }
+      : {}),


That ... (x ? {y} : {}) is quite cute. No change suggested.

Fixes #1728

Ref #1740

Ref #1740 Closes #1753

gibson042 requested a review from erights August 26, 2023 19:49

erights reviewed Aug 26, 2023

View reviewed changes

erights approved these changes Aug 27, 2023

View reviewed changes

fix(exo): Extend InterfaceGuard to support symbol-keyed methods

72101f9

Fixes #1728

gibson042 force-pushed the gibson-1728-symbol-keyed-methodguards branch from 3a2223f to 72101f9 Compare August 27, 2023 16:03

gibson042 merged commit d6ea36b into master Aug 27, 2023

gibson042 deleted the gibson-1728-symbol-keyed-methodguards branch August 27, 2023 16:11

This was referenced Aug 27, 2023

Changes to @endo/pass-style and up likely needed for ocapn conformance. #1587

Open

fix(publish-kit): workaround endo bug #1728 Agoric/agoric-sdk#8232

Closed

DO NOT MERGE test #2832 against "current" endo master Agoric/agoric-sdk#8233

Closed

gibson042 added a commit that referenced this pull request Aug 30, 2023

docs(exo): Update NEWS.md

ea2b35c

Ref #1740

gibson042 mentioned this pull request Aug 30, 2023

docs(exo): Update NEWS.md #1753

Merged

gibson042 added a commit that referenced this pull request Aug 30, 2023

docs(exo): Update NEWS.md

c38f7ba

Ref #1740

gibson042 added a commit that referenced this pull request Aug 30, 2023

docs(exo): Update NEWS.md

b511ff0

Ref #1740 Closes #1753

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(exo): Extend InterfaceGuard to support symbol-keyed methods #1740

fix(exo): Extend InterfaceGuard to support symbol-keyed methods #1740

gibson042 commented Aug 26, 2023

erights left a comment

erights commented Aug 27, 2023 •

edited

Loading

erights left a comment

erights Aug 27, 2023

erights Aug 27, 2023

gibson042 Aug 27, 2023

erights Aug 27, 2023

erights Aug 27, 2023

gibson042 Aug 27, 2023

erights Aug 27, 2023

erights Aug 27, 2023

gibson042 Aug 27, 2023

erights Aug 27, 2023

		* methodGuards: { [K in keyof T]: K extends symbol ? never : T[K] },
		* symbolMethodGuards?: CopyMap<symbol, MethodGuard>,

fix(exo): Extend InterfaceGuard to support symbol-keyed methods #1740

fix(exo): Extend InterfaceGuard to support symbol-keyed methods #1740

Conversation

gibson042 commented Aug 26, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

erights left a comment

Choose a reason for hiding this comment

erights commented Aug 27, 2023 • edited Loading

erights 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

erights commented Aug 27, 2023 •

edited

Loading