use @endo/errors #8332
Closed
use @endo/errors #8332
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
turadg
force-pushed
the
5672-endo-errors
branch
from
12f678e to
9be10ee
Compare
Deploying agoric-sdk with
|
| Latest commit: |
f01fd88
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://32faadcf.agoric-sdk.pages.dev |
| Branch Preview URL: | https://5672-endo-errors.agoric-sdk.pages.dev |
This was referenced Jun 14, 2024
Closed
mergify bot
pushed a commit
that referenced
this pull request
Jun 17, 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 pull request
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 pull request
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 pull request
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes: #5672
Description
Use the new
@endo/errorsand retire@agoric/assert.This deletes the
@agoric/assertpackage but doesn't so far as to deprecate it in NPM.Security Considerations
Relies more directly on Endo, instead of through the
assertpackage.Scaling Considerations
n/a, just refactoring
Documentation Considerations
Nothing externally facing
Testing Considerations
CI
Upgrade Considerations
TBD.
@endo/errorsmay require a newer SES than the vats have in their global env (for theassertglobal).