Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solution without generators #4

Merged
merged 4 commits into from
Jan 9, 2020
Merged

Conversation

DavidBruant
Copy link
Contributor

I tried it mostly for fun
Sharing the result in case it's useful
All tests pass, but i'm not sure there are enough tests to consider this solution valid

Fixes #3

@Chris-Hibbert
Copy link

Thanks, David! I like the style of invocation here much better than the generator style. I'll leave it to MarkM to opine on whether this is secure in the way he's looking for and other language-level issues.

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM++!!

After the first skim I was concerned whether you preserved the security properties of the original. You did exactly. Your's is superior in every way.

The reason I avoided this style, and set about looking for another way, was because I knew that the hypothetical future Jessie static verifier would have a problem with the shared mutable object. However, to "solve" this problem, I turned to generators, which is definitely not in Jessie. If we're going to make special cases in the Jessie verifier to allow layer-cakes, I'd rather find special cases to support your pattern than mine.

Good!

@erights
Copy link
Member

erights commented Dec 10, 2019

Could you also update the README to match? Thanks.

@DavidBruant
Copy link
Contributor Author

After the first skim I was concerned whether you preserved the security properties of the original

Is there a way to capture all or some of these in the form of one or a couple of unit tests?

Could you also update the README to match?

ok, i'll do that

@DavidBruant DavidBruant requested a review from erights December 27, 2019 17:07
@DavidBruant
Copy link
Contributor Author

I'm not fully sure about the wording of "intermediate function"

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very close to me ;)

README.md Outdated

## The Objects-as-closures Pattern

The classic object or object-oriented programming has public methods and private instance variables. There are several ways to encode this in JavaScript. The following code uses JavaScript's class syntax with the new support for private instance variables.
The classic object or object-oriented programming has public methods and private instance variables. There are several ways to encode this in JavaScript. The following code uses JavaScript's class syntax with the [new support for private instance variables](https://github.com/tc39/proposal-private-methods).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That link is specifically private methods, rather than private state variables. Use https://github.com/tc39/proposal-class-fields#private-fields instead.

README.md Outdated
@@ -91,9 +91,9 @@ function makeWobblyPoint(x, y, wobble) {

The pattern above has more of the flexibility associated with traits or mixins. Each layer is expressed separately. The layers are then combined by a distinct maker function `makeWobblyPoint`. Different making functions can combine overlapping sets of layers in different manners.

The code for expressing the combinable layers is written in the peculiar manner shown above, using generators. We introduce this peculiar generator pattern to solve a hard problem: The code in each layer needs to capture a lexical variable that refers to the object as a whole. However, the object-as-a-whole is not yet assembled, and will be assembled only by a distinct piece of code written in a distinct scope.
The code for expressing the combinable layers is written in the peculiar manner shown above, using intermediate function. We introduce this peculiar pattern to solve a hard problem: The code in each layer needs to capture a lexical variable that refers to the object as a whole. However, the object-as-a-whole is not yet assembled, and will be assembled only by a distinct piece of code written in a distinct scope.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using an intermediate

The object-as-a-whole explanation is no longer compelling because we're not assembling the object before binding it. There is no such hard problem.

README.md Outdated

In the generator function pattern shown above, the methods of a given layer are defined in the scope of variables such as `self` and `supr` that are bound on the left of the `yield`. These are bound to values extracted from the value that the `yield` returns. However, before that happens, the generator yields the argument to `yield`, the object containing that layer's methods, to be combined to make the object itself. The helper function `makeClassCake` exported by this repository first runs through the list of generators it is given, extracting the layer functions from each, combining them into the overall object. Only when the object is complete does it go back to these generators, in order to bind that object to each layer's `self` variable.
In the pattern shown above, the methods of a given layer are defined in the scope of variables such as `self` and `supr` that are provided as arguments to the intermediate function. The helper function `makeClassCake` exported by this repository first runs through all the intermediate functions it is given, extracting the layer functions from each, combining them into the overall object. Only when the object is complete does it go back to these generators, in order to bind that object to each layer's `self` variable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the same reason, this paragraph doesn't really make sense. makeClassCake doesn't first extract anything. Also, no generators.

suprs.push(nextSupr)
}

return finalObject;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Harden. See #5

function* BasePointLayer(x, y) {
const [self] = yield {
function BasePointLayer(x, y) {
return self => ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Harden the returned record. See #5

function* WobblyPointLayer(wobble) {
const [_self, supr] = yield {
function WobblyPointLayer(wobble) {
return (_self, supr) => ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise. And below.

}),
);
function AbstractPointLayer(x, y) {
return self => harden({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're only testing the hardening cases, you can just remove redundant cases.

@DavidBruant DavidBruant requested a review from erights December 31, 2019 10:23
@DavidBruant
Copy link
Contributor Author

@erights ping (in case you haven't seen it)

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@DavidBruant
Copy link
Contributor Author

i'm realizing now i do not have the rights to merge a PR on this repo...

@michaelfig michaelfig merged commit 777313d into Agoric:master Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generator-less alternative proposal
4 participants