feat(marshal): Allow promise stand-ins #1313

gibson042 · 2022-10-11T19:25:59Z

Stacked on #1311

This is the most restrictive solution, in which a pseudo-promise is not
allowed to have any properties or methods (although that may be relaxed
in the future to support a then method and/or settlement/resolution
data).

TODO:

combine promise and safe-promise helpers (after perf(marshal): Extend new assert style to passStyleOf check calls #1310 )

erights · 2022-10-11T20:22:16Z

@gibson042 since this is stacked on #1311 , I edited the github PR to compare to #1311 's fork for easier reviewability. If this is not appropriate, feel free to change back.

(#1311 has since been merged, so this note is no longer relevant)

mhofman · 2022-10-11T20:38:27Z

packages/marshal/src/helpers/promise.js

+      return;
+    }
+
+    PromiseHelper.canBeValid(candidate, assertChecker);


Seems like we're duplicating the isPromise check if the candidate is not a native promise, which means really we're creating 2 throw away Promise objects.

But only one extra one, since we gotta do at least one. Unless we wanna invert the order of tests, which is likely to be net more expensive.

It's actually even more than that, because passStyleOf itself uses isPromise on every [frozen] object, and then every one that fails the check and does not have an explicit PASS_STYLE is subject to iteration over each helper's canBeValid in priority order, with promise's using isPromise and then each object accepted by that canBeValid is passed to assertValid (which also uses isPromise). I'll reorder to check PASS_STYLE first, but that will still leave two calls for ersatz promises, and if we really care then I think we'll need a WeakMap.

passStyleOf memoizes

mhofman · 2022-10-11T20:39:23Z

packages/marshal/src/helpers/promise.js

+    const descKeys = ownKeys(candidate);
+    (descKeys.length === 1 && descKeys[0] === PASS_STYLE) ||
+      assert.fail(X`Must not have any own properties: ${candidate}`);


I thought that for @FUDCo's use case, we needed at least a toString() method? When discussing this with @erights we didn't see a need to disallow any more methods than for remotables.

Being able to specify the toString() method is literally the only thing that matters to me here. Everything else is just machinery to enable that to work.

... toString() method ...

Cannot do that in any case, and this PR wisely does not attempt to. What do you really need? What are you doing now, so that your current state works? My sense is that you only need a pair of functions which can be local to a context, where

getSomething(makePromiseWithSomething(something)) === something

for some appropriate somethings. We can do that trivially with real promises and a weakmap, yes?

When discussing this with @erights we didn't see a need to disallow any more methods than for remotables.

True. And what I said then contradicts what I said above. There are two simple extremes, and we should do one or the other.

Allow mostly-arbitrary unvalidatable methods, like we do for remotables. This is why remotables can have an arbitrary toString method.

Data-only, which is where this PR currently is, and why I'm trying to avoid any unvalidatable behavior.

The problem is that we are still operating under the "any module might be multiply instantiated" restriction, and so any objective predicate, like passStyleOf, cannot brand things to remember that they are safe by construction. It can only tell that it is safe by inspection. passStyleOf does use a memo table to avoid redundant work, which is much like a brand table. Except that if absent, it always falls back to inspection. The behavior of non-primordial non-global methods cannot be inspected to be safe.

This comes down to a tension between practicality and the principle that unmarshaling should recover full state. We can relax this constraint if the use case is in fact sufficiently strong, but we can't go in the other direction.

erights

Not approving for two reasons:

The requested changes, in particular using checkTagRecord for better validation.
In light of subsequent discussions, I am queasy about introducing something passStyleOf recognizes as a promise type, but which cannot yet work with E( nor E.when(.

Despite the second bullet, please do respond to requested changes, so we'll be closer to moving ahead if/when the second bullet is resolved. There are two needs this PR would be a step towards:

@FUDCo 's immediate need for his encode/decode manipulation in the post-smallcaps SwingSet tests,
@mhofman 's plans for virtual persistent promises.

@FUDCo already has something working. I'd like to see that before judging that use to be urgent. Assuming it is not urgent, I'd like to hold this off until we find something that works with E and E.when, probably as part of doing virtual and durable promises as the same time.

packages/marshal/src/helpers/promise.js

erights · 2022-10-11T20:35:49Z

packages/marshal/src/passStyleOf.js

@@ -20,7 +21,7 @@ import { assertSafePromise } from './helpers/safe-promise.js';
 /** @typedef {import('./types.js').PassStyleOf} PassStyleOf */
 /** @typedef {import('./types.js').PrimitiveStyle} PrimitiveStyle */

-/** @typedef {Exclude<PassStyle, PrimitiveStyle | "promise">} HelperPassStyle */


Whoa! TIL Exclude

I see this is from #1043 which I should have paid more attention to at the time. It surprises me, but it does look good.

No change suggested.

erights · 2022-10-11T20:53:07Z

packages/marshal/src/passStyleOf.js

@@ -195,6 +197,7 @@ export const passStyleOf = makePassStyleOf([
  CopyRecordHelper,
  TaggedHelper,
  RemotableHelper,
+  PromiseHelper,


Shouldn't you also delete

if (isPromise(inner)) { assertSafePromise(inner); return 'promise'; }

from lines 150-153 above?

Only if we also move the prohibition of a then method. It seemed cleaner to keep it at the expense of some redundancy.

erights · 2022-10-11T20:57:02Z

packages/marshal/test/test-marshal-capdata.js

+      serializeBodyFormat: 'capdata',
+    },
+  );
+  const assertRoundTrip = (val, body, slots, description) => {


More better testing once we have arb-passable.js?

Absolutely!

;)

No change suggested for this PR.

packages/marshal/src/helpers/promise.js

erights · 2022-10-12T19:58:01Z

packages/marshal/src/passStyleOf.js

+  // "promise" was a late addition, so we tolerate its absence.
+  if (HelperTable.promise === undefined) {
+    delete HelperTable.promise;
+  }


But it is an addition provided by this same PR. In what scenario could this condition ever happen?

If any code outside of this package calls makePassStyleOf, which is exported. However, there are no examples of that anywhere in endo or agoric-sdk AFAICT... should I remove this leeway?

Where is makePassStyleOf exported? I should not be.

Oh, my mistake! I guess I was looking elsewhere. Fixed.

erights · 2022-10-12T19:59:58Z

packages/marshal/src/helpers/promise.js

    return (
-      (candidate[PASS_STYLE] === 'promise' ||
+      ((passStyleDesc && passStyleDesc.value === 'promise') ||


Aha. Would be vulnerable to the obscure Object.prototype.value = junk arrack on descriptors we just talked about in the endo meeting. (Were you there for that?) However, this is in the marshal package, not the ses package, which should only execute after lockdown, and so is not vulnerable. But amusing nonetheless!

No change suggested.

erights · 2022-10-12T20:04:58Z

packages/marshal/src/helpers/promise.js

-    const proto = getPrototypeOf(candidate);
-    proto === objectPrototype ||
-      proto === null ||
+    checkTagRecord(candidate, 'promise', assertChecker);


Yes, good choice moving checkTagRecord from canBeValid to assertValid.

No change suggested.

erights · 2022-10-12T20:09:03Z

packages/marshal/src/helpers/promise.js

+    // XXX Should this (and TaggedHelper.assertValid) support a null prototype?
+    getPrototypeOf(candidate) === objectPrototype ||
      assert.fail(X`Unexpected prototype for: ${candidate}`);


Good question. Yes, I think both should. But here's a counter-argument that says we shouldn't support it at all, i.e., that we should remove such support even from copyRecords:

Any such non-standard null prototype is not pass invariant. If such a copyRecord is passed through any marshal barrier, the copyRecord that gets unserialized on the other side will always inherit directly from `Object.prototype'.

So, I am still on the fence. Opinions?

I am in favor of removing support for all null-prototype passables unless we have a clear scenario benefiting from them, in which case every check for Object.prototype should be expanded accordingly.

I agree. So no change suggested here. Anytime you'd like to propagate this reform (assuming it doesn't break anything but tests), I'll happily review. Thanks!

Necroing this thread but an argument could be made that a copyRecord should be unmarshalled with a null prototype instead.

Given that we agree on preserving pass-invariance, there are only two solutions:

All CopyRecords inherit from Object.prototype (status quo)

All CopyRecords inherit from null (what we’d all prefer if we could have it everywhere at reasonable cost)

Yes, we could change passStyleOf and unserialize to be consistent with (2). What we cannot reasonably change is requiring everyone to write

harden({ __proto__: null, foo: 3 })

rather than the status quo

harden({foo: 3})

Yes, there are approaches to this notational constraint, like using something instead of harden that either

modifies the argument record in place (like Far does), or

produces a new object derived from the argument object

Altogether, for the way we use CopyRecords everywhere, I don’t think either of these coping strategies are at reasonable notational and explanatory cost. Such a cure would be worse than the disease.

packages/marshal/src/helpers/promise.js

erights · 2022-10-12T20:13:58Z

packages/marshal/src/passStyleOf.js

@@ -20,7 +21,7 @@ import { assertSafePromise } from './helpers/safe-promise.js';
 /** @typedef {import('./types.js').PassStyleOf} PassStyleOf */
 /** @typedef {import('./types.js').PrimitiveStyle} PrimitiveStyle */

-/** @typedef {Exclude<PassStyle, PrimitiveStyle | "promise">} HelperPassStyle */


No change suggested.

packages/marshal/src/passStyleOf.js

erights · 2022-10-12T20:18:38Z

packages/marshal/test/test-marshal-capdata.js

+      serializeBodyFormat: 'capdata',
+    },
+  );
+  const assertRoundTrip = (val, body, slots, description) => {


;)

No change suggested for this PR.

packages/marshal/test/test-marshal-capdata.js

erights · 2022-10-12T20:33:41Z

packages/marshal/test/test-passStyleOf.js

@@ -55,6 +77,89 @@ test('some passStyleOf rejections', t => {
    message: /\[Promise\]" - Must not have any own properties: \["then"\]/,
  });

+  // Pseudo-promise
+  for (const proto of [null, {}]) {
+    const fakeprbadProto = makePseudoPromiseVariant(proto);


This one is genuinely fake. No change suggested.

erights

Were we to go ahead with this, its current state would LGTM

erights · 2022-10-12T23:24:56Z

packages/marshal/test/test-passStyleOf.js

@@ -45,6 +45,12 @@ test('passStyleOf basic success cases', t => {
  t.is(passStyleOf(harden({ then: 'non-function then ok' })), 'copyRecord');
  t.is(passStyleOf(makeTagged('unknown', undefined)), 'tagged');
  t.is(passStyleOf(harden(Error('ok'))), 'error');
+
+  const hairlessError = Error('hairless');


Cool name!

No change suggested.

https://en.wikipedia.org/wiki/No-hair_theorem 🤓

Fixes #1312 This is the most restrictive solution, in which an ersatz promise is not allowed to have any properties or methods (although that may be relaxed in the future to support a `then` method and/or settlement/resolution data).

Better aligns with tag records

Co-authored-by: Mark S. Miller <[email protected]>

…otable Fixes #1318

Ref #1324

Aligns with #1315

mhofman · 2022-10-13T17:56:10Z

packages/marshal/src/helpers/promise.js

+    const tagDesc = getOwnPropertyDescriptor(candidate, toStringTag);
+    (tagDesc && tagDesc.value === 'Pseudo-promise') ||
+      assert.fail(
+        X`Pseudo-promise must be an object with ${q(toStringTag)} ${q(
+          'Pseudo-promise',
+        )}: ${candidate}`,
+      );


This seems overly restrictive. I'm wondering if we may ever want to differentiate between different kinds of pseudo promises in which case toStringTag would be appropriate.

I figure we can relax this constraint if that need arises.

mhofman · 2022-10-13T17:57:12Z

packages/marshal/src/helpers/promise.js

+      );
+    const keys = ownKeys(candidate);
+    keys.every(k => k === PASS_STYLE || k === toStringTag) ||
+      assert.fail(X`Unexpected properties on pseudo-promise ${keys}`);


Suggested change

assert.fail(X`Unexpected properties on pseudo-promise ${keys}`);

assert.fail(X`Unexpected properties on pseudo-promise ${keys.filter(k => k !== PASS_STYLE && k !== toStringTag)}`);

Done with the ...rest pattern used elsewhere.

erights · 2022-11-18T23:52:40Z

@mhofman I just created the label pseudo-promise and labeled this PR with that label, to keep track of PRs and issues relevant for thinking about possible pseudo-promise semantics.

gibson042 requested review from mhofman, erights and FUDCo October 11, 2022 19:25

erights mentioned this pull request Oct 11, 2022

fix(marshal): Increase consistency for encoding/decoding CapData #1311

Merged

erights changed the base branch from master to gibson-2022-10-marshal-assertions October 11, 2022 20:19

mhofman reviewed Oct 11, 2022

View reviewed changes

erights requested changes Oct 11, 2022

View reviewed changes

erights reviewed Oct 11, 2022

View reviewed changes

packages/marshal/src/helpers/promise.js Outdated Show resolved Hide resolved

erights mentioned this pull request Oct 11, 2022

marshal: Allow promise stand-ins #1312

Open

gibson042 force-pushed the gibson-2022-10-marshal-assertions branch from 08b7e45 to c206d14 Compare October 11, 2022 23:59

gibson042 force-pushed the gibson-1312-passstyleof-promise branch from 6dff770 to b016e59 Compare October 12, 2022 00:34

erights reviewed Oct 12, 2022

View reviewed changes

gibson042 mentioned this pull request Oct 13, 2022

fix(marshal): Check if input is an error before checking if it is remotable #1323

Merged

gibson042 force-pushed the gibson-2022-10-marshal-assertions branch from c206d14 to 7f0aa17 Compare October 13, 2022 15:11

Base automatically changed from gibson-2022-10-marshal-assertions to master October 13, 2022 15:18

gibson042 mentioned this pull request Oct 13, 2022

marshal: null should not be accepted in place of Object.prototype #1324

Closed

gibson042 force-pushed the gibson-1312-passstyleof-promise branch from 3bcc377 to 389d4c7 Compare October 13, 2022 15:28

gibson042 and others added 11 commits October 13, 2022 11:53

feat(marshal): Allow promise stand-ins

c22d935

Fixes #1312 This is the most restrictive solution, in which an ersatz promise is not allowed to have any properties or methods (although that may be relaxed in the future to support a `then` method and/or settlement/resolution data).

fix(marshal): Respond to PR feedback

f94f9a8

fix(marshal): Tighten pseudo-promise constraints

0bded79

Better aligns with tag records

refactor(marshal): Simplify check

d6cc7c6

Co-authored-by: Mark S. Miller <[email protected]>

fix(marshal): Check if input is an error before checking if it is rem…

778e9d9

…otable Fixes #1318

chore(marshal): Remove references to "ersatz promise"

76a8a9a

chore(marshal): Resolve prototype requirement

ed88bd3

Ref #1324

chore(marshal): Require a "promise" passStyleOf helper

8b1692b

refactor(marshal): Merge passStyleOf safe-promise and promise helpers

3c6afec

refactor(marshal): Update promise helper exports

afa9ffe

refactor(marshal): Avoid calling isPromise unnecessarily

2bf623c

perf(marshal): Skip assert.details calls where output will be ignored

397a2bf

Aligns with #1315

gibson042 force-pushed the gibson-1312-passstyleof-promise branch from 4c41e58 to 397a2bf Compare October 13, 2022 17:35

gibson042 requested review from erights and mhofman October 13, 2022 17:46

mhofman reviewed Oct 13, 2022

View reviewed changes

gibson042 added 2 commits October 13, 2022 23:24

chore(marshal): Only report unexpected keys

8aa7618

fix(marshal): Only allow async_hooks data properties (not accessors)

df0df16

erights added the pseudo-promise Virtual, Durable, or Passable promise-likes label Nov 18, 2022

gibson042 marked this pull request as draft March 29, 2023 16:46

	assert.fail(X`Unexpected properties on pseudo-promise ${keys}`);
	assert.fail(X`Unexpected properties on pseudo-promise ${keys.filter(k => k !== PASS_STYLE && k !== toStringTag)}`);

feat(marshal): Allow promise stand-ins #1313

Are you sure you want to change the base?

feat(marshal): Allow promise stand-ins #1313

Conversation

gibson042 commented Oct 11, 2022 • edited Loading

erights commented Oct 11, 2022 • edited Loading

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 Oct 11, 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 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

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 Feb 14, 2023 • edited Loading

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 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

erights commented Nov 18, 2022

gibson042 commented Oct 11, 2022 •

edited

Loading

erights commented Oct 11, 2022 •

edited

Loading

erights Oct 11, 2022 •

edited

Loading

erights Feb 14, 2023 •

edited

Loading