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.

Sign up for GitHub

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

Jump to bottom

Enable enforcement that far must be declared #3525

Merged
merged 3 commits into from
Aug 9, 2021
Merged

Enable enforcement that far must be declared #3525

merged 3 commits into from
Aug 9, 2021

Conversation

erights
Copy link
Member

@erights erights commented Jul 25, 2021 •
edited
Loading

Completes #3504

Fixes #2018

As of this PR, we still default to not enforcing, so there should not be any compat problem with dapps. However, this PR enables turning the enforcement on with an environment variable.

@erights erights self-assigned this Jul 25, 2021
@erights erights changed the base branch from master to 2018-markm-far-must-be-declared July 25, 2021 19:09
@erights erights mentioned this pull request Jul 25, 2021
Merged
@erights erights requested a review from michaelfig July 25, 2021 19:21
Base automatically changed from 2018-markm-far-must-be-declared to master July 25, 2021 19:29
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch 2 times, most recently from db9b191 to ed410d0 Compare July 26, 2021 06:49
@erights erights changed the title markm enforce far must be declared Enforce far must be declared Jul 26, 2021
michaelfig
michaelfig requested changes Jul 26, 2021
View reviewed changes
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

I'm putting this "Request changes" in place to remind us not to merge this until we can have a more detailed audit of dapp code.

But it's still a laudable goal to try to get tests to pass.

@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch from ed410d0 to 0adb9b3 Compare July 27, 2021 00:49
@erights erights changed the base branch from master to markm-far-functions July 27, 2021 00:49
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch from 0adb9b3 to 7650cd0 Compare July 27, 2021 02:42
Base automatically changed from markm-far-functions to master July 27, 2021 03:10
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch 4 times, most recently from f68a85e to 85a90b9 Compare July 29, 2021 13:20
@erights erights changed the base branch from master to markm-newly-missing-fars July 29, 2021 13:21
@erights erights mentioned this pull request Jul 29, 2021
Merged
Base automatically changed from markm-newly-missing-fars to master July 29, 2021 17:16
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch 2 times, most recently from 0924274 to 308a58e Compare July 29, 2021 20:33
@erights erights mentioned this pull request Jul 29, 2021
Closed
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch 2 times, most recently from 6ffb2c5 to b7d8cc3 Compare July 31, 2021 21:43
@erights erights mentioned this pull request Aug 1, 2021
Merged
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch 2 times, most recently from 9336739 to 9be3ee7 Compare August 6, 2021 00:45
@erights erights mentioned this pull request Aug 6, 2021
Merged
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch from 9be3ee7 to 0ae998e Compare August 6, 2021 02:03
@erights erights changed the base branch from master to markm-oops-new-stores-forgot-far August 6, 2021 02:04
Base automatically changed from markm-oops-new-stores-forgot-far to master August 6, 2021 03:40
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch from 0ae998e to 6a1a69b Compare August 6, 2021 16:51
@erights erights requested a review from michaelfig August 6, 2021 20:17
@erights
Copy link
Member Author

erights commented Aug 6, 2021

@michaelfig I changed it to an environment variable, kinda. PTAL.

@erights erights requested a review from kriskowal August 6, 2021 20:24
@erights
Copy link
Member Author

erights commented Aug 6, 2021

@kriskowal I added you as a reviewer so you could evaluate the suitability and paranoia of this way of handling environment options / static configuration parameters. And the suitability for eventual migration into Endo.

@erights erights changed the title Enforce far must be declared Enable enforcement that far must be declared Aug 6, 2021
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch from 8e7e108 to 9ec15f8 Compare August 6, 2021 22:45
kriskowal
kriskowal requested changes Aug 6, 2021
View reviewed changes
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

I suggest that we use a sub-shape of Node.js process regardless of execution environment because it’s trivial to endow, greatly improves forward portability, and anticipates the need to eventually thread arguments into Compartments, particularly Endo “agent” compartments, as well.

This is a suggestion but I feel strongly enough about it to press the request changes button.

packages/marshal/src/helpers/environment-options.js Outdated Show resolved Hide resolved
packages/marshal/src/helpers/environment-options.js Outdated Show resolved Hide resolved
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch from 9ec15f8 to 4720ada Compare August 7, 2021 20:58
@erights
Copy link
Member Author

erights commented Aug 7, 2021

I suggest that we use a sub-shape of Node.js process regardless of execution environment because it’s trivial to endow, greatly improves forward portability, and anticipates the need to eventually thread arguments into Compartments, particularly Endo “agent” compartments, as well.

This is a suggestion but I feel strongly enough about it to press the request changes button.

@kris Done. PTAL.

@erights erights requested a review from kriskowal August 7, 2021 22:01
@erights erights force-pushed the 2018-markm-enforce-far-must-be-declared branch from d926fdb to 4653b7f Compare August 9, 2021 22:20
@erights erights enabled auto-merge (squash) August 9, 2021 22:23
@erights erights disabled auto-merge August 9, 2021 22:38
michaelfig
michaelfig approved these changes Aug 9, 2021
View reviewed changes
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM! Minor doc fixes, if desirable.

packages/marshal/src/helpers/environment-options.js Outdated Show resolved Hide resolved
packages/marshal/src/helpers/environment-options.js Outdated Show resolved Hide resolved
@erights erights mentioned this pull request Aug 9, 2021
Open
@erights erights enabled auto-merge (squash) August 9, 2021 23:00
@erights erights merged commit 9408242 into master Aug 9, 2021
@erights erights deleted the 2018-markm-enforce-far-must-be-declared branch August 9, 2021 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelfig michaelfig michaelfig approved these changes

@kriskowal kriskowal kriskowal approved these changes

Assignees

@erights erights

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Empty objects should be data, Far should be marked as such
3 participants
@erights @kriskowal @michaelfig