Rewrite error serialization by FrederikBolding · Pull Request #61 · MetaMask/rpc-errors

FrederikBolding · 2022-10-27T14:08:45Z

Rewrite error serialization to allow any error that conforms to the JsonRpcError JSON-compliant type.

If the error does not conform to this type, it will be wrapped in an Internal RPC error and the original error will be included as the data.cause. Any non JSON-compliant props of the error will be removed.

Fixes https://github.com/MetaMask/eth-rpc-errors/issues/51

kumavis · 2023-03-13T18:56:48Z

+ // If the error does not match the JsonRpcError type, use the fallback error, but try to include the original error as `cause`
+ const cause = serializeCause(error);
+ const fallbackWithCause = {
+ ...fallbackError,


drops non enumerable properties

What would you prefer then? Setting enumerable to true for fallbackError?

In my opinion, this wouldn't be an issue since we specify the fallback error and it includes the cause that may still have a stack.

Presumably what we'd want to do here is preserve the fallbackError as much as possible, mutating it only to add the cause property.
In this case the fallback error is already expected to be a "serializable object" though, right? So maybe we don't need to worry about non-enumerable properties. Though the JSDoc description should certainly clarify that expectation if that is the case.

Yeah, I would expect it to be serializable according to the type. I can add it to the doc string

socket-security · 2023-04-11T12:50:43Z

New dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

📊 Modified Dependency Overview:

⬆️ Updated Package	Version Diff	Added Capability Access	`+/-` Transitive Count	Publisher
@metamask/utils@5.0.0	3.6.0...5.0.0	network	`+47/-0`	metamaskbot
typescript@4.9.5	4.7.4...4.9.5	network, shell, environment	`+0/-0`	typescript-bot

Gudahtt

LGTM!

#158) This ensures that non-empty string `error.message` properties of serialized errors are preserved by default, even if the serialized error is not [a valid JSON-RPC error](https://www.jsonrpc.org/specification#error_object). This behavior can be overridden by setting `shouldPreserveMessage: false`. In #61, our error serialization logic was considerably improved. One of the behavioral changes made at the time was to always overwrite the `message` property with that of the fallback error (practically always the "internal JSON-RPC-error"), regardless of whether a non-empty string message was present on the original error object. We have yet to ship this everywhere in our stack, in part because such a change may be breaking for our consumers. By reverting to the old behavior for the `message` property only, we avoid these potential breakages and improve the accessibility of potentially useful information to consumers (i.e. directly in the error message as opposed to buried in `error.data.cause.message`).

FrederikBolding force-pushed the fb/fix-error-serialization branch 4 times, most recently from 4fac448 to d92428bCompare November 2, 2022 12:02

Mrtenz requested changes Nov 2, 2022
View reviewed changes

Comment threadsrc/utils.ts Outdated
Comment threadsrc/utils.ts Outdated
Comment threadsrc/utils.ts Outdated
Comment threadsrc/utils.ts Outdated

FrederikBolding changed the title ~~Fix error serialization~~Rewrite error serializationNov 3, 2022

rekmarks self-assigned this Nov 10, 2022

FrederikBolding unassigned rekmarks Nov 24, 2022

Gudahtt reviewed Nov 24, 2022
View reviewed changes

Comment threadsrc/utils.ts Outdated

kumavis reviewed Mar 8, 2023
View reviewed changes

Comment threadsrc/utils.ts Outdated

kumavis reviewed Mar 8, 2023
View reviewed changes

Comment threadsrc/utils.ts Outdated

kumavis reviewed Mar 8, 2023
View reviewed changes

Comment threadsrc/utils.ts Outdated

kumavis reviewed Mar 8, 2023
View reviewed changes

Comment threadsrc/utils.ts Outdated

FrederikBolding dismissed a stale review via 132112fMarch 9, 2023 10:06

FrederikBolding added 9 commits March 9, 2023 11:08

Fix error serialization
578fb33

Fix tests
3f8d1ce

Potential implementation
181f2e2

Use latest utils
b29f14a

Implement subset error
f7b90e4

Add cause property
7be7ca4

Typing
6e8737b

More implementation changes
42316b8

Simplify error serialization strategy
5a145b0

FrederikBolding force-pushed the fb/fix-error-serialization branch from 132112f to 5a145b0Compare March 9, 2023 10:08

Simplify
7c0a984

FrederikBolding marked this pull request as ready for review March 9, 2023 10:18

FrederikBolding requested a review from a team as a code owner March 9, 2023 10:18

FrederikBolding mentioned this pull request Mar 9, 2023
[WIP] Fix faulty error serialization logic #57
Closed

Gudahtt reviewed Mar 9, 2023
View reviewed changes

Comment threadsrc/utils.ts Outdated
Comment threadsrc/utils.ts Outdated
Comment threadsrc/utils.ts Outdated
Comment threadsrc/utils.ts Outdated

Fix PR comments
2b337bc

kumavis reviewed Mar 10, 2023
View reviewed changes

Comment threadsrc/utils.ts Outdated

kumavis reviewed Mar 10, 2023
View reviewed changes

Comment threadsrc/utils.ts Outdated

Simplify further
8abb62b

kumavis reviewed Mar 13, 2023
View reviewed changes

Comment threadsrc/utils.ts Outdated

kumavis reviewed Mar 13, 2023
View reviewed changes

Gudahtt reviewed Apr 11, 2023
View reviewed changes

Comment threadsrc/utils.ts

Gudahtt reviewed Apr 11, 2023
View reviewed changes

Comment threadsrc/utils.ts

Clarify doc string
9ddba1e

Gudahtt reviewed Apr 11, 2023
View reviewed changes

Comment threadsrc/utils.test.ts

Address PR comments
07fe7d4

Gudahtt previously approved these changes Apr 11, 2023
View reviewed changes

Add another test case
5349014

FrederikBolding dismissed Gudahtt’s stale review via 5349014April 11, 2023 12:56

Gudahtt approved these changes Apr 11, 2023
View reviewed changes

Mrtenz approved these changes Apr 11, 2023
View reviewed changes

FrederikBolding merged commit 8191913 into mainApr 11, 2023

FrederikBolding deleted the fb/fix-error-serialization branch April 11, 2023 13:00

grypez mentioned this pull request Sep 28, 2024
feat(kernel): Add Error as a possible params type of CommandReply. MetaMask/ocap-kernel#113
Merged

rekmarks mentioned this pull request Oct 7, 2024
feat: Preserve original messages during error serialization by default #158
Merged

Uh oh!

Uh oh!

Conversation

FrederikBolding commented Oct 27, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kumavisMar 13, 2023

Choose a reason for hiding this comment

Uh oh!

FrederikBoldingMar 14, 2023

Choose a reason for hiding this comment

Uh oh!

FrederikBoldingApr 11, 2023

Choose a reason for hiding this comment

Uh oh!

GudahttApr 11, 2023

Choose a reason for hiding this comment

Uh oh!

FrederikBoldingApr 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

socket-securityBot commented Apr 11, 2023

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FrederikBolding commented Oct 27, 2022•
edited
Loading