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

migrate to @endo/errors package #5672

Closed
dckc opened this issue Jun 26, 2022 · 12 comments · Fixed by #9513
Closed

migrate to @endo/errors package #5672

dckc opened this issue Jun 26, 2022 · 12 comments · Fixed by #9513
Assignees
Labels
devex developer experience hygiene Tidying up around the house technical-debt vaults_triage DO NOT USE

Comments

@dckc
Copy link
Member

dckc commented Jun 26, 2022

What is the Problem Being Solved?

assert is a global in SES. However, IDEs don't know about it or its members. To allow automatic imports we have @agoric/assert, but it's useful outside of Agoric.

Description of the Design

Move most of @agoric/assert to use @endo/errors. Move NonNullish checked cast to @agoric/internal.

Acceptance

No more use of global assert in agoric-sdk. This will support getting assertion functions without having set up a SES environment.

@dckc dckc added hygiene Tidying up around the house technical-debt labels Jun 26, 2022
@dckc dckc self-assigned this Jun 26, 2022
dckc added a commit that referenced this issue Jun 27, 2022
@dckc
Copy link
Member Author

dckc commented Jun 27, 2022

@erights the little justification I was able to find on this was not sufficient to convince @turadg : #5673 (review)

@dckc dckc changed the title deprecate @agoric/assert package deprecate @agoric/assert package? Jun 27, 2022
@dckc
Copy link
Member Author

dckc commented Jun 28, 2022

@turadg as we discussed, I added a note to our Coding Style:

Note: pending resolution of #5672, within agoric-sdk, let's use the SES global rather than importing from @agoric/assert.
That is: use const { details: X, quote: q} from assert;.

@Tartuffo Tartuffo unassigned dckc and erights Nov 4, 2022
@turadg
Copy link
Member

turadg commented Nov 10, 2022

@abebeos can you expand on what your interest is in whether @agoric/assert exists? It has no impact on SES or Endo.

@erights
Copy link
Member

erights commented Nov 11, 2022

Hi @abebeos , appreciated. This outstanding question is indeed technical debt we need to resolve. Currently we use both styles:

const { details: X, quote: q } = assert;

vs

import { details as X, quote as q } from '@agoric/assert';

Aside from history, I don't see any good reason for these to coexist. We should consistently use one or the other. In addition, the @agoric/assert, as its name implies, exists at the agoric-sdk level, not the endo level, making it impossible to adopt it for general use without deeper intervention.

@turadg , do your reasons for keeping @agoric/assert still apply to our system after all the work you've done repairing our types? Are these reasons strong enough that you think we should move it to @endo/assert and adopt it everywhere? If not, can we repair whatever the remaining reasons are for @agoric/assert and then deprecate it, for eventual removal?

FWIW, for new code, I always use the assert global rather than the @agoric/assert package and have never noticed any resulting pain.

@erights
Copy link
Member

erights commented Nov 11, 2022

Now that endojs/endo#1334 and endojs/endo#1350 have been merged, endo has fully moved from the old

assert(cond, X`...`);

style to the new

cond || Fail`...`;

style. @kriskowal has already started turning the endo release crank, after which Fail will be available for use in agoric-sdk as well. Once it is, we can simplify, repair, and complete #6482 , consistently using this style throughout agoric-sdk as well. If we revive and complete #5673 first, then #6482 can be simpler. Otherwise, we'd need #6482 to additionally export Fail from the @agoric/assert package in the meantime.

So resolving this issue now is timely. Thanks!

@erights
Copy link
Member

erights commented Nov 11, 2022

Hi @abebeos , the functionality you mention, "An assertion library that keeps sensitive data outside of the Error", is already functionality provided by the assert global provided by SES. See https://github.com/endojs/endo/blob/master/packages/ses/src/error/README.md

I don't think there's any reason to introduce a distinct assert-intra.

@turadg
Copy link
Member

turadg commented Nov 14, 2022

I'd like to request that we don't deprecate this package and instead explain why it exists.

May I ask why it exists?

For me, as a developer of agoric-sdk, it was valuable to be able to type as assertion export in my IDE and autocomplete to get the import. That's what I was referring to in the preceding sentence,

Having that export would let IDEs autocomplete when someone tries to use X as a function

Another advantage of keeping this package is it informs the developer of what to do when globalThis.assert isn't defined,

const globalAssert = globalThis.assert;
if (globalAssert === undefined) {
throw new Error(
`Cannot initialize @agoric/assert, missing globalThis.assert, import 'ses' before '@agoric/assert'`,
);
}

If we revive and complete #5673 first, then #6482 can be simpler.

What's simpler about,

const { details: X, Fail } = assert;

than

import { assert, details as X } from '@agoric/assert';

? They're both used in the PR. I do think it may be worth consolidating, though I would ask that we consolidate around the import. I'm not clear on the arguments why not to, within this repo.

Otherwise, we'd need #6482 to additionally export Fail from the @agoric/assert package in the meantime.

Yes, I'm happy to do that. #6566

@turadg
Copy link
Member

turadg commented Aug 4, 2023

@kriskowal has agreed to have an @endo/assert, as a ponyfill for the assert shim. I've renamed this issue to be about migrating to that.

The NonNullish checked cast isn't on the assert global so it won't go along. We'll move it to agoric/internal instead.

@turadg turadg changed the title deprecate @agoric/assert package? migrate to @endo/assert package Aug 4, 2023
@turadg turadg changed the title migrate to @endo/assert package migrate to @endo/errors package Sep 12, 2023
@turadg
Copy link
Member

turadg commented Feb 9, 2024

Reassigning to @erights who's working on it in #8781

@turadg turadg assigned erights and unassigned turadg Feb 9, 2024
@erights
Copy link
Member

erights commented Feb 12, 2024

At endojs/endo#2042 (comment) I suspect that the @endo/errors package may not be successfully exporting the types it should.

  • I need to figure this out before we switch agoric-sdk to using it.

@turadg
Copy link
Member

turadg commented Jun 12, 2024

Another problem with expecting the global assert is it's only available as a side-effect of Endo being inited. If you import something like @agoric/ertp, you can't use it until Endo is inited but you don't know that and nothing says so.

If @agoric/ertp used the exports from @endo/errors then there would be no reliance on side-effects and that package would encapsulate shimming as needed.

Shall we move all use of assert global to an import? If so shall it be part of this Issue or another?

@dckc dckc added the devex developer experience label Jun 12, 2024
@erights
Copy link
Member

erights commented Jun 12, 2024

Shall we move all use of assert global to an import?

Yes, for everything that can depend on @endo/errors without introducing a cyclic dependency. This includes all of the agoric-sdk repo and almost all of the endo repo.

If so shall it be part of this Issue or another?

This issue seems a like fine place.

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 bot pushed a commit that referenced this issue 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 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 mergify bot closed this as completed in #9513 Jul 3, 2024
@mergify mergify bot closed this as completed in 940d3f0 Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devex developer experience hygiene Tidying up around the house technical-debt vaults_triage DO NOT USE
Projects
None yet
5 participants