Promise for non-Passable is not Passable

[turadg]

Refs: #2406

Description

@mhofman suggested a fix for the FIXME in #2406. As expected it trip on recursion:

packages/pass-style/src/deeplyFulfilled.js:74:32 - error TS2589: Type instantiation is excessively deep and possibly infinite.

74 export const deeplyFulfilled = async val => {
                                  ~~~~~~~~~~~~~~

Leaving this PR in draft until that's solved. @mhofman @michaelfig feel free to push commits.

Security Considerations

Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls?

Scaling Considerations

Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated?

Documentation Considerations

Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users?

Testing Considerations

Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet?

Compatibility Considerations

Does this change break any prior usage patterns? Does this change allow usage patterns to evolve?

Upgrade Considerations

What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed?

Include *BREAKING*: in the commit message with migration instructions for any breaking change.

Update NEWS.md for user-facing changes.

Delete guidance from pull request description before merge (including this!)

[erights]

Promise for non-Passable is not Passable

When I first read this, I thought it was about promises and whether they were passable. Took me a bit to understand that this is actually about the Promise type and the Passable type. Would be good to rephrase to make clear this is not about what might actually happen at runtime.

The fact that a passable unresolved promise might later fulfill to a non-passable value is a significant hazard in our system we need to remain aware of, whatever the type system might claim to the contrary.

[mhofman]

The fact that a passable unresolved promise might later fulfill to a non-passable value is a significant hazard in our system we need to remain aware of, whatever the type system might claim to the contrary.

This is a good point, maybe this point to a mistake in the approach. The Promise (if an expected PassableCap) is Passable, but the result value may be never if it's itself non-Passable.

Edit: Ugh I keep confusion what happens between Passable definition and mapped types of potentially passable.

[mhofman]

I think I found a way to break the recursion issues, albeit with a theoretically breaking change in the type definition of Passable, but in practice I think it's backwards compatible.

I also took the opportunity to further narrow the type of PassableCap: when a promise, it's only a promise for a RemotableObject. I believe it matches actual explicit usages of the type.

I have not tested this change applied to agoric-sdk yet.

[turadg]

tested this change applied to agoric-sdk

We can use Agoric/agoric-sdk#9385 . I've pointed it to this branch.

Results with 017b2c8

• lint-primary
• lint-rest

[erights]

Is there a reason to remove the default?

In general when I write a type-constrained type parameter, I also provide that type constraint as a default binding of the type parameter. Should I stop doing so?

[mhofman]

For generic functions we want to rely on inference. I don't remember exactly why I dropped it, but I think a default doesn't have any advantages here, especially one where the default value is the constraint itself.

For type definitions it's a different story as it allows you to reference the bare type without type parameters.

[erights]

Nice. I'lll just mention that I'm a bit surprised that RemotableBrand is defined by @endo/eventual-send but RemotableObject is defined by @endo/pass-style. No change suggested.

[mhofman]

Yeah they're very different, somewhat orthogonal things. I've been discussing with @michaelfig, and from eventual-send's pov, the target or arguments don't require any specific pass-style. RemotableBrand helps the typing to capture the shape of remote methods (and the shape of local data) without exposing any actual property on the type itself (its storing them in the private fields of a "stamped" class).

RemotableObject is the pass-style brand, which includes the 2 symbol properties (PASS_STYLE and Symbol.toStringTag), denoting a remotable in passables.

[erights]

What does it mean to both type-constrain the type variable AllowPromise to boolean but give it a default of any?

[erights]

Curious why you didn't adopt the convention of the previous two type variables and write

Suggested change

	AllowPromise extends boolean = any,
	P extends Promise = Promise,

?

[erights]

Or, given the other changes

Suggested change

	AllowPromise extends boolean = any,
	P extends PromiseLike = PromiseLike,

[mhofman]

It's a bit of a hack in the type system. I need to conditionally include a PromiseLike in the union, or a never. But the PromiseLike needs to be parametrized with a specific constraint. As such this type parameter cannot be provided as a type (or inferred), but instead is just a flag to say "please allow promises in the union".

The any default is also a hack to make TS bail out when trying to type match. Without it we wouldn't be able to assign a Passable without promises (AllowPromise=false) to ones with it (or really places where we don't care). I don't fully understand why, this is where I ended at experimentally.

[erights]

Where do you make use of this fourth parameter?

[mhofman]

Internally to avoid including PromiseLike in the union of acceptable types inside the PromiseLike when recursing the type.