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

refactor: use @endo/errors #9513

Merged
merged 31 commits into from
Jul 3, 2024
Merged

refactor: use @endo/errors #9513

merged 31 commits into from
Jul 3, 2024

Conversation

erights
Copy link
Member

@erights erights commented Jun 15, 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).

@erights erights changed the title Markm 5672 endo errors 2 refactor: use endo/errors Jun 15, 2024
Copy link

cloudflare-workers-and-pages bot commented Jun 15, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2a07b36
Status: ✅  Deploy successful!
Preview URL: https://f32623ba.agoric-sdk.pages.dev
Branch Preview URL: https://markm-5672-endo-errors-2.agoric-sdk.pages.dev

View logs

}
if (VERBOSE) {
console.log('REJECTED at top of event loop', reason);
}
};

const wrapFunction =
(func, sendingError, X) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

Why were we passing X around rather than using a module-global one?

@erights erights changed the title refactor: use endo/errors refactor: use @endo/errors Jun 15, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-5672-endo-errors-2 branch 2 times, most recently from d74c5bf to a0ab697 Compare June 15, 2024 04:50
@erights erights mentioned this pull request Jun 15, 2024
@erights erights changed the base branch from master to markm-fix-endowments June 16, 2024 19:48
@erights erights marked this pull request as ready for review June 16, 2024 20:41
@erights erights requested review from turadg and kriskowal June 16, 2024 20:41
@erights erights self-assigned this Jun 16, 2024
erights added a commit that referenced this pull request Jun 16, 2024
erights added a commit to endojs/endo that referenced this pull request 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 pull request 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
Base automatically changed from markm-fix-endowments to master June 17, 2024 22:22
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.
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

This is in my review queue. It has many conflicts with master so I'm requesting a rebase before reviewing.

@erights
Copy link
Member Author

erights commented Jul 1, 2024

This is in my review queue. It has many conflicts with master so I'm requesting a rebase before reviewing.

Done. PTAL

@erights erights requested a review from turadg July 1, 2024 17:53
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.

With a couple nits. Comments marked “Aside” do not need to be addressed in this change. I hope to never review this again :-)

packages/ERTP/src/mathHelpers/natMathHelpers.js Outdated Show resolved Hide resolved
packages/zoe/src/contractFacet/offerHandlerStorage.js Outdated Show resolved Hide resolved
packages/swingset-runner/src/chain.js Outdated Show resolved Hide resolved
packages/pegasus/src/once-promise-kit.js Outdated Show resolved Hide resolved
packages/internal/src/index.js Show resolved Hide resolved
@mhofman
Copy link
Member

mhofman commented Jul 3, 2024

TBD. @endo/errors may require a newer SES than the vats have in their global env (for the assert global).

The SES version in new or upgraded vats after upgrade-16 is now in sync with master, and will be forward as we release from a master branch.

Since #9514 was included in upgrade-16, and that the ZCF bundle is updated by that release, I believe it is safe for this to land because any new or upgraded Zoe contract will find the correct assert in its compartment's global.

@erights erights added the automerge:squash Automatically squash merge label Jul 3, 2024
@mergify mergify bot merged commit 940d3f0 into master Jul 3, 2024
79 checks passed
@mergify mergify bot deleted the markm-5672-endo-errors-2 branch July 3, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge technical-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

migrate to @endo/errors package
4 participants