feat(ses): Add assert.makeError and deprecate assert.error as an alias #2895
Conversation
gibson042
force-pushed
the
gibson-2025-07-cleanup-ses-assert
branch
from
96d30b1 to
63d982a
Compare
| * to prevent revealing secrets up the exceptional path. In the example | ||
| * above, the thrown error may reveal only that `sky.color` is a string, | ||
| * whereas the same diagnostic printed to the console reveals that the | ||
| * sky was green. This masking can be disabled for an individual substitution value | ||
| * using `quote`. |
There was a problem hiding this comment.
This concrete "green" example text deserves to go somewhere. Things moved around enough that perhaps I missed it, but AFAICT you seem to have dropped it.
(Extra credit if we can find a way to slip in an allusion to the Hume/Goldman "grue" and "bleen", but only if it does not detract from the explanation for normal users ;) .)
There was a problem hiding this comment.
It's now the actual === expected || Fail`${actual} should be ${q(expected)}`; example for quote, but if you prefer the "sky" example I could switch that over.
packages/ses/types.d.ts
Outdated
| payload: any, | ||
| spaces?: string | number, | ||
| ): /** The declassified and quoted payload */ StringablePayload; | ||
| quote(value: any, spaces?: string | number): Stringable; |
There was a problem hiding this comment.
Huh. Neither the old or new text say what the optional spaces parameter means, and I no longer remember. Would be nice to fix, but feel free not to.
There was a problem hiding this comment.
It’s the third arg to JSON.stringify and I forgot we threaded it. It’s not a great idea to thread it.
packages/errors/index.js
Outdated
| @@ -49,9 +53,10 @@ if (missing.length > 0) { | |||
| const { | |||
| bare, | |||
| details: redacted, | |||
There was a problem hiding this comment.
| details: redacted, | |
| details: X, |
There was a problem hiding this comment.
Or better yet, normalize assert.X as the replacement for a deprecated assert.details, like this PR does for assert.makeError vs assert.error.
Need not be in this PR but I'd at least like your opinion ;)
There was a problem hiding this comment.
I agree that the introduction of name redacted (and for that matter, likewise throwRedacted) is odd. This file exports both assert and properties thereof, the latter with both full names and "conventional abbreviations and aliases", and details is the only property that is exported but not with its literal name. I'm pushing a commit that adds it.
packages/errors/index.js
Outdated
| @@ -75,7 +81,7 @@ export { | |||
| assert, | |||
| // related utilities that aren't assertions | |||
| bareOrQuote as bare, | |||
| makeError, | |||
| bestMakeError as makeError, | |||
| note, | |||
| quote, | |||
| redacted, | |||
There was a problem hiding this comment.
| redacted, | |
| X as redacted, // and deprecate `redacted` if possible | |
| Fail as throwRedacted, // and deprecate `throwRedacted` if possible | |
| Fail, | |
| ... |
| * @typedef {object} HiddenDetails | ||
| * | ||
| * Captures the arguments passed to the `details` template string tag. | ||
| * @typedef {{ template: TemplateStringsArray | string[], args: any[] }} DetailsParts |
There was a problem hiding this comment.
I'm ok with the rename if the doc-comment here mentions that a DetailParts may hold redacted secrets, and so must not leak. I originally named it Hidden* to suggest that such care should be taken.
| @@ -196,7 +198,11 @@ freeze(unredactedDetails); | |||
| export { unredactedDetails }; | |||
There was a problem hiding this comment.
Surprised to see that I exported unredactedDetails. Does not seem to be imported anywhere. If we cannot yet kill the (likely unused) export, could we at least deprecate it?
In any case, feel free not to address this quirk in this PR.
|
(I think the two CI errors were flakes, so restarted them. We'll see.) |
packages/ses/types.d.ts
Outdated
| payload: any, | ||
| spaces?: string | number, | ||
| ): /** The declassified and quoted payload */ StringablePayload; | ||
| quote(value: any, spaces?: string | number): Stringable; |
There was a problem hiding this comment.
It’s the third arg to JSON.stringify and I forgot we threaded it. It’s not a great idea to thread it.
…tion for `assert`
...as the culmination of general cleanup.
Description
assertan arrow function, resolving a TODOassert.makeErrorand deprecateassert.erroras an alias@endo/errorsexport ofassert.detailswith its own nameSecurity Considerations
None known.
Scaling Considerations
n/a
Documentation Considerations
If accepted, agoric-sdk use of
assert.errorshould be replaced withassert.makeError.Testing Considerations
Existing coverage is sufficient.
Compatibility Considerations
assert.erroris deprecated, but retained as an alias ofassert.makeErrorto support existing code.Upgrade Considerations
NEWS.md was updated to document the change.