Agoric / layer-cake

Class/traits-like composition of objects-as-closures
Apache License 2.0
8 stars 4 forks source link

Topics currently implicitly solved #5

Open DavidBruant opened 4 years ago

DavidBruant commented 4 years ago

If there is an intention to start using layer-cake i feel a couple of topics need to be made explicit (at least in terms of intention being documented in the open and likely a couple of automated tests)

getter/setters

Are these meant to be composed? Currently they are by the use of Object.getOwnPropertyDescriptors, but there are no automated tests on this topic, so it's not clear whether it's intentional and meant to stay

self mutability during object construction

In the PR, the final object is passed to layerFun as is and is mutable It's not clear to me how much this is expected or a concern from a security point of view

final object mutability

The object returned from makeClassCake and makeTraitCake is mutable (at least in the sense of property addition) Again, it's not clear to me whether this is intentional However, i'm not sure it's necessarily a choice this lib should be making. Maybe this choice should be left to the consumer

erights commented 4 years ago

On getter/setter. Yes. The use of descriptors is on purpose, intended to support accessor properties by copying the descriptors, rather than calling the getter.

erights commented 4 years ago

On self mutability: It is a concern. In light of your question, I went back and looked, and it is in fact a bit worse than I thought. The layerFuns get the finalObject in a non-frozen form and cannot be prevented from mucking it up. The intent is that non-interference policy be expressed by the combineFunction, but it really can't do much about that. Altogether, I think it is fine, but should probably worry about it more. Nevertheless, you should proceed.

erights commented 4 years ago

On final object mutability: I originally wrote this without any non-test use of harden because it started as an expository experiment I didn't want to make too specific for our uses. Now that it is specific to us, I think there a several places we should put in harden calls:

erights commented 4 years ago

These will bring this code closer to being valid Jessie. However, neither before or after this PR can be valid Jessie by current Jessie rules. For the before, because Jessie has no generators. For the after, because finalObject is being passed around in a non-frozen state. Also, the whole descriptor notion is outside Jessie, which applies both before and after.

Although we should still keep this code as close to being in Jessie as we can, I think the appropriate level of ambition is that it be in SES but made available to Jessie, in the same way we expect to keep Proxy and Reflect out of Jessie, but to use them in SES to provide to Jessie a Jessie-friendly membrane abstraction. The key thing is that the layer code, and the call to makeClassCake and makeTraitCake for composing these layers, can all be in Jessie. I do not include makeCakeMaker because interesting confineLayers are likely not in Jessie.

DavidBruant commented 4 years ago

The layerFuns get the finalObject in a non-frozen form and cannot be prevented from mucking it up. The intent is that non-interference policy be expressed by the combineFunction, but it really can't do much about that.

I see a possible solution: finalObject could be split into a target and a proxy. The lib has access to the target and can mutate it directly to combine the layers. The lib passes to each layer function only the proxy. If a layer tries to mutate the proxy directly (as opposed to via method calls), the proxy throws

DavidBruant commented 4 years ago

On getter/setter. Yes. The use of descriptors is on purpose, intended to support accessor properties by copying the descriptors, rather than calling the getter.

Sounds good, keeping the issue open to remember to add automated tests for this

DavidBruant commented 4 years ago

@erights I see another concer which is currently not explciitely covered in test cases: In case of naming conflicts, class-based cake favors the top-most layer. However, the case for trait-base cake is less clear to me. When there is a conflict, should makeTraitCake throw? It currently does via Object.defineProperties which throws when trying to change the value of a non-configurable property

erights commented 4 years ago

The intent of this simple makeTraitCake is indeed that it should throw; thereby enforcing a policy that traits are completely disjoint. That is the right policy for the use we're currently making of trait-like composition.