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

feat(internal): new prepareAttenuator leveraging callbacks #7586

Merged
merged 2 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 161 additions & 4 deletions packages/internal/src/callback.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,15 @@
import { E } from '@endo/far';
import { isObject, isPassableSymbol } from '@endo/marshal';

const { Fail } = assert;
const { Fail, quote: q } = assert;

const { fromEntries } = Object;

const { ownKeys: rawOwnKeys } = Reflect;
const ownKeys =
/** @type {<T extends PropertyKey>(obj: {[K in T]?: unknown}) => T[]} */ (
rawOwnKeys
);

/**
* @template {(...args: unknown[]) => any} I
Expand All @@ -14,6 +22,24 @@ const { Fail } = assert;
* @typedef {import('./types').SyncCallback<I>} SyncCallback
*/

/** @template T @typedef {import('@endo/eventual-send').RemotableBrand<{}, T> & T} Farable */

/**
* @param {unknown} key
* @returns {key is PropertyKey} FIXME: should be just `PropertyKey` but TS
* complains it can't be used as an index type.
*/
const isPropertyKey = key => {
switch (typeof key) {
case 'string':
case 'number':
case 'symbol':
return true;
default:
return false;
}
};

/**
* Synchronously call a callback.
*
Expand All @@ -29,6 +55,7 @@ export const callSync = (callback, ...args) => {
}
return target[methodName](...bound, ...args);
};
harden(callSync);

/**
* Eventual send to a callback.
Expand All @@ -45,6 +72,7 @@ export const callE = (callback, ...args) => {
}
return E(target)[methodName](...bound, ...args);
};
harden(callE);

/**
* Create a callback from a near function.
Expand All @@ -60,7 +88,7 @@ export const makeSyncFunctionCallback = (target, ...bound) => {
typeof target === 'function' ||
Fail`sync function callback target must be a function: ${target}`;
/** @type {unknown} */
const cb = harden({ target, bound });
const cb = harden({ target, bound, isSync: true });
return /** @type {SyncCallback<I>} */ (cb);
};
harden(makeSyncFunctionCallback);
Expand Down Expand Up @@ -107,7 +135,7 @@ export const makeSyncMethodCallback = (target, methodName, ...bound) => {
isPassableSymbol(methodName) ||
Fail`method name must be a string or passable symbol: ${methodName}`;
/** @type {unknown} */
const cb = harden({ target, methodName, bound });
const cb = harden({ target, methodName, bound, isSync: true });
return /** @type {SyncCallback<I>} */ (cb);
};
harden(makeSyncMethodCallback);
Expand Down Expand Up @@ -139,7 +167,7 @@ harden(makeMethodCallback);

/**
* @param {any} callback
* @returns {callback is Callback}
* @returns {callback is Callback<any>}
*/
export const isCallback = callback => {
if (!isObject(callback)) {
Expand All @@ -155,3 +183,132 @@ export const isCallback = callback => {
);
};
harden(isCallback);

/**
* Prepare an attenuator class whose methods can be redirected via callbacks.
michaelfig marked this conversation as resolved.
Show resolved Hide resolved
*
* @template {PropertyKey} M
* @param {import('@agoric/zone').Zone} zone The zone in which to allocate attenuators.
* @param {M[]} methodNames Methods to forward.
* @param {object} opts
* @param {InterfaceGuard} [opts.interfaceGuard] An interface guard for the
* new attenuator.
* @param {string} [opts.tag] A tag for the new attenuator exoClass.
*/
export const prepareAttenuator = (
zone,
methodNames,
{ interfaceGuard, tag = 'Attenuator' } = {},
) => {
/**
* @typedef {(this: any, ...args: unknown[]) => any} Method
* @typedef {{ [K in M]: Method }} Methods
* @typedef {{ [K in M]?: Callback<any> | null}} Overrides
*/
const methods = fromEntries(
methodNames.map(key => {
// Only allow the `PropertyKey` type for the target method key.
if (!isPropertyKey(key)) {
throw Fail`key ${q(key)} is not a PropertyKey`;
}

const m = /** @type {Methods} */ ({
erights marked this conversation as resolved.
Show resolved Hide resolved
// Explicitly use concise method syntax to preserve `this` but prevent
// constructor behavior.
/** @type {Method} */
[key](...args) {
// Support both synchronous and async callbacks.
const cb = this.state.cbs[key];
if (!cb) {
const err = assert.error(
`unimplemented ${q(tag)} method ${q(key)}`,
);
if (this.state.isSync) {
throw err;
}
return Promise.reject(err);
}
if (cb.isSync) {
return callSync(cb, ...args);
}
return callE(cb, ...args);
},
})[key];
return /** @type {const} */ ([key, m]);
}),
);

const methodKeys = /** @type {M[]} */ (ownKeys(methods));

/**
* Create an exo object whose behavior is composed from a default target
* and/or individual method override callbacks.
*
* @param {object} opts
* @param {unknown} [opts.target] The target for any methods that
* weren't specified in `opts.overrides`.
Comment on lines +248 to +249
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use case for making target optional (e.g., makeAttenuator({ target: undefined, overrides }))?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to compose an object entirely out of callbacks, you don't need a default target.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we encourage use of makeAttenuator for composing an object from callbacks? I would expect not, as opposed to e.g.

const makeInstance = zone.exoClass(tag, interfaceGuard, initEmpty, { ...callbacks });

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Aside from legacy...)

Now that we have attenuators, when would we ever use callbacks? AFAICT, attenuators are just better. If so, perhaps we should try to phase out callbacks over time.

Copy link
Member

@mhofman mhofman May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how attenuators supplant callbacks. Thanks to callbacks you can compose a new object of a given shape from different existing objects. They are complimentary.

* @param {boolean} [opts.isSync=false] Whether the target should be treated
* as synchronously available.
* @param {Overrides} [opts.overrides] Set individual
* callbacks for methods (whose names must be defined in the
* `prepareAttenuator` or `prepareGuardedAttenuator` call). Nullish overrides
* mean to throw.
*/
const makeAttenuator = zone.exoClass(
tag,
interfaceGuard,
/**
* @param {object} opts
* @param {any} [opts.target]
* @param {boolean} [opts.isSync=false]
* @param {Overrides} [opts.overrides]
*/
({
target = null,
isSync = false,
overrides = /** @type {Overrides} */ ({}),
}) => {
const cbs = /** @type {Overrides} */ ({});

const remaining = new Set(methodKeys);
for (const key of ownKeys(overrides)) {
remaining.has(key) ||
Fail`${q(tag)} overrides[${q(key)}] not allowed by methodNames`;

remaining.delete(key);
const cb = overrides[key];
if (cb != null) {
isCallback(cb) ||
Fail`${q(tag)} overrides[${q(key)}] is not a callback; got ${cb}`;
}
cbs[key] = cb;
}
for (const key of remaining) {
if (isSync) {
cbs[key] = makeSyncMethodCallback(target, key);
} else {
cbs[key] = makeMethodCallback(target, key);
}
}
return harden({ cbs, isSync });
},
/** @type {Methods} */ (methods),
);
return makeAttenuator;
};
harden(prepareAttenuator);

/**
* Prepare an attenuator whose methodNames are derived from the interfaceGuard.
*
* @param {import('@agoric/zone').Zone} zone
* @param {InterfaceGuard} interfaceGuard
* @param {object} [opts]
* @param {string} [opts.tag]
*/
export const prepareGuardedAttenuator = (zone, interfaceGuard, opts = {}) => {
const { methodGuards } = interfaceGuard;
const methodNames = ownKeys(methodGuards);
return prepareAttenuator(zone, methodNames, { ...opts, interfaceGuard });
};
harden(prepareGuardedAttenuator);
4 changes: 4 additions & 0 deletions packages/internal/src/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ export declare class Callback<I extends (...args: unknown[]) => any> {
public methodName?: PropertyKey;

public bound: unknown[];

public isSync: boolean;
}

export declare class SyncCallback<
I extends (...args: unknown[]) => any,
> extends Callback<I> {
private syncIface: I;

public isSync: true;
}
101 changes: 91 additions & 10 deletions packages/internal/test/test-callback.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import '@endo/init';
import test from 'ava';

import { Far } from '@endo/far';
import { heapZone } from '@agoric/zone';
import * as cb from '../src/callback.js';

test('near function callbacks', t => {
Expand All @@ -16,19 +17,19 @@ test('near function callbacks', t => {

/** @type {import('../src/callback').SyncCallback<typeof f>} */
const cb0 = cb.makeSyncFunctionCallback(f);
t.deepEqual(cb0, { target: f, bound: [] });
t.deepEqual(cb0, { target: f, bound: [], isSync: true });

/** @type {import('../src/callback').SyncCallback<(b: number, c: string) => string>} */
const cb1 = cb.makeSyncFunctionCallback(f, 9);
t.deepEqual(cb1, { target: f, bound: [9] });
t.deepEqual(cb1, { target: f, bound: [9], isSync: true });

/** @type {import('../src/callback').SyncCallback<(c: string) => string>} */
const cb2 = cb.makeSyncFunctionCallback(f, 9, 10);
t.deepEqual(cb2, { target: f, bound: [9, 10] });
t.deepEqual(cb2, { target: f, bound: [9, 10], isSync: true });

// @ts-expect-error deliberate: boolean is not assignable to string
const cb3 = cb.makeSyncFunctionCallback(f, 9, 10, true);
t.deepEqual(cb3, { target: f, bound: [9, 10, true] });
t.deepEqual(cb3, { target: f, bound: [9, 10, true], isSync: true });

// @ts-expect-error deliberate: Expected 4 arguments but got 5
t.is(cb.callSync(cb0, 2, 3, 'go', 'bad'), '5go');
Expand Down Expand Up @@ -74,23 +75,33 @@ test('near method callbacks', t => {

/** @type {import('../src/callback').SyncCallback<typeof o.m1>} */
const cb0 = cb.makeSyncMethodCallback(o, 'm1');
t.deepEqual(cb0, { target: o, methodName: 'm1', bound: [] });
t.deepEqual(cb0, { target: o, methodName: 'm1', bound: [], isSync: true });

/** @type {import('../src/callback').SyncCallback<(b: number, c: string) => string>} */
const cb1 = cb.makeSyncMethodCallback(o, 'm1', 9);
t.deepEqual(cb1, { target: o, methodName: 'm1', bound: [9] });
t.deepEqual(cb1, { target: o, methodName: 'm1', bound: [9], isSync: true });

/** @type {import('../src/callback').SyncCallback<(c: string) => string>} */
const cb2 = cb.makeSyncMethodCallback(o, 'm1', 9, 10);
t.deepEqual(cb2, { target: o, methodName: 'm1', bound: [9, 10] });
t.deepEqual(cb2, {
target: o,
methodName: 'm1',
bound: [9, 10],
isSync: true,
});

// @ts-expect-error deliberate: boolean is not assignable to string
const cb3 = cb.makeSyncMethodCallback(o, 'm1', 9, 10, true);
t.deepEqual(cb3, { target: o, methodName: 'm1', bound: [9, 10, true] });
t.deepEqual(cb3, {
target: o,
methodName: 'm1',
bound: [9, 10, true],
isSync: true,
});

/** @type {import('../src/callback').SyncCallback<(c: string) => string>} */
const cb4 = cb.makeSyncMethodCallback(o, m2, 9, 10);
t.deepEqual(cb4, { target: o, methodName: m2, bound: [9, 10] });
t.deepEqual(cb4, { target: o, methodName: m2, bound: [9, 10], isSync: true });

// @ts-expect-error deliberate: Expected 4 arguments but got 5
t.is(cb.callSync(cb0, 2, 3, 'go', 'bad'), '5go');
Expand All @@ -104,7 +115,7 @@ test('near method callbacks', t => {

// @ts-expect-error deliberate: Promise provides no match for the signature
const cbp2 = cb.makeSyncMethodCallback(Promise.resolve(o), 'm1', 9, 10);
t.like(cbp2, { methodName: 'm1', bound: [9, 10] });
t.like(cbp2, { methodName: 'm1', bound: [9, 10], isSync: true });
t.assert(cbp2.target instanceof Promise);
t.throws(() => cb.callSync(cbp2, 'go'), { message: /not a function/ });
});
Expand Down Expand Up @@ -253,3 +264,73 @@ test('isCallback', t => {
'missing bound args',
);
});

test('makeAttenuator', async t => {
const makeAttenuator = cb.prepareAttenuator(heapZone, [
'm0',
'm1',
'm2',
'm4',
]);
const target = Far('original', {
m0() {
return 'return original.m0';
},
m1() {
return 'return original.m1';
},
m2() {
throw Error('unexpected original.m2');
},
m3() {
throw Error('unexpected original.m3');
},
});
t.throws(
// @ts-expect-error deliberate: m3 is not on the yeslist
() => makeAttenuator({ target, overrides: { m3: null } }),
{
message: `"Attenuator" overrides["m3"] not allowed by methodNames`,
},
);

// Null out a method.
const atE = makeAttenuator({ target, overrides: { m1: null } });
const p1 = atE.m0();
t.assert(p1 instanceof Promise);
t.is(await p1, 'return original.m0');
await t.throwsAsync(() => atE.m1(), {
message: `unimplemented "Attenuator" method "m1"`,
});
await t.throwsAsync(() => atE.m2(), { message: `unexpected original.m2` });
// @ts-expect-error deliberately attenuated out of existence
t.throws(() => atE.m3(), { message: /not a function/ });
await t.throwsAsync(() => atE.m4(), { message: /target has no method "m4"/ });
Comment on lines +306 to +308
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these errors be more similar? Can they be?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first is an engine-specific TypeError for which not a function is the terse message you get when atE.m3 is not defined (in Node.js for this test).

The second is a rejection by the HandledPromise shim that contains much more specific information, such as the methods that do exist, and the type of target. We could change this one, but its format is pretty deeply entrenched in Agoric SDK unit tests.


const atSync = makeAttenuator({
target,
isSync: true,
overrides: {
m1: null,
m2: cb.makeMethodCallback(
{
abc() {
return 'return abc';
},
},
'abc',
),
},
});

t.is(atSync.m0(), 'return original.m0');
t.throws(() => atSync.m1(), {
message: `unimplemented "Attenuator" method "m1"`,
});
const p2 = atSync.m2();
t.assert(p2 instanceof Promise);
t.is(await p2, 'return abc');
// @ts-expect-error deliberately attenuated out of existence
t.throws(() => atSync.m3(), { message: /not a function/ });
t.throws(() => atSync.m4(), { message: /not a function/ });
});
Loading