feat: support M.raw() in method guards

[michaelfig]

closes: #1725 closes: #1772
refs: #1724

Description

Complete the #1724 experiment, as well as using the defaultGuards option (originally discussed in #1725 (comment)) instead of boolean flags for sloppy and raw.

Security Considerations

The new M.raw() guard is only honoured by arguments and return values in defendSyncMethod, so it should not leak elsewhere as a pattern.

Scaling Considerations

Some additional overhead in redacting raw arguments (in defendSyncArgs), but work is minimized by calculating relevant data structures while wrapping methods before any exos are created (in defendSyncMethod).

Documentation Considerations

Testing Considerations

Upgrade Considerations

Backward compatibility is implemented for interfaceGuards created using the legacy sloppy parameter.

[erights]

Looks great! But the klass: '...' issue must first be fixed. See the AwaitArgGuard as an example guide.

[erights]

Since this property is new, consider whether it should be moved into the optional part of this splitRecord pattern. But if you are confident we don't need to, then don't.

[michaelfig]

Thanks, I reintroduced sloppy and moved defaultGuards down to remain compatible.

[erights]

Actually, you introduced defaultGuards in the mandatory section. If we get away with that, then we know this new code never encounters any InterfaceGuardPayloads created by code before this PR. That's great! If we believe that, then I think you can simply delete the optional sloppy: below. Otherwise, I think both sloppy: and defaultGuards: need to go into the optional section.

[michaelfig]

I think we need both sloppy and defaultGuards in the optional section.

[michaelfig]

All comments are addressed, and CI is passing. Ready for rereview!

[gibson042]

Even reading the linked issues, I'm not clear on the motivation for introducing this non-Passable "raw" argument/return and unknown-method functionality, and am wondering if it might be better to instead establish a convention for e.g. conveying mutable objects via thunks and/or introduce specific configuration for supporting unknown methods. Can you explain the need?

[gibson042]

https://github.com/Agoric/agoric-sdk/blob/master/packages/SwingSet/docs/virtual-objects.md draws a distinction between an exo object (therein a "VDO") and a facet, so self: Facet<M> seems wrong. I think the "Facet" type should be renamed accordingly, perhaps to something like "Guarded"?

[gibson042]

Clarifying suggestion (but note that at this point I am not clear on the specifics of what "redaction" is doing here at all):

Suggested change

	const defendSyncArgs = (syncArgs, redactConfig, label = undefined) => {
	const {
	declaredLen,
	hasRestArgGuard,
	paramsPattern,
	rawRestArgs,
	redactedIndices,
	} = redactConfig;
	const defendSyncArgs = (syncArgs, redactionConfig, label = undefined) => {
	const {
	declaredLen,
	hasRestArgGuard,
	paramsPattern,
	rawRestArgs,
	redactedIndices,
	} = redactionConfig;

[gibson042]

I don't think this Math.min is doing anything; slice already clamps its second argument to the length of the array.

Suggested change

	redactedArgs = syncArgs.slice(0, Math.min(syncArgs.length, declaredLen));
	redactedArgs = syncArgs.slice(0, declaredLen);

[gibson042]

The redactedArgs name seems a little misleading, implying e.g. "those arguments that have been redacted". Maybe instead matchableArgs?

Also, I question the value of completely ignoring raw rest args here—it's probably better to preserve their presence:

  // Use syncArgs if possible, but copy it when necessary to implement redactions.
  let matchableArgs = syncArgs;
  if (rawRestArgs && syncArgs.length > declaredLen) {
    const restLen = syncArgs.length - declaredLen;
    const redactedRest = Array(restLen).fill(REDACTED_RAW_ARG);
    matchableArgs = [...syncArgs.slice(0, declaredLen), ...redactedRest];
  } else if (redactedIndices.length > 0 && redactedIndices[0] < syncArgs.length) {
    matchableArgs = [...syncArgs];
  }

  for (const i of redactedIndices) {
    if (i >= matchableArgs.length) {
      break;
    }
    matchableArgs[i] = REDACTED_RAW_ARG;
  }

  mustMatch(harden(redactedArgs), paramsPattern, label);

[gibson042]

Suggested change

	rawRestArgGuard = undefined;
	rawRestArgGuard = REDACTED_RAW_ARG;

[michaelfig]

This needed some finessing, after replacing undefined with the only pattern that could possibly work: M.arrayOf(REDACTED_RAW_ARG).

[gibson042]

Ah, better yet then:

Suggested change

	rawRestArgGuard = undefined;
	matchableRestArgGuard = M.any();

[michaelfig]

If we're going through the effort to create rest arguments that are each REDACTED_RAW_ARG, I'd rather do the stricter arrayOf pattern. Without being strict, I see no compelling reason to do anything other than just drop raw rest arguments rather than doing this whole match-but-bypass dance.

[gibson042]

Suggested change

	const redactConfig = redactRawArgs(methodGuardPayload);
	const redactionConfig = toRedactionConfig(methodGuardPayload);

[gibson042]

Suggested change

	* In return position, return the result without any hardening or ßchecking.
	* In return position, return the result without any hardening or checking.

[michaelfig]

Even reading the linked issues, I'm not clear on the motivation for introducing this non-Passable "raw" argument/return

Exos used to be tightly coupled to @endo/pass-style, so that it was impossible to define an exo method that either took a non-Passable as an argument or returned a non-Passable. The usage I am working towards is an exo-promise that allows settler.resolve(anyValueAtAll). There is no way to express such a method on an exo class, since the old most lenient argument guard (M.any()) was to harden(anyValueAtAll) and then assert it was passable.

and unknown-method functionality,

We already had the unknown-method functionality called sloppy, and @erights proposed extending it to allow the specification of sloppy-passable methods, as well as sloppy-raw methods. I did the implementation in this PR, even though I have no particular use for the functionality at this point, and prefer the "raw" argument/return annotations since they are stricter.

and am wondering if it might be better to instead establish a convention

I want to be able to create exo objects or facets whose methods can be just as flexible as a vanilla JS function, but have that be an opt-out so that the Exo model of guards and patterns is the default behaviour.

for e.g. conveying mutable objects via thinks

Funny you should mention that. Thunks are non-passable, too!

and/or introduce specific configuration for supporting unknown methods.

The specific configuration is to explicitly specify M.interface('Foo', { meth: M.call().rest(M.raw()).returns(M.raw()) }). Arguably, we could lean in further and do something like M.interface('Foo', { meth: M.callRaw() }) too.

Can you explain the need?

I don't want to make breaking design changes to contracts or libraries that I'm converting to be Exo-objects. Especially when so much of their behaviour fits into the Exo-object model already. M.raw() makes that possible. The unknown method support is tangential but related, and I don't really care as much about it.

[erights]

What does this no-effect statement do?

[erights]

Why is null here correct? The original

Suggested change

	const context = { state, facets: null };
	const context = { state, facets: {} };

seems more correct.

[michaelfig]

I intended for the null to make it explicit that facets is not yet initialized. I could have used undefined instead, but I think {} implies that the identity will be kept intact (which it isn't). The code below replaces it with the actual facets object.

[erights]

LGTM!

But especially curious about facets: null. Am I missing something?

[gibson042]

Even reading the linked issues, I'm not clear on the motivation for introducing this non-Passable "raw" argument/return

Exos used to be tightly coupled to @endo/pass-style, so that it was impossible to define an exo method that either took a non-Passable as an argument or returned a non-Passable. The usage I am working towards is an exo-promise that allows settler.resolve(anyValueAtAll). There is no way to express such a method on an exo class, since the old most lenient argument guard (M.any()) was to harden(anyValueAtAll) and then assert it was passable.

I think that just pushes the question back a level... what is the underlying motivation for non-Passable promise resolution? M.raw() seems like a reasonable way to get a pattern representing literally any value, and defaultGuards "passable" vs. "raw" seems good for its propagation through defineExo{Class,ClassKit}, but I'm still wondering why we need to do anything at all here.

for e.g. conveying mutable objects via thinks

Funny you should mention that. Thunks are non-passable, too!

Are they?

import "@endo/init";
import { Far } from "@endo/far";
import { M, matches } from "@endo/patterns";

const fn = Far("fn", () => "returned");
fn();
// => "returned"
matches(fn, M.remotable());
// => true

I don't want to make breaking design changes to contracts or libraries that I'm converting to be Exo-objects.

Ah, this seems like it's heading in the direction I care about. What is being converted to exo, what is the benefit, and how do they currently interact with non-Passable values?

[gibson042]

Why allow "never"? I think we would normally use undefined to indicate an unchanged default, and certainly not a word that could be misinterpreted as meaning something like "even less than raw".

Suggested change

	* @typedef {'never' | 'passable' | 'raw'} DefaultGuardType
	* @typedef {undefined | 'passable' | 'raw'} DefaultGuardType

[gibson042]

Clarity/repository consistency nit:

Suggested change

	* @template {{}} T
	* @template {object} T

(details)

$ patt='[{]([{][}]|object)[}]'
$ git grep -liE "$patt" packages/ | xargs grep --no-filename -oE "$patt" | sort | uniq -c
      9 {{}}
    187 {object}

[michaelfig]

They are not the same. Object.create(proto) is a type error unless proto extends {}.

[gibson042]

Suggested change

	* @typedef {{}} RawGuardPayload
	* @typedef {object} RawGuardPayload

[michaelfig]

This time, I'm specifying that RawGuardPayload is the empty object, not just anything of type object.

[gibson042]

Ah, better yet then:

Suggested change

	rawRestArgGuard = undefined;
	matchableRestArgGuard = M.any();

[gibson042]

With the above suggestion to replace "never" with undefined, this could also be simplified.

Suggested change

	* options: {defaultGuards?: 'passable' | 'raw', sloppy?: true }) => InterfaceGuard<any>
	* options: {defaultGuards?: DefaultGuardType, sloppy?: true }) => InterfaceGuard<any>

[michaelfig]

Can't really simplify, because it has special meaning as a function overload.

[gibson042]

👍 👍 for adding this.

Suggested change

	* In parameter position, guard a parameter by awaiting it. Can only be used in
	* parameter position of an `M.callWhen`.
	* `M.await(M.nat())`, for example, with `await` the corresponding argument,
	* check that the fulfillment of the `await` satisfies the `M.nat()`
	* pattern, and only then proceed to call the raw method with that fulfillment.
	* If the argument already passes the `M.nat()` pattern, then the result of
	* `await`ing it will still pass, and the `M.callWhen` will still delay the
	* raw method call to a future turn.
	* If the argument is a promise that rejects rather than fulfills, or if its
	* fulfillment does not satisfy the nested pattern, then the call is rejected
	* without ever calling the raw method.
	* Guard a positional parameter in `M.callWhen`, awaiting it and matching its
	* fulfillment against the provided pattern.
	* For example, `M.callWhen(M.await(M.nat())).returns()` will await the first
	* argument, check that its fulfillment satisfies `M.nat()`, and only then call
	* the guarded method with that fulfillment. If the argument is a non-promise
	* value that already satisfies `M.nat()`, then the result of `await`ing it will
	* still pass, and `M.callWhen` will still delay the guarded method call to a
	* future turn.
	* If the argument is a promise that rejects rather than fulfills, or if its
	* fulfillment does not satisfy the nested pattern, then the call is rejected
	* without ever invoking the guarded method.

[michaelfig]

Credit to @erights for the addition. I just cherry-picked it. 😊

[gibson042]

Is it possible to define these types in a way that doesn't require so much repetition?

/**
 * @typedef {MethodGuardOptMaker & MethodGuardRestMaker & MethodGuardFinalizer} MethodGuardMaker
 * A method name and parameter/return signature like:
 * ```js
 *   foo(a, b, c = d, ...e) => f
 * ```
 * should be guarded by something like:
 * ```js
 * {
 *   ...otherMethodGuards,
 *   foo: M.call(AShape, BShape).optional(CShape).rest(EShape).returns(FShape),
 * }
 * ```
 */

/**
 * @typedef {object} MethodGuardOptMaker
 * @property {(...optArgGuards: ArgGuard[]) => (MethodGuardRestMaker & MethodGuardFinalizer)} optional
 * Optional arguments not guarded with `M.raw()` are automatically hardened and
 * must be Passable.
 */

/**
 * @typedef {object} MethodGuardRestMaker
 * @property {(restArgGuard: SyncValueGuard) => MethodGuardFinalizer} rest
 * If the rest argument guard is not `M.raw()`, all rest arguments are
 * automatically hardened and must be Passable.
 */

/**
 * @typedef {object} MethodGuardFinalizer
 * @property {(returnGuard?: SyncValueGuard) => MethodGuard} returns
 * If the return guard is not `M.raw()`, the return value is automatically
 * hardened and must be Passable.
 */

[michaelfig]

Thanks, I like that! Usually, my DRY factor is 2, but I am more lenient sometimes when I haven't seen the best way to factor.

[michaelfig]

const fn = Far("fn", () => "returned");
fn();
// => "returned"
matches(fn, M.remotable());
// => true

Oh, I misspoke when I said "thunks are non-passable". I meant to say "passable thunks are not consistently supported in SwingSet and other random parts of our system, so I wouldn't count on using them for anything important until they have had significantly more testing."

Ah, this seems like it's heading in the direction I care about. What is being converted to exo, what is the benefit, and how do they currently interact with non-Passable values?

IBC/Network vats and Pegasus are being made durable and upgradable. I'm trying to do a minimal implementation of upgrade-tolerant promise-like remotables, and that's the last in a long line of trying to do promise-like remotables and bashing my head into the ceiling imposed by the passable restrictions. This PR is the result of trying to raise that ceiling so that I can use Zones and Exo-objects for durability as advertised.