fix(exo): allow richer behaviorMethods

[erights]

closes: #1817
refs: #1815 #1808 #1773 #1766

Description

The intention of the exo-making methods was to be able to use a JS class' .prototype as a source of raw methods. This PR

• fixes the bugs reported in Exo classes need to handle non-enumerable raw methods correctly. #1817 that prevented use of JS classes as a source of raw methods for an exo class.
• additionally accommodates JS class inheritance so that superclasses can also provide raw classes.
• provides tests that show use of JS classes to construct Exo classes.
• these tests also provide hypothetical helper abstractions for better supporting such use of classes.
  • These helpers remain in tests because this PR merely demonstrates that such support is possible, without committing to the particular helpers to actually provide. We need at least to bikeshed from the currently awful names (defineExoClassFromJSClass).
• these tests demonstrate the use of both super and late-bound self in inheritance among these methods, with super calls bypassing guards, while self calls are guarded.

This PR also provides tests demonstrating the use of JS classes to directly provide unguarded behavior at the @endo/pass-style / Far level of abstraction. This PR makes no changes to the src/ code at this level, demonstrating what support is already possible. Unlike the above exo-level support, we do not recommend the coding style shown in these pass-style level tests. However, neither can we reasonably prevent it, so we need to understand it.

Security Considerations

Corresponding examples at

endo/packages/pass-style/test/test-far-wobbly-point.js

Lines 132 to 144 in d50a60b

	// This test, though correct, demonstrates a sucurity weakness of
	// this approach to JS class inheritance at this
	// `@endo/pass-style` / `Far` level of abstraction. The weakness
	// is that the overridden method from a superclass can nevertheless
	// be directly applied to an instance of the subclass. The
	// subclass override did not suppress the use of the overridden
	// method as, effectively, part of the subclass' instance's public
	// API.
	//
	// See the corresponding example at
	// `test-exo-wobbly-point.js` to see the absence of this vulnerability
	// at the Exo level of abstraction.
	t.is(apply(otherPt.getX, otherWpt, []), 3);

and

endo/packages/exo/test/test-exo-wobbly-point.js

Lines 183 to 193 in d50a60b

	// This error behavior shows the absence of the security vulnerability
	// explained at the corresponding example in `test-far-wobbly-point.js`
	// for the `@endo/pass-style` / `Far` level of abstraction. At the exo level
	// of abstraction, a raw-method subclass overriding an inherited superclass
	// method denies that method to clients of instances of the subclass.
	// At the same time, this overridden method remains available within
	// the overriding subclass via unguarded `super` calls.
	t.throws(() => apply(otherPt.getX, otherWpt, []), {
	message:
	'"In \\"getX\\" method of (ExoPoint)" may only be applied to a valid instance: "[Alleged: ExoWobblyPoint]"',
	});

demonstrates that the demonstrated non-recommended use of class inheritance at the far level of abstraction has a security vulnerability absent from the use of class inheritance at the exo level of abstraction. This is one reason why we do not recommend the former but may recommend the latter.

Scaling Considerations

If classes had large number of methods and class inheritance chains were deep, this has somewhat worse scaling than a JS class programmer would normally expect. However, if either of these are reasonably small, there should not be any significant scaling problem.

Documentation Considerations

If we decide that we like and wish to promote the use of JS classes demonstrated by this PR, we should move some support (defineExoClassFromJSClass) into exported sources and we should document the whole approach. The purpose of this PR is not to establish such practice or decide on its exact shape, but merely to demonstrate that this PR makes such JS class-based practice possible.

In particular, we may be able to improve of the usability shown in this PR by using JS class decorators. It is a worthy experiment. (attn @mhofman )

Testing Considerations

The src changes that this PR makes are only to exo-tools.js code shared among heap, virtual, and durable exos. However, the tests only test the behavior of heap exos. Once agoric-sdk is upgraded to depend on the changes from this PR, we should reproduce these tests for virtual and durable exos. Ideally, we would use Zones to write the tests once and then reuse it to test all three types of zone.

This PR makes no effort to test JS classes used for facets of exo kits. Nor does it provide any hypothetical helper functions to help write such exo kits. If we decide that this code pattern for using JS classes is attractive, a later PR should indeed extend the approach to exo kits.

Upgrade Considerations

This PR makes no changes to data. For existing exo-making code that only uses object literals for own enumerable methods, this PR should not result in any behavior change or even internal representation change. The only code it should make a difference for is code that previously did not work because it needed the fixes and features provided by this PR. We are not aware of any such code prior to this PR.

[erights]

@michaelfig , reviewers,

I rebased and resolved merge conflicts with #1831

[kriskowal]

Aside: If I convince the OCapN folks that method invocations should be case-normalized, then a method named “get interface guard” would only be addressable as get-interface-guard in Guile, get_interface_guard in Python, getInterfaceGuard in JavaScript, GetInterfaceName in Go &c. This is sufficiently far over the horizon and addressable with a non-breaking change (duplicate the method) that it doesn’t make sense to belabor now.

[erights]

At #1809 (review) @michaelfig writes:

Looking good, just some quibbles. I'd approve based on their resolution,

All quibbles resolved. PTAL.

but I have a feeling that some of the other reviewers you tagged should weigh in first.

@turadg @mhofman , Care to? Thanks.

[erights]

Reviewers,
In case @kriskowal is able, before the hackathon, to do a new endo release & upgrade agoric-sdk to depend on it, I'd really like to be able to get this in so I could build on it. Given the reviewing @michaelfig has already done, you may find the remaining review work could be done quickly, perhaps? Just sayin ;) . Thanks.

[turadg]

I raised some issues at the periphery with assertions and documentation.

We need at least to bikeshed from the currently awful names (defineExoClassFromJSClass).

I think "bikeshed" here is facetious but I want to point out that API naming in Endo merits careful consideration. They will stick around a long time and we want to avoid developers being confused by them or having misconceptions about what they do.

That said, I don't think defineExoClassFromJSClass is awful. It's long but it says what it does. It's probably also not the end state for the dev-facing API because I expect defining the class with decorators will be the most common.

[erights]

We need at least to bikeshed from the currently awful names (defineExoClassFromJSClass).

I think "bikeshed" here is facetious but I want to point out that API naming in Endo merits careful consideration. They will stick around a long time and we want to avoid developers being confused by them or having misconceptions about what they do.

I agree completely, which is why these awful names --- as well as the entirety of the defineExoClassFromJSClass helper abstractions --- are in a test/ file rather than a src/ file. I want to put a lot more thought into them before providing them for actual usage. Their purpose in the test is to establish the that src/ changes of this PR enable such things to be possible.

That said, even as a test/ POC of possibility, I'm happy to consider suggested improvements.

[turadg]

No blocking concerns. I do think the underscores are worth considering before merging.

I'd also suggest cleaning up the commits to reduce noise in the changelog. Most of the "fix" are really "fixup" that should be squashed into noteworthy commits.

Btw, if a commit shouldn't appear in the changelog, it shouldn't have conventional commit message. Noise in the commit history isn't as bad as noise in the changelog.

[erights]

I'd also suggest cleaning up the commits to reduce noise in the changelog. Most of the "fix" are really "fixup" that should be squashed into noteworthy commits.

Btw, if a commit shouldn't appear in the changelog, it shouldn't have conventional commit message. Noise in the commit history isn't as bad as noise in the changelog.

Manually squashed those away before merging. Done.