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

Confusion about which assert to endow a compartment with #9515

Closed
erights opened this issue Jun 15, 2024 · 0 comments · Fixed by #9514
Closed

Confusion about which assert to endow a compartment with #9515

erights opened this issue Jun 15, 2024 · 0 comments · Fixed by #9514
Assignees
Labels
bug Something isn't working

Comments

@erights
Copy link
Member

erights commented Jun 15, 2024

The @endo/errors package assumes that globalThis.assert is the unstructured assert created by the ses package's assert.js module. @endo/errors uses that to build and export a more structured assert which it exports by that name, together with some other exports (e.g., makeError, annotateError) which it derives from the unstructured globalThis.error which it finds. Fortunately, @endo/errors does a sanity check on the globalThis.assert it finds to see if it seems to be the unstructured assert it assumes.

However, attempts to fix #5672 immediately starting running into problems like https://github.com/Agoric/agoric-sdk/actions/runs/9068381384/job/24918442236?pr=8332 with the tell-tale error message

Cannot initialize @endo/errors, missing globalThis.assert methods error, note, details, Fail, quote, makeAssert

These missing methods are exactly the method @endo/errors expects on the unstructured assert is uses and absent on the structured assert it exports. The cause is that the modules gathering endowments for a new Compartment were using a top level assert variable to populate the assert property of the endowments, that then became the globalThis.assert in the spawned Compartment. When these modules were changed to obtain their top-level assert binding by importing it from the @endo/errors module, they changed the assert they were endowing to the wrong assert.

@erights erights added the bug Something isn't working label Jun 15, 2024
@erights erights self-assigned this Jun 15, 2024
erights added a commit to endojs/endo that referenced this issue Jun 17, 2024
Closes: #XXXX
Refs: Agoric/agoric-sdk#9515
Agoric/agoric-sdk#9514

## Description

Addresses Agoric/agoric-sdk#9515 as applied to
endo, by doing for endo what
Agoric/agoric-sdk#9514 does for agoric-sdk

To address Agoric/agoric-sdk#5672 for endo ,
we should change all applicable uses of `assert` to obtain `assert` by
importing it from the `@endo/errors` package. However, attempts to do so
ran into the symptoms reported at
Agoric/agoric-sdk#9515 for the reasons
reported there -- accidentally using the imported `assert` as the
endowment for the global `assert` of new Compartments.

This PR prepares the ground for these fixes to
Agoric/agoric-sdk#5672 for endo by
unambiguously endowing with the original unstructured
`globalThis.assert`, which will remain the correct endowment even when
other uses of `assert` have migrated to the one imported from
`@endo/errors`. By itself, this PR, preceding those fixes to
Agoric/agoric-sdk#5672 for endo , is not
actually fixing a bug in the sense of a behavioral change. Reviewers,
let me know if you think this PR should be labeled a "refactor" because
of this.


### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

anyone gathering endowments for a new compartment should be aware of
this issue, and be sure to use `globalThis.assert` as the `assert`
endowment. Fortunately, this will only be of concern to advanced
developers.

### Testing Considerations

Since this PR doesn't actually cause any behavioral change, it cannot be
tested in place. Rather, its test will be whether #2324 still passes CI
once rebased on this PR.

***Update***: #2324 is now passing CI well enough to consider this PR
(#2323) to be adequately tested.


### Compatibility Considerations

The point. This PR itself only prepares the ground for the equivalent of
Agoric/agoric-sdk#9513 for endo. By itself it
has no other effect, and so no other compat issues.

### Upgrade Considerations

none
erights added a commit to endojs/endo that referenced this issue Jun 17, 2024
Staged on #2323 

Closes: #XXXX
Refs: Agoric/agoric-sdk#5672
Agoric/agoric-sdk#9513

## Description

Does for endo what Agoric/agoric-sdk#9513 does
for agoric-sdk --- importing `assert` from @endo/errors --- when it can
do so without causing dependency cycles. For the remaining packages that
would cause dependency cycles, just move them towards more modern assert
conventions while still using the unstructured global `assert`

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

We should document `assert` only as imported from @endo/errors, since
our users will not generally contribute code within endo's internal
dependency cycles.

### Testing Considerations

The CI run on this PR also serves to test #2323, since that one, by
itself, does not cause a behavioral change. It's purpose is to enable
this PR not to hit the problem described at
Agoric/agoric-sdk#9515

### Compatibility Considerations

none
### Upgrade Considerations

none
@mergify mergify bot closed this as completed in #9514 Jun 17, 2024
@mergify mergify bot closed this as completed in f908f89 Jun 17, 2024
mhofman pushed a commit that referenced this issue Jun 20, 2024
closes: #9515 
refs: #5672 #8332 #9513  endojs/endo#2323

## Description

To address #5672 , we should change all uses of `assert` to obtain `assert` by importing it from the `@endo/errors` package. However, attempts to do so (#8332 #9513) ran into the symptoms reported at #9515 for the reasons reported there -- accidentally using the imported `assert` as the endowment for the global `assert` of new Compartments.

This PR prepares the ground for these fixes to #5672 by unambiguously endowing with the original unstructured `globalThis.assert`, which will remain the correct endowment even when other uses of `assert` have migrated to the one imported from `@endo/errors`. By itself, this PR, preceding those fixes to #5672 , is not actually fixing a bug in the sense of a behavioral change. Reviewers, let me know if you think this PR should be labeled a "refactor" because of this.

### Security Considerations
none
### Scaling Considerations
none
### Documentation Considerations
anyone gathering endowments for a new compartment should be aware of this issue, and be sure to use `globalThis.assert` as the `assert` endowment. Fortunately, this will only be of concern to advanced developers.
### Testing Considerations
Since this PR doesn't actually cause any behavioral change, it cannot be tested in place. Rather, its test will be whether #9513 still passes CI once rebased on this PR.

***Update***: #9513 is now passing CI well enough to consider this PR (#9514) to be adequately tested.

### Upgrade Considerations
This PR by itself does not have any dependence on @endo/errors existing, and so should be compatible even when linked against fairly ancient versions of endo.
mhofman pushed a commit that referenced this issue Jun 22, 2024
closes: #9515 
refs: #5672 #8332 #9513  endojs/endo#2323

## Description

To address #5672 , we should change all uses of `assert` to obtain `assert` by importing it from the `@endo/errors` package. However, attempts to do so (#8332 #9513) ran into the symptoms reported at #9515 for the reasons reported there -- accidentally using the imported `assert` as the endowment for the global `assert` of new Compartments.

This PR prepares the ground for these fixes to #5672 by unambiguously endowing with the original unstructured `globalThis.assert`, which will remain the correct endowment even when other uses of `assert` have migrated to the one imported from `@endo/errors`. By itself, this PR, preceding those fixes to #5672 , is not actually fixing a bug in the sense of a behavioral change. Reviewers, let me know if you think this PR should be labeled a "refactor" because of this.

### Security Considerations
none
### Scaling Considerations
none
### Documentation Considerations
anyone gathering endowments for a new compartment should be aware of this issue, and be sure to use `globalThis.assert` as the `assert` endowment. Fortunately, this will only be of concern to advanced developers.
### Testing Considerations
Since this PR doesn't actually cause any behavioral change, it cannot be tested in place. Rather, its test will be whether #9513 still passes CI once rebased on this PR.

***Update***: #9513 is now passing CI well enough to consider this PR (#9514) to be adequately tested.

### Upgrade Considerations
This PR by itself does not have any dependence on @endo/errors existing, and so should be compatible even when linked against fairly ancient versions of endo.
mergify bot pushed a commit that referenced this issue Jul 3, 2024
Staged on #9514

closes: #5672
refs: #8332 #9512 #9514

Some description text below copied from #8332

## Description

Use the new `@endo/errors` and retire `@agoric/assert`.

This deletes the `@agoric/assert` package but doesn't so far as to deprecate it in NPM.

Fixes #5672 -- migrating all applicable uses of `assert` to import it from @endo/errors and use that one. This is a fresh attempt to do over what #8332 and #9512 tried to do.

Note that this is staged on #9514 which addresses #9515 -- ensuring that the `assert` used in endowing a new compartment is not the `assert` imported from @endo/errors but is rather the original `globalThis.assert`. For the agoric-sdk repo, that should be the only needed use of `assert` that should not be changed to use the one imported from @endo/errors.

### Security Considerations

Relies more directly on Endo, instead of through the `assert` package.

### Scaling Considerations
none

### Documentation Considerations
we should generally document @endo/errors as the only source of `assert`. However, we still need to call out the special case of Compartment endowments covered by #9514 and #9515 

### Testing Considerations

If CI passes here, it not only tests the correctness of this PR, but also of #9514, since #9514 by itself does not cause any testable changes. It is only to enable the changes in this PR to work.

### Upgrade Considerations

TBD. `@endo/errors` may require a newer SES than the vats have in their global env (for the `assert` global).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant