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

feat(pass-style,exo): exo boundary only throws throwables #2266

Merged
merged 1 commit into from
May 6, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented May 5, 2024

closes: #XXXX
refs: #2223

Description

Modest step towards #2223 ,

  • bringing all the intended safety: the exo boundary only throws throwables, and that callWhen async exo methods only reject with throwable reasons.
  • but with less flexibility: the definition of passable errors and throwables is narrower than it need be.

Security Considerations

This is one of three necessary steps towards having the exo boundaries become the new intravat defensive security boundary, rather than treating every object boundary as a potential defensive security boundary. The rationale for this step is that the throw-control-path is too hard for reviewers to pay full attention to, so we wish to prevent caps from leaking over the exo boundary via the throw path. (E prevented caps from escaping via throws in general, but we're not in a position to be able to enforce that in general within Hardened JS.)

The other two steps are

  • Converting uses of Far for making far objects into making exos.
  • Addressing the leakage of promise settlements across exo boundaries due to the inability to synchronously enforce such a constraint on a promise passed through the boundary.

Scaling Considerations

Given that the exceptional pathways (throws and rejects) are only used for low frequency exceptional conditions, or at least exceptional conditions whose speed we don't care about, making this slow path a tiny bit slower should be inconsequential. Indeed, I expect the overall impact to be unmeasurable. (But we won't know until we measure!)

Documentation Considerations

This restriction on what can be throws across the exo boundary (or for exo async callWhen methods, what can be used as a rejection reason) needs to be documented. But it should not affect any normal code, and so documents an edge case.

Testing Considerations

tests included

Compatibility Considerations

If there are currently any throws or rejects that violate these constraints (likely) where other code depends on these violations (unlikely), then we have a compat problem.

Upgrade Considerations

None.

  • [ ] 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 May 5, 2024
@erights erights marked this pull request as ready for review May 5, 2024 05:28
@erights erights requested a review from kriskowal May 5, 2024 05:28
@erights
Copy link
Contributor Author

erights commented May 5, 2024

agoric-sdk compat testing underway at Agoric/agoric-sdk#9201 , with

pound-endo-branch: markm-to-throwable-2
and
force:integration label

@erights
Copy link
Contributor Author

erights commented May 5, 2024

@kriskowal I hopefully marked this as next-release because it would be really nice for that to happen, and I also hope this is an easy enough review to not impose much delay. But if not, feel free not to include this in the next release.

@erights
Copy link
Contributor Author

erights commented May 5, 2024

CI at Agoric/agoric-sdk#9201 failed due to the presence of
patches/@endo+exo+1.4.0.patch , which seems will need to disappear in the next sync anyway.
So I just added to it a removal of that patch file to Agoric/agoric-sdk#9201 and am trying again.

@erights
Copy link
Contributor Author

erights commented May 5, 2024

CI is still failing a lot, but none of the failures seem to have anything to do with this PR.

@erights
Copy link
Contributor Author

erights commented May 5, 2024

Restaged Agoric/agoric-sdk#9201 on Agoric/agoric-sdk#8774 and trying again. I see that Agoric/agoric-sdk#8774 is not green either. But if Agoric/agoric-sdk#9201 only has the same failures as Agoric/agoric-sdk#8774 , I'll count that as a success.

@erights erights requested a review from turadg May 5, 2024 06:04
@erights
Copy link
Contributor Author

erights commented May 5, 2024

Reviewers, because it is possible in theory that some old code was depending on an exo throwing something that would now be classified as a non-throwable, this PR is in theory a compat break. So once again, please let me know whether you think this PR should be marked as a feat(pass-style,exo)!:. IMO if Agoric/agoric-sdk#9201 comes out as clean as Agoric/agoric-sdk#8774 , I hope the answer will be no, we do not need the !.

Speaking of which, Agoric/agoric-sdk#9201 CI now has novel failures such as

Error: src/storage-test-utils.js(2,1): error TS9006: Declaration emit for this file requires using private name 'PASS_STYLE' from module '"/home/runner/work/agoric-sdk/agoric-sdk/node_modules/@endo/pass-style/src/types"'. An explicit type annotation may unblock declaration emit.

at https://github.com/Agoric/agoric-sdk/actions/runs/8956132010/job/24597592447?pr=9201#step:6:2640

These still seem unrelated to this PR, so I don't understand why they are different from Agoric/agoric-sdk#8774 . OTOH, it does seem to have something to do with the @endo/pass-style module, so perhaps it is related to this PR in a way I don't understand.

@erights erights force-pushed the markm-to-throwable-2 branch 3 times, most recently from 19b5817 to 8966b06 Compare May 5, 2024 06:45
@kriskowal
Copy link
Member

You are down to a couple ts-expect-errors that can be removed in integration. You can do me a favor by adding a commit to endo-integration-master on Agoric SDK so it gets picked up by CI and eventually by me for the sync. Agoric/agoric-sdk#9201 (comment)

@erights erights merged commit 2f0888e into master May 6, 2024
17 checks passed
@erights erights deleted the markm-to-throwable-2 branch May 6, 2024 21:33
@erights erights mentioned this pull request May 6, 2024
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.

2 participants