AggregateError is not supported on all engines #9504
Labels
bug
Something isn't working
Comments
mhofman
added a commit
that referenced
this issue
Jun 20, 2024
…9508) closes: #9504 ## Description While #9275 switched to a global `AggregrateError` constructor, it was unclear if that was safe because that global is not currently available in xsnap. Instead of reverting, this PR moves the "shared" internal helpers that used an `AggregateError` to the `node` folder, to make it clear they should only be used in that environment. All usages of these helpers were already in Node only. All other usages of `AggregateError` were audited to confirm they only happened on Node as well. ### Security Considerations None ### Scaling Considerations None ### Documentation Considerations None ### Testing Considerations We're unfortunately lacking coverage of some code under xsnap, Open to ideas on how to improve testing here It would also be nice somehow to make sure the `node` folders do no end up imported in bundled code. Maybe having a pseudo `import 'node:process'` could work? @kriskowal ### Upgrade Considerations None
mhofman
added a commit
that referenced
this issue
Jun 22, 2024
…9508) closes: #9504 ## Description While #9275 switched to a global `AggregrateError` constructor, it was unclear if that was safe because that global is not currently available in xsnap. Instead of reverting, this PR moves the "shared" internal helpers that used an `AggregateError` to the `node` folder, to make it clear they should only be used in that environment. All usages of these helpers were already in Node only. All other usages of `AggregateError` were audited to confirm they only happened on Node as well. ### Security Considerations None ### Scaling Considerations None ### Documentation Considerations None ### Testing Considerations We're unfortunately lacking coverage of some code under xsnap, Open to ideas on how to improve testing here It would also be nice somehow to make sure the `node` folders do no end up imported in bundled code. Maybe having a pseudo `import 'node:process'` could work? @kriskowal ### Upgrade Considerations None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
#9275 switched from using a custom
AggregateErrorapproach to only relying on a global one. However the version of XS we're using in agoric-sdk does not yet supportAggregateError.Steps to reproduce
None, this is only used in edge case error scenarios, and as such does not seem to be exercised. Comes from an audit of the code.
Expected behavior
Only using features available in all our supported engines
Platform environment
master
Description of the Design
Either revert #9275 or introduce a trusted shim in
xsnap-lockdownfor AggregateError.Security Considerations
If going the trusted shim route, we should not introduce a dependency on 3rd party npm packages as that'd require continuous audits on version changes, for which we don't have a good documented process. Vendor in instead.
Test Plan
Besides the edge case scenarios under which this would currently be exercised, it appears we don't run any non swingset tests under XS. So ever if we add a unit test to the
internalpackage, we have no way to run it under XS currently.The text was updated successfully, but these errors were encountered: