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(ses,pass-style,marshal): permit Promise.any, AggregateError, cause #2042

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Feb 8, 2024

closes: #550
refs: #550 (comment) https://github.com/cosmology-tech/cosmos-kit/blob/b95e2f92585fd12d7532e846b98cd3b757c1acb8/packages/core/src/utils/endpoint.ts#L28 Agoric/agoric-sdk#7937

Description

We had previously omitted AggregateError from permits.js, effectively not permitting it as part of SES, because of concern with it not following the NativeError patterns we assume for the other errors. On further examination just now, the only non-uniformity I see are the addition of the own property errors. As an own property, it is irrelevant to the job of permits.js.

Likewise we have postponed dealing with Error cause. But it is an own property as well, and so besides the point of permits.js.

Having omitted AggregateError, we also declined to permit Promise.any since it returns an AggregateError instance. Thus, this PR simultaneously permits Promise.any.

  • Still need to adapt the ses console
  • Still need to adapt pass-style
  • Still need to adapt marshal
  • Still need to test

I am pleasantly surprised that it does not look like I need to adapt the taming of the Error constructor itself to add a cause argument to the replacement safe constructors. The replacement constructors already take and pass through ...rest arguments. But

  • Still need to test cause argument for the faux Error constructors, as well as a representative set of the permitted error constructors.

Security Considerations

Unlikely. But anything that permits more of the engine's surface area does in theory increase vulnerability.

Scaling Considerations

None

Documentation Considerations

If we currently document this weird exception to normal JS expectations, we can delete that explanation.

Otherwise, no docs needed since this PR just removes a surprising difference from normal JS developer expectations.

Testing Considerations

Currently, to test the handling of *Error subclasses of Error, we just test with a representative subclass such as UriError. This PR duplicates some of these tests for AggregateError.

Compatibility Considerations

We need to examine what happens if a ses that permits AggregateError talks to a ses that does not. This is mostly a marshal issue.

Some agoric-sdk compat concerns noted at #550 (comment)

The previous release of Endo seems to have some dependence on Node 12 which did not have AggregateError. This PR accommodates such early platforms reluctantly, so as not to break tests on those early platforms.

Upgrade Considerations

Nothing Breaking.

  • 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 Feb 8, 2024
@erights
Copy link
Contributor Author

erights commented Feb 8, 2024

@kriskowal , I'll need your help with the failure at https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042
that I do not know how to reproduce locally. (When I try naively, it locally succeeds.)

Run cd packages/ses && yarn test:platform-compatibility
  cd packages/ses && yarn test:platform-compatibility
  shell: /usr/bin/bash -e {0}
yarn run v1.[2](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:2)2.19
$ node test/package/test.cjs
/home/runner/work/endo/endo/packages/ses/dist/ses.cjs:2[3](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:3)73
  AggregateError];
  ^

ReferenceError: AggregateError is not defined
    at /home/runner/work/endo/endo/packages/ses/dist/ses.cjs:2373:3
    at Array.functors.imports (/home/runner/work/endo/endo/packages/ses/dist/ses.cjs:3721:3)
    at /home/runner/work/endo/endo/packages/ses/dist/ses.cjs:1109[4](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:5):1[5](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:6)
    at Object.<anonymous> (/home/runner/work/endo/endo/packages/ses/dist/ses.cjs:11722:3)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:8[6](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:7)3:32)
    at Function.Module._load (internal/modules/cjs/loader.js:[7](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:8)0[8](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:9):14)
    at Module.require (internal/modules/cjs/loader.js:887:1[9](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:10))
    at require (internal/modules/cjs/helpers.js:74:[18](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:19))
error Command failed with exit code 1.

@kriskowal
Copy link
Member

@kriskowal , I'll need your help with the failure at https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042 that I do not know how to reproduce locally. (When I try naively, it locally succeeds.)

Run cd packages/ses && yarn test:platform-compatibility
  cd packages/ses && yarn test:platform-compatibility
  shell: /usr/bin/bash -e {0}
yarn run v1.[2](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:2)2.19
$ node test/package/test.cjs
/home/runner/work/endo/endo/packages/ses/dist/ses.cjs:2[3](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:3)73
  AggregateError];
  ^

ReferenceError: AggregateError is not defined
    at /home/runner/work/endo/endo/packages/ses/dist/ses.cjs:2373:3
    at Array.functors.imports (/home/runner/work/endo/endo/packages/ses/dist/ses.cjs:3721:3)
    at /home/runner/work/endo/endo/packages/ses/dist/ses.cjs:1109[4](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:5):1[5](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:6)
    at Object.<anonymous> (/home/runner/work/endo/endo/packages/ses/dist/ses.cjs:11722:3)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:8[6](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:7)3:32)
    at Function.Module._load (internal/modules/cjs/loader.js:[7](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:8)0[8](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:9):14)
    at Module.require (internal/modules/cjs/loader.js:887:1[9](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:10))
    at require (internal/modules/cjs/helpers.js:74:[18](https://github.com/endojs/endo/actions/runs/7836517151/job/21384340084?pr=2042#step:11:19))
error Command failed with exit code 1.

I tracked this down. The CI job walks back to Node.js 12, before the introduction of AggregateError. I think we can delete the platform-compatibility CI job.

@erights erights changed the title feat(ses): permit Promise.any, AggregateError feat(ses,pass-style,marshal): permit Promise.any, AggregateError Feb 8, 2024
@erights erights changed the title feat(ses,pass-style,marshal): permit Promise.any, AggregateError feat(ses,pass-style,marshal): permit Promise.any, AggregateError, cause Feb 8, 2024
@erights
Copy link
Contributor Author

erights commented Feb 8, 2024

I think we can delete the platform-compatibility CI job.

Can I leave this to you, and proceed assuming it?

@erights
Copy link
Contributor Author

erights commented Feb 8, 2024

I think we can delete the platform-compatibility CI job.

Can I leave this to you, and proceed assuming it?

@kriskowal did. Done.

@erights erights force-pushed the markm-550-any-w-aggregate-error branch 8 times, most recently from b9802aa to 0c98c96 Compare February 10, 2024 23:29
@erights
Copy link
Contributor Author

erights commented Feb 11, 2024

Several places we use an ErrorConstructor generically, assuming its signature is uniform across all error constructors.

  • This uniform signature had not included cause. Now it should.
  • AggregateError may be passed as such an error constructor. But when such a generic error constructor parameter is used to make an error constructor, we need to adjust to the different parameters of the AggregateError constructor.

@erights erights force-pushed the markm-550-any-w-aggregate-error branch 8 times, most recently from ca5b2a4 to 2987499 Compare February 11, 2024 07:04
@erights erights force-pushed the markm-550-any-w-aggregate-error branch from 2987499 to f508532 Compare February 11, 2024 18:53
@erights erights changed the base branch from master to markm-tolerate-extra-error-props February 11, 2024 18:53
@erights erights force-pushed the markm-550-any-w-aggregate-error branch 2 times, most recently from bcb917d to 3d9354d Compare February 21, 2024 14:27
@erights
Copy link
Contributor Author

erights commented Feb 21, 2024

Seeing #2093 , I assume we'll do a new endo release soon, and then try again to do an agoric-sdk sync based on that. Given that, I'd really like to get this approved, merged, and into that upcoming endo release if possible.

From the review comments above, the outstanding issue seems to be explicit testing under XS. Reviewers, although I agree we need this in general, here's why I think we no longer need this specifically for this case:

#endo-branch: markm-550-any-w-aggregate-error

seems to indicate that ses at least initializes successfully in XS and does not cause any breakage in existing agoric-sdk CI tests. OTOH, the failure of the previous endo release despite #endo-branch testing of agoric-sdk indicates that #endo-branch is doing less compat testing than we thought. I don't know where the line is. Advise or clarification appreciated.

  • With the changes mentioned at feat(ses,pass-style,marshal): permit Promise.any, AggregateError, cause #2042 (comment) above, endo CI establishes that this PR now adapts to platforms such as Node 12 that lack AggregateError or were prior to introduction of AggregateError into the language standard. Presumably, this dependence on version of the language was the main reason to be more concerned about cross-platform testing for this PR than for other PRs.

Further, even after we have test262-like tests running under XS, this still doesn't give us adequate tooling to run this PR's tests under XS, since this PR's tests are written in ava. Do we have plans to be able to run our ava tests under XS? I am not aware of any. (Having raised this, I think we should. But not immediately. And I'm not sure how hard it would actually be.)

So, reviewers, what do you think of reviewing this PR for possible merging now, before we have the tooling to explicitly run the tests under XS.

@kriskowal
Copy link
Member

Seeing #2093 , I assume we'll do a new endo release soon, and then try again to do an agoric-sdk sync based on that. Given that, I'd really like to get this approved, merged, and into that upcoming endo release if possible.

From the review comments above, the outstanding issue seems to be explicit testing under XS. Reviewers, although I agree we need this in general, here's why I think we no longer need this specifically for this case:

#endo-branch: markm-550-any-w-aggregate-error

seems to indicate that ses at least initializes successfully in XS and does not cause any breakage in existing agoric-sdk CI tests. OTOH, the failure of the previous endo release despite #endo-branch testing of agoric-sdk indicates that #endo-branch is doing less compat testing than we thought. I don't know where the line is. Advise or clarification appreciated.

  • With the changes mentioned at feat(ses,pass-style,marshal): permit Promise.any, AggregateError, cause #2042 (comment) above, endo CI establishes that this PR now adapts to platforms such as Node 12 that lack AggregateError or were prior to introduction of AggregateError into the language standard. Presumably, this dependence on version of the language was the main reason to be more concerned about cross-platform testing for this PR than for other PRs.

Further, even after we have test262-like tests running under XS, this still doesn't give us adequate tooling to run this PR's tests under XS, since this PR's tests are written in ava. Do we have plans to be able to run our ava tests under XS? I am not aware of any. (Having raised this, I think we should. But not immediately. And I'm not sure how hard it would actually be.)

So, reviewers, what do you think of reviewing this PR for possible merging now, before we have the tooling to explicitly run the tests under XS.

We now have a test262 runner that spans XS on master but it is not integrated into CI. #1847

We do have plans in motion to build SES 262 tests. I do not have a clear idea of how to build 262 tests that target modules. It probably would involve bundling.

@dckc
Copy link
Contributor

dckc commented Feb 21, 2024

reviewers, what do you think of reviewing this PR for possible merging now, before we have the tooling to explicitly run the tests under XS.

I don't object. never did. But nor do I feel confident in approving. I hope another reviewer can do that.

@erights erights force-pushed the markm-550-any-w-aggregate-error branch from 3d9354d to 947710e Compare February 21, 2024 17:22
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.

I’m confident enough that there are no backward-compatibility surprises to merge and find out in Agoric SDK sync CI. @gibson042’s blessing on the Smallcaps change would be great since that’s the one area where my confidence is poorly substantiated.

A thought experiment I couldn’t complete on my own:

  • What is the current behavior for marshaling AggregateError or Error cause? Were the properties stripped or the message rejected?
  • If existing programs could send AggregateError, can existing programs receive an AggregateError and handle the appearance of new properties?
  • If existing programs could not send AggregateError, I see no compatibility concerns.

packages/common/NEWS.md Outdated Show resolved Hide resolved
packages/common/NEWS.md Outdated Show resolved Hide resolved
packages/errors/NEWS.md Outdated Show resolved Hide resolved
packages/marshal/NEWS.md Outdated Show resolved Hide resolved
packages/marshal/test/test-marshal-capdata.js Outdated Show resolved Hide resolved
packages/pass-style/NEWS.md Outdated Show resolved Hide resolved
packages/pass-style/src/passStyleOf.js Outdated Show resolved Hide resolved
packages/ses/NEWS.md Outdated Show resolved Hide resolved
@erights
Copy link
Contributor Author

erights commented Feb 21, 2024

I’m confident enough that there are no backward-compatibility surprises to merge and find out in Agoric SDK sync CI. @gibson042’s blessing on the Smallcaps change would be great since that’s the one area where my confidence is poorly substantiated.

The additional test cases in test-marshal-capdata.js and test-marshal-smallcaps.js are exactly parallel.

A thought experiment I couldn’t complete on my own:

  • What is the current behavior for marshaling AggregateError or Error cause? Were the properties stripped or the message rejected?

Prior to this PR, as AggregateError is passed as an Error. error.cause and error.errors as well as any additional properties are not sent.

With this PR, AggregateError is passed as AggregateError. However, it is still the case that error.cause and error.errors as well as any additional properties are not sent, since we cannot yet assume that all counter-parties have been upgraded to at least #2052 (already in the previous endo release) that enables them to tolerate receiving extra properties.

  • If existing programs could send AggregateError, can existing programs receive an AggregateError and handle the appearance of new properties?
  • If existing programs could not send AggregateError, I see no compatibility concerns.

Prior to this PR, a program attempting to send an AggregateError would actually send an Error. So I think this falls into your "I see no compatibility concerns" case.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few test comments, but I agree with @erights's assessment.

Comment on lines +157 to +160
if (typeof AggregateError === 'undefined') {
t.pass('skip test on platforms prior to AggregateError');
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems too disruptive; we should only skip the narrow subset of test logic that won't work as expected.

packages/marshal/test/test-marshal-capdata.js Outdated Show resolved Hide resolved
passStyleOfRecur,
check = undefined,
) => {
const reject = !!check && (details => check(false, details));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2097 for a more concise pattern.

@kriskowal kriskowal force-pushed the markm-550-any-w-aggregate-error branch 3 times, most recently from ca51656 to 45fafbc Compare February 22, 2024 01:06
@kriskowal kriskowal force-pushed the markm-550-any-w-aggregate-error branch from 45fafbc to 5762dd4 Compare February 22, 2024 01:09
@kriskowal kriskowal merged commit 18fb8c0 into master Feb 22, 2024
14 checks passed
@kriskowal kriskowal deleted the markm-550-any-w-aggregate-error branch February 22, 2024 01:17
mergify bot added a commit to Agoric/agoric-sdk that referenced this pull request Apr 22, 2024
#9275)

closes: #XXXX
refs: endojs/endo#2042

## Description

Starting at endojs/endo#2042 endo fully supports
the real `AggregateError` constructor, so there is no longer any need
for the `makeAggregateError` workaround. The @agoric/internal packages
*should* not be imported by anything outside the agoric-sdk repo, so
technically I could remove it in this same PR. As a conservative
don't-break-things, I still export it as deprecated.

Even after upgrading to an endo supporting `AggregateError`, the old
`makeAggregateError` was emulating it using a direct instance of
`Error`, so this is in theory not a pure refactor. But I would be
flabbergasted if any code came to depend on this not being a genuine
`AggregateError`, in which case this change is a pure refactor.

Reviewers, please let me know if you'd prefer that I delete
`makeAggregateError`, as I'd be happy to.

### Security Considerations
none

### Scaling Considerations

none
### Documentation Considerations

none
### Testing Considerations

none
### Upgrade Considerations

none

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mergify bot added a commit to Agoric/agoric-sdk that referenced this pull request Apr 30, 2024
closes: #XXXX
refs: endojs/endo#2096
endojs/endo#2042

## Description

Deleting @agoric/base-zone's implementation of `isPassable` completes
the migration of `isPassable` from @agoric/base-zone to @endo/pass-style
explained in endojs/endo#2096 and started in
endojs/endo#2042

The remaining issue explained in
endojs/endo#2096 , changing how `isPassable`
is implemented, remains to be done in @endo/pass-style. But this need no
longer concern us here since that difference will now be encapsulated
from us.

### Security Considerations
None
### Scaling Considerations

We know that `passStyleOf` remains a performance hotspot that needs
attention. This PR does not affect that at all. But I'll note that the
remaining suggested change from
endojs/endo#2096 --- to implement `isPassable`
and `passStyleOf` in terms of a more expressive internal function
parameterized by a checker --- might make this performance issue worse.
Just something to keep in mind as we tune `passStyleOf`. Attn @gibson042

### Documentation Considerations

none
### Testing Considerations

none
### Upgrade Considerations

As of this PR, @agoric/base-zone also no longer exports `isPassable`,
potentially breaking importers outside agoric-sdk until they are
modified to import it from @endo/pass-style as well. This PR does take
care of all such import sites within agoric-sdk.

If this turns out to be a problem in practice, this PR could be changed
to have @agoric/base-zone reexport the `isPassable` it imports from
@endo/pass-style, but deprecate that reexport, leaving it to future work
to change those old import sites outside agoric-sdk.

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@mhofman
Copy link
Contributor

mhofman commented Jun 12, 2024

Talking about XS testing, we definitely should, and in particular we should test under the version of XS used by agoric-sdk, which currently does not support AggregateError

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.

Add AggregateError and Promise.any to whitelist
7 participants