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

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

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Mar 18, 2024

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 erights self-assigned this Mar 18, 2024
@erights erights force-pushed the markm-make-error-freezes branch from 640d1c0 to cf992eb Compare March 18, 2024 20:14
@erights erights changed the title fix(ses,errors): makeError freezes returned error fix(ses,errors)!: toPassableError fixed. (is/assert)PassableError removed. Mar 18, 2024
@erights erights force-pushed the markm-make-error-freezes branch 3 times, most recently from d15eb16 to c4db275 Compare March 23, 2024 22:57
@erights erights changed the title fix(ses,errors)!: toPassableError fixed. (is/assert)PassableError removed. fix(pass-style)!: toPassableError fixed. (is/assert)PassableError removed. Mar 23, 2024
@erights erights force-pushed the markm-make-error-freezes branch 2 times, most recently from fbbfc93 to 7de25c7 Compare March 24, 2024 00:04
@erights erights force-pushed the markm-make-error-freezes branch from 7de25c7 to 973c120 Compare March 24, 2024 00:17
@erights erights requested review from gibson042 and kriskowal March 24, 2024 00:19
@erights erights force-pushed the markm-make-error-freezes branch from 973c120 to 64fb457 Compare March 24, 2024 00:26
@erights erights marked this pull request as ready for review March 24, 2024 00:33
@erights
Copy link
Contributor Author

erights commented Mar 24, 2024

This small PR is R4R

@erights erights force-pushed the markm-make-error-freezes branch from 64fb457 to 68a6bc2 Compare March 24, 2024 18:10
@erights erights force-pushed the markm-make-error-freezes branch from 68a6bc2 to fccab59 Compare April 5, 2024 00:54
@kriskowal
Copy link
Member

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

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.

Aside: It would be good if we could put together some Playwright tests (see endo/browser-tests) that exercise marshal, pass-style, & captp.

@erights erights force-pushed the markm-make-error-freezes branch from fccab59 to 2ce3733 Compare April 5, 2024 18:57
@erights erights changed the title fix(pass-style)!: toPassableError fixed. (is/assert)PassableError removed. fix(pass-style): toPassableError fixed. (is/assert)PassableError removed. Apr 5, 2024
@erights
Copy link
Contributor Author

erights commented Apr 5, 2024

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

@erights
Copy link
Contributor Author

erights commented Apr 5, 2024

sync test in CI at Agoric/agoric-sdk#9201 . We'll see what happens.

@erights
Copy link
Contributor Author

erights commented Apr 5, 2024

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

@erights erights merged commit 205e45f into master Apr 5, 2024
18 checks passed
@erights erights deleted the markm-make-error-freezes branch April 5, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isPassableError, assertPassableError, and toPassableError judge non-frozen errors to be passable.
2 participants