-
Notifications
You must be signed in to change notification settings - Fork 75
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
Conversation
640d1c0
to
cf992eb
Compare
d15eb16
to
c4db275
Compare
fbbfc93
to
7de25c7
Compare
7de25c7
to
973c120
Compare
973c120
to
64fb457
Compare
This small PR is R4R |
64fb457
to
68a6bc2
Compare
68a6bc2
to
fccab59
Compare
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 |
There was a problem hiding this 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.
fccab59
to
2ce3733
Compare
|
sync test in CI at Agoric/agoric-sdk#9201 . We'll see what happens. |
Agoric/agoric-sdk#9201 with the |
closes: #2173
refs: #XXXX
Description
As of the last endo release, @endo/passStyle exports two broken tester functions:
isPassableError
andassertPassableError
, neither of which correctly do what their names promise. In particular, they will judge some non-frozen errors to be passable even thoughpassStyleOf(e)
will correctly throw,isPassable(e)
will correctly sayfalse
, andmatches(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 fixestoPassable
.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 thanfeat(...):
. 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
andassertPassableError
. 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.[ ] UpdatesNEWS.md
for user-facing changes.