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

Conversation

michaelfig
Copy link
Member

closes: Needs issue

Description

Adds a mechanism to define an exo class presenting the same external API, but backed per-instance by passable callback descriptors.

Security Considerations

Improves encapsulation; instead of marshalling raw callback descriptors (which expose their unattenuated targets), an exo ocap can be conveyed.

Scaling Considerations

Documentation Considerations

Testing Considerations

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

Interesting approach, but I'm concerned about the cognitive overhead of having to explicitly create callbacks for the simple case of a single target.

packages/internal/src/callback.js Outdated Show resolved Hide resolved
packages/internal/src/callback.js Outdated Show resolved Hide resolved
packages/internal/src/callback.js Show resolved Hide resolved
packages/internal/src/callback.js Outdated Show resolved Hide resolved
packages/internal/test/test-callback.js Outdated Show resolved Hide resolved
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for entertaining those changes.

Remainder are only small nits that don't have any meaningful impact.

packages/internal/src/callback.js Outdated Show resolved Hide resolved
packages/internal/src/callback.js Outdated Show resolved Hide resolved
packages/internal/src/callback.js Outdated Show resolved Hide resolved
@michaelfig michaelfig marked this pull request as ready for review May 3, 2023 20:06
@erights erights self-requested a review May 4, 2023 00:10
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

(I was surprised to see how dependent this is on the Callback abstraction, but no objection)

@michaelfig
Copy link
Member Author

Hi @gibson042, I'm merging this now to unblock other work.

However, if you still want to give a detailed review, please leave it here and I will address comments and implement changes in a new PR.

@michaelfig michaelfig added the automerge:no-update (expert!) Automatically merge without updates label May 4, 2023
@mergify mergify bot merged commit 9cebd87 into master May 4, 2023
@mergify mergify bot deleted the mfig-exo-attenuator branch May 4, 2023 05:07
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Belated review complete.

Comment on lines +248 to +249
* @param {unknown} [opts.target] The target for any methods that
* weren't specified in `opts.overrides`.
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.

Comment on lines +306 to +308
// @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"/ });
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates patterns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants