fix(pass-style): toPassableError fixed. (is/assert)PassableError removed.

erights commented 4 months ago

closes: #2173 refs: #XXXX

Description

As of the last endo release, @endo/passStyle exports two broken tester functions: isPassableError and assertPassableError, neither of which correctly do what their names promise. In particular, they will judge some non-frozen errors to be passable even though passStyleOf(e) will correctly throw, isPassable(e) will correctly say false, and matches(e, M.error()) will correctly throw. (matches throws if the specimen is not passable).

Their one use was by toPassableError which was therefore incorrect, which is how I noticed the problem. This PR fixes toPassable.

Fortunately, these two functions do not appear to be used anywhere else. They are also unneeded, as the other correct tests suffice. Therefore I would like to withdraw them rather than fix them, even though it would be trivial to fix them. This PR currently deletes them.

Reviewers, deleting released exports is technically a compat break, so I have currently marked marked this PR feat(...)!: rather than feat(...):. If it is ok with you, I would like to remove the !. Please let me know in a comment.

Security Considerations

exporting test functions that incorrectly judge non-frozen errors to be passable, and exporting a toPassableError that incorrectly would return such non-frozen errors, is a potential local security hazard, though there are no known vulnerabilities due to this hazard. I'll hazard a guess that it is unlikely there are currently any such undiscovered vulnerabilities. But as we make more use of mutually suspicious tenants coexisting in the same vat, if we don't fix this hazard, it may well lead to future vulnerabilities.

Scaling Considerations

none

Documentation Considerations

Current typedoc may have automatically generated docs for isPassableError and assertPassableError. But they will disappear on the first run of typedoc following this PR, so no problem.

Testing Considerations

Added a test that I checked does fail before this PR.

The bad news is that this bug was not caught before this PR by any tests. The bug was only caught by a surprise during new development.

Compatibility Considerations

See the top PR comment for the compat issues in withdrawing an export that was present in a previous release.

Reviewers, with your approval, I would like to delete the ! because this is only a compat issue in theory, but probably not in practice.

Upgrade Considerations

None.

Though a withdrawal is technically breaking, I don't think it warrants a BREAKING note nor a mention in NEWS.md. Reviewers, please let me know if you think otherwise.

~[ ] Includes *BREAKING*: in the commit message with migration instructions for any breaking change.~
~[ ] Updates NEWS.md for user-facing changes.~

erights commented 4 months ago

This small PR is R4R

kriskowal commented 3 months ago

I’m prepared to believe that this is non-breaking in practice. Given that known dependent packages are largely in Agoric SDK, I’d be satisfied by a green integration PR pinned to #endo-branch: master https://www.npmjs.com/package/@endo/pass-style?activeTab=dependents

erights commented 3 months ago

! removed. Will do the sync tests before merging. What would be involved in also doing the browser tests?

erights commented 3 months ago

sync test in CI at https://github.com/Agoric/agoric-sdk/pull/9201 . We'll see what happens.

erights commented 3 months ago

https://github.com/Agoric/agoric-sdk/pull/9201 with the force:integration label and #endo-branch: markm-make-error-freezes completed CI as all green. So I am about to merge

endojs / endo