feat(marshal): tolerate extra error props

[erights]

closes: #XXXX
refs: #2042 #550

Description

To enable future versions of marshal to send extra error properties, like cause and error, marshal must tolerate receiving them.

Sets the stage for #2024

Security Considerations

by tolerating the reception of extra error props, we no longer inhibit senders from sending them, including misspelled or otherwise mistaken ones. This weaken the degree to which unmarshaling validates. This is the necessary price of accommodating expected future growth.

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

tests included also serve as a baseline for #2042, which needs to change them in order to pass again. This provides a record of the difference between this PR and #2042.

Compatibility Considerations

The whole point, is to accommodate an expected future in which some extra properties are sent, such as cause and errors.

Upgrade Considerations

This change, to tolerate receiving extra properties, needs to roll out everywhere, before we can roll out a future change sending extra properties such as cause or errors. (#2042, though it is a future change that builds on this one, is not that future change because it still does not send any extra properties.)

This PR is not breaking. Nor is it combined with #2042. The future change where extra error properties are sent will not be a breaking change compared to code incorporating this PR (or this PR + #2042). However, that future change will be breaking compared to code prior to this PR.

No news update needed.

• Includes *BREAKING*: in the commit message with migration instructions for any breaking change.
• Updates NEWS.md for user-facing changes.

[erights]

@gibson042 , as far as I could tell, I did not need to make a corresponding change to encodePassable.js. Is this true, or did I miss something? What about tests of encodePassable?

[erights]

@kriskowal , although #2042 is also ready for review, I do not expect to get it into the next endo release. It is also large, and therefore has more design issues we may need to argue through.

But because of the stage setting nature of this PR and well as its minimal effect, I would very much like to get this PR into the next endo release if possible.

[dckc]

Upgrade Considerations

None

I wonder if it's more subtle than that. This one (call it n) is not a breaking change, and a future change (n+m) to take advantage of this won't look like a breaking change compared to this one, but n+m will be a breaking change w.r.t. n-1.

So this looks like one of those changes that has to roll out ~everywhere before anything can take advantage of it.

[erights]

Upgrade Considerations

None

I wonder if it's more subtle than that. This one (call it n) is not a breaking change, and a future change (n+m) to take advantage of this won't look like a breaking change compared to this one, but n+m will be a breaking change w.r.t. n-1.

So this looks like one of those changes that has to roll out ~everywhere before anything can take advantage of it.

Ok, I updated the PR "Upgrade Considerations" section to explain this. But it still doesn't seem to be that this change should be considered breaking or NEWSworthy. Reviewers, please let me know if you disagree, and what you suggest that I do. Thanks.

[dckc]

this looks plausible to me, but the history goes back to #550 , so I'm inclined to defer to someone with more context. LMK if they're not available in a useful timeframe.

[kriskowal]

This is a relaxation and a new feature. I do think a note in NEWS about the new capability to transmit errors with properties is worth mentioning and will be useful for future investigations.

[kriskowal]

So this looks like one of those changes that has to roll out ~everywhere before anything can take advantage of it.

This seems accurate to me.

In isolation, this allows a node to receive messages that no node can yet produce. When they can produce such messages, a new node will be able to send messages that will be rejected by an old node. There is some hazard that new programs will pass tests locally and discover they fail in integration with an old node. When we broach that, we need a migration plan.

[erights]

This is a relaxation and a new feature. I do think a note in NEWS about the new capability to transmit errors with properties is worth mentioning and will be useful for future investigations.

Done

[gibson042]

@gibson042 , as far as I could tell, I did not need to make a corresponding change to encodePassable.js. Is this true, or did I miss something? What about tests of encodePassable?

Confirmed; passable decoders already support decoding errors into whatever comes from a caller-provided decodeError (and don't even assert that the results are errors, although they probably should) and encoders require only that the result has the right prefix.

But a dedicated test in test-encodePassable.js might make sense.

[gibson042]

Thank you for this commentary.