-
Notifications
You must be signed in to change notification settings - Fork 74
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
feat(ses): export Permits #1981
base: master
Are you sure you want to change the base?
Conversation
eab0fb9
to
ff10068
Compare
This exports the entirety of `src/permits.js` as the `Permits` object via the `tools` export. We are using this information in `lavamoat-tofu` for SES-compat detection, and we would like to keep it up-to-date with SES. There are other ways we could accomplish this, but exporting it is the easiest way for `lavamoat-tofu` to consume it.
ff10068
to
f76918e
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @boneskull and the rest of your teammates on Graphite |
@kumavis This is the "whitelist" that gets pulled into |
I would prefer to avoid making the permits structure part of the SES public API so we can remain at liberty to change its structure and type as need evolves. It exists in its current form because of a host of simplifying assumptions that are not permanently reliable. We recently added support for symbols. We could conceivably need to allow (If we did export it as-is, I would want to export it from a separate module.) This is a case where I would prefer that you copy and paste. If we ejected the permits into another package, so that I would like @erights to weigh in on these considerations. |
I find it useful, moving it into a lower package seems appropriate |
First, a nit.
The @kriskowal 's main point
is true. That is not necessarily incompat with a Some other considerations
Nevertheless, I remain open to exporting via Seems like a good discussion for an upcoming Endo meeting! |
Added to the agenda, though we have demos with Spritely scheduled. I feel strongly that if we export permits, it should be from another package so that we don’t have to ramp the SES major version if its schema changes. That is tricky, though. Harden would not be available in that package. Pervasively shallow- freezing the permits would be a lot of ceremony. |
Did this get discussed already or no? If not, I'll be on the next call |
This did not get discussed. Consider the topic commuted! |
Summary:
However, the permits are acyclic. If we ejected the permits to I’m curious whether @erights would find that safe enough. |
"ejected"? |
fwiw: I have no opinion on how they get exported, but it'd be nice if we could find some mutually-agreed-upon way to do so. 😄 |
Move code currently in |
Rereading with that in mind...
yes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plan of record:
- Create a new package,
@endo/permits
usingscripts/create-package.sh permits
- Move
permits.js
into@endo/permits
undersrc
. - Add a transitive property tree freeze and freeze
permits
. export permits
, import them inses
What about enablements? What other intrinsic enumerations should we consider? |
Description
This exports the entirety of
src/permits.js
as thePermits
object via thetools
export.We are using this information in
lavamoat-tofu
for SES-compat detection, and we would like to keep it up-to-date with SES (LavaMoat/LavaMoat#814).There are other ways we could accomplish this, but exporting it is the easiest way for
lavamoat-tofu
to consume it.Security Considerations
Unknown
Scaling Considerations
No
Documentation Considerations
The
tools
export is not currently documented. Whether this is considered a private API is unknown to me. Assuming it's not considered a private API, this change adds more stuff to the public API which will need to be taken into consideration during versioning.Testing Considerations
I'm not sure it's valuable to test that an export simply exists, but I can add such a test if desired.
Upgrade Considerations
n/a