Implement the latest version of the `defineKind` API #4962

FUDCo · 2022-03-30T06:40:05Z

This implements the newest version of the defineKind API, as laid out in #4905.

FUDCo · 2022-03-30T06:46:12Z

Note: I've marked this as DRAFT because it will not pass CI, because there are a couple of uses of defineKind elsewhere in the source tree that need to be updated to use the new API, and what those uses are doing is complicated and weird and hard to understand. MarkM and I will pair on fixing these on Thursday afternoon, but I wanted to get this PR up so the main substance can be reviewed.

warner · 2022-03-30T07:14:49Z

Does this also add the crosslinks and unweakables?

FUDCo · 2022-03-30T07:28:56Z

Yes. Two separate commits, one for the API overhaul and one for the anti-GC-observability stuff.

I'm assigned as an owner of #4962 , but I don't see much reason for that. I think the `*` line at the top was intended to match things that don't match other lines, but I don't think it works that way. I think it tags me as an owner of every PR.

dckc

I defer to other reviewers.

packages/ERTP/src/purse.js

packages/SwingSet/docs/virtual-objects.md

packages/SwingSet/src/liveslots/virtualObjectManager.js

warner · 2022-04-02T23:59:36Z

packages/SwingSet/src/liveslots/virtualObjectManager.js

@@ -411,6 +449,27 @@ export function makeVirtualObjectManager(
    }
  }

+  function copyMethods(behaviorTemplate) {
+    const obj = {};
+    for (const [name, func] of Object.entries(behaviorTemplate)) {


This enables Symbol-named methods, yeah? I think we want that, at least in the long run, and anyways Far should be the judge of what keys are allowed, so I think Object.entries is exactly right.

I wish. Two factors combine to make this not work well:

Object.entries omits non-enumerable properties

Object literals with symbol-named properties initialize those properties to be non-enumerable.

> x = {[Symbol.asyncIterator]: () => null} { [Symbol(Symbol.asyncIterator)]: [Function: [Symbol.asyncIterator]] } > Object.entries(x) [] > Object.getOwnPropertyDescriptors(x) { [Symbol(Symbol.asyncIterator)]: { value: [Function: [Symbol.asyncIterator]], writable: true, enumerable: true, configurable: true } }

No, forget what I just said. The session I captured above shows that my second bullet is false. The new property is enumerable. But Object.entries omitted it anyway. Which corrected my dim memory: Object.entries only includes string-named properties.

In any case, I agree that Far should know what it accepts and others should make minimal assumptions about what that is. Use Reflect.ownKeys or Object.getOwnPropertyDescriptors in order to minimize such assumptions.

So are we going to explicitly allow symbol-named methods here and leave it to Far to reject them?
What about symbol-named facets?

Thinking about this further, symbol-named facets aren't straightforward, because I need to put them in order and sorting Symbols is a pain. In any event, I don't think there's a use case for this aside from obsessive generality, so let's take the idea of symbol-named facets off the table for now.

On deeper analysis, symbol-named methods are kind of PITA too, though if we think it's worth the effort I can do it. But I really don't think it's worth the effort.

Definitely no symbol-named facets. The record naming facets is a copyRecord, and the properties of a copyRecord must be string-named own enumerable properties, so they're exempt from all these painful differences between ways to enumerate own properties. So this restriction is architectural, not expedient.

symbol-named methods here and leave it to Far to reject them?

Far does not reject symbol-named methods. Nor does it reject methods named by non-enumerable properties.

But we currently have only one case we need, @@asyncIterator, which we could continue to special case for now. If postponing general support for symbol-named methods gets us to MN1 faster, I'm all for postponing. But I think our distributed computational model should eventually support symbol-named methods in general.

If I can put in symbol-named methods in less than, say, an hour's work I'll do it. I suspect I probably can.

A weird thought occurred to me as I was pondering implementation: as long as we do not support symbol-named methods in our over-the-wire message encoding, someone can use symbol-named methods as a kind of private-to-the-local-vat scoping technique. I'm not saying this is a good idea, but I can imagine people doing it. Then, when we turn that feature on, we break their imagined security perimeter. (Even once we support symbols in the method selector portion of a message, I presume they would still be able to use non-registered, non-well-known symbols for this purpose.) This makes me wonder if we should hold off implementing symbol-named methods until we are also implementing symbols as method selectors in messages.

packages/SwingSet/src/liveslots/virtualObjectManager.js

FUDCo · 2022-04-04T23:42:42Z

I think this is now ready for final review.

Note to @michaelfig @Chris-Hibbert @turadg: would you please take a look at packages/run-protocol/src/vaultFactory/vault.js? I had to make fairly invasive changes to update it for the new defineKind API,, which I don't believe changed any of the semantics (it passes all the tests, anyway), but given the radical transformation I prefer somebody take a look who actually understands the substance of the underlying code.

turadg · 2022-04-05T15:20:30Z

would you please take a look at packages/run-protocol/src/vaultFactory/vault.js

Thanks for the ping. I don't see any change in semantics, which is good. I see a separation of selfBehavior and helperBehavior, which is good. I do have some concerns.

Git blame churn. Many lines in this file are being touched. I suggest we have a codeowner of run-protocol be the author of the commit for vault.js so that git blame tracks back. I'm happy to take that on.
Loss of JSDoc. JSDoc (at least in my IDE) is on the value of the object and when a function is composed into a new object (e.g. selfBehavior) it doesn't come along. I think each self behavior function should be nested in the selfBehavior object. Without that the function documentation will be lost in the calling context.
Untyped facets. That can be fixed with,

/**
 * @typedef {{
 *   state: ImmutableState & MutableState,
 *   facets: {
 *     self: import('@agoric/vat-data/src/types').FunctionsMinusContext<typeof selfBehavior>,
 *     helper: import('@agoric/vat-data/src/types').FunctionsMinusContext<typeof helperBehavior>,
 *   },
 * }} MethodContext
 */

Loss of argument names in types. With the type transformation to remove the first parameter, the names of the other parameters become args_0 etc. I don't know what to do about this. Maybe there's a trick to do in FunctionsMinusContext. I'll research this.
Loss of type information within behavior functions. Previously the type of state was declared once and shared in each behavior function because the value was shared. Now the context passed in can be anything unless there's an annotation constraining it. I don't know how to solve this without annotating each function.
Loss of calback types like OfferHandler. This could be solved with a PlusContext partner for MinusContext. E.g. {WithContext<OfferHandler>} below.

agoric-sdk/packages/run-protocol/src/vaultFactory/vault.js

Line 360 in df924b3

/* * @type {OfferHandler} XXX needs a proper type def; this one won't do */

Not all the above has to be solved in this PR. My blocking requests are to:

inline the function definitions to the behavior objects
type facets in MethodContext

packages/ERTP/src/payment.js

packages/run-protocol/src/vaultFactory/vault.js

turadg · 2022-04-05T15:22:56Z

packages/run-protocol/src/vaultFactory/vault.js

-  const collateralBrand = manager.getCollateralBrand();
-  const debtBrand = manager.getDebtBrand();
+// #region Computed constants
+const collateralBrand = ({ state }) => state.manager.getCollateralBrand();


please move the behavior functions within the object that will hold them

I'm unclear what you mean by "the behavior functions". Did you mean inline all the behavior functions or just the two that you highlighted in this comment? Because they weren't all inlined before, by any means.

I did mean all of them. They weren't all inlined in object creation but they were all nested and that's a property that I want to maintain. They also all had JSDoc before and the new object construction breaks that unless they're inlined.

Done! Please give it a look.

Perfect, thank you.

packages/run-protocol/src/vaultFactory/vault.js

warner

Two small changes to make, plus the xsnap-native submodule change needs to not be in this PR. Rebasing onto trunk might clear that out, but if not grab me and we can figure it out.

packages/SwingSet/src/liveslots/virtualObjectManager.js

warner · 2022-04-05T17:43:35Z

packages/SwingSet/src/liveslots/virtualObjectManager.js

+      unweakable.add(state);
+      if (facetNames === null) {
+        const context = { state };
+        unweakable.add(context);


because I just wondered about it again, and it took me a minute to convince myself it was ok, let's add a one-line comment here

# 'context' does not need a linkToCohort because it's held by the bound methods

oh, crud, I got the comment backwards, and it's going to confuse me in its current state

please replace with:

'context' does not need a linkToCohort because it holds the facets (which hold the cohort)

sorry

FUDCo · 2022-04-05T20:31:09Z

How do I expunge the xsnap-native nonsense that somehow got sucked into this PR? Rebasing didn't seem to do the trick.

warner

r+ module that one extra comment change

warner · 2022-04-06T01:34:36Z

packages/SwingSet/src/liveslots/virtualObjectManager.js

+      unweakable.add(state);
+      if (facetNames === null) {
+        const context = { state };
+        unweakable.add(context);


oh, crud, I got the comment backwards, and it's going to confuse me in its current state

please replace with:

'context' does not need a linkToCohort because it holds the facets (which hold the cohort)

sorry

FUDCo · 2022-04-06T04:31:10Z

r+ module that one extra comment change

OK, but I think your prior attempt at the comment was also correct.

Closes #4905

FUDCo added enhancement New feature or request SwingSet package: SwingSet labels Mar 30, 2022

FUDCo requested a review from warner March 30, 2022 06:40

FUDCo self-assigned this Mar 30, 2022

FUDCo requested review from erights, Chris-Hibbert, dckc and michaelfig as code owners March 30, 2022 06:40

FUDCo removed request for dckc, erights, michaelfig and Chris-Hibbert March 30, 2022 06:40

FUDCo marked this pull request as draft March 30, 2022 06:45

dckc mentioned this pull request Mar 30, 2022

chore: remove dckc from * "everything" owner list #4963

Merged

FUDCo force-pushed the 4905-revised-revised-kind-api branch 4 times, most recently from e3d5627 to 688718a Compare April 1, 2022 07:35

dckc reviewed Apr 1, 2022

View reviewed changes

turadg reviewed Apr 1, 2022

View reviewed changes

packages/ERTP/src/purse.js Show resolved Hide resolved

warner reviewed Apr 2, 2022

View reviewed changes

packages/SwingSet/docs/virtual-objects.md Outdated Show resolved Hide resolved

warner reviewed Apr 2, 2022

View reviewed changes

packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved

warner reviewed Apr 2, 2022

View reviewed changes

packages/SwingSet/src/liveslots/virtualObjectManager.js Show resolved Hide resolved

warner reviewed Apr 2, 2022

View reviewed changes

packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved

warner reviewed Apr 2, 2022

View reviewed changes

warner reviewed Apr 3, 2022

View reviewed changes

packages/SwingSet/src/liveslots/virtualObjectManager.js Outdated Show resolved Hide resolved

FUDCo requested review from michaelfig, Chris-Hibbert, erights and warner April 4, 2022 23:39

FUDCo force-pushed the 4905-revised-revised-kind-api branch from 586ac9b to 4dad8be Compare April 5, 2022 00:09

turadg requested changes Apr 5, 2022

View reviewed changes

turadg reviewed Apr 5, 2022

View reviewed changes

packages/run-protocol/src/vaultFactory/vault.js Outdated Show resolved Hide resolved

warner requested changes Apr 5, 2022

View reviewed changes

FUDCo mentioned this pull request Apr 5, 2022

ERTP payment will need proper TS type def for new VO API #5018

Closed

FUDCo force-pushed the 4905-revised-revised-kind-api branch from 4dad8be to 5065fa9 Compare April 5, 2022 20:40

FUDCo requested a review from warner April 5, 2022 20:43

FUDCo force-pushed the 4905-revised-revised-kind-api branch from c6d08ff to 6b728a3 Compare April 6, 2022 01:04

warner approved these changes Apr 6, 2022

View reviewed changes

FUDCo force-pushed the 4905-revised-revised-kind-api branch from 6b728a3 to 1d3d756 Compare April 6, 2022 04:35

FUDCo requested a review from turadg April 6, 2022 04:35

turadg approved these changes Apr 6, 2022

View reviewed changes

FUDCo force-pushed the 4905-revised-revised-kind-api branch from 1d3d756 to 43df7ec Compare April 6, 2022 18:07

FUDCo added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Apr 6, 2022

feat: yet another overhaul of the defineKind API

3e02d42

Closes #4905

FUDCo force-pushed the 4905-revised-revised-kind-api branch from 43df7ec to 3e02d42 Compare April 6, 2022 18:09

FUDCo added the automerge:rebase Automatically rebase updates, then merge label Apr 6, 2022

mergify bot merged commit fd61223 into master Apr 6, 2022

mergify bot deleted the 4905-revised-revised-kind-api branch April 6, 2022 18:24

turadg mentioned this pull request Apr 7, 2022

chore(run-protocol): type facets of vault #5038

Merged

FUDCo mentioned this pull request Apr 26, 2022

vat upgrade by replacing virtual-object behavior record #3062

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the latest version of the `defineKind` API #4962

Implement the latest version of the `defineKind` API #4962

FUDCo commented Mar 30, 2022 •

edited

Loading

FUDCo commented Mar 30, 2022

warner commented Mar 30, 2022

FUDCo commented Mar 30, 2022

dckc left a comment

warner Apr 2, 2022

erights Apr 3, 2022

erights Apr 3, 2022

FUDCo Apr 3, 2022 •

edited

Loading

FUDCo Apr 3, 2022

FUDCo Apr 3, 2022

erights Apr 3, 2022 •

edited

Loading

FUDCo Apr 3, 2022

FUDCo Apr 4, 2022

FUDCo commented Apr 4, 2022

turadg commented Apr 5, 2022

turadg Apr 5, 2022

FUDCo Apr 5, 2022

turadg Apr 5, 2022

FUDCo Apr 6, 2022

turadg Apr 6, 2022

warner left a comment

warner Apr 5, 2022

warner Apr 6, 2022

FUDCo commented Apr 5, 2022 •

edited

Loading

warner left a comment

warner Apr 6, 2022

FUDCo commented Apr 6, 2022

Implement the latest version of the defineKind API #4962

Implement the latest version of the defineKind API #4962

Conversation

FUDCo commented Mar 30, 2022 • edited Loading

FUDCo commented Mar 30, 2022

warner commented Mar 30, 2022

FUDCo commented Mar 30, 2022

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

FUDCo Apr 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights Apr 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FUDCo commented Apr 4, 2022

turadg commented Apr 5, 2022

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

warner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FUDCo commented Apr 5, 2022 • edited Loading

warner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FUDCo commented Apr 6, 2022

Implement the latest version of the `defineKind` API #4962

Implement the latest version of the `defineKind` API #4962

FUDCo commented Mar 30, 2022 •

edited

Loading

FUDCo Apr 3, 2022 •

edited

Loading

erights Apr 3, 2022 •

edited

Loading

FUDCo commented Apr 5, 2022 •

edited

Loading