feat(snap-account-service): report migration error - #9696
Merged
ccharly merged 12 commits intoJul 30, 2026
Conversation
ccharly
commented
Jul 29, 2026
| }); | ||
| }, | ||
| (error) => { | ||
| reportError(this.#messenger, 'Migration failed after unlock', error); |
Contributor
Author
There was a problem hiding this comment.
No real sensitive data (from error) should escape from here. I ran multiple checks with claude on this.
This can be compared with living an error bubbles up from a :withKeyring* call basically, which means, errors from keyring.{serialize,deserialize} could be thrown here.
In this context, we mostly use Snap keyrings (legacy + Snap keyring v2), so we could expect errors coming from there.
The real errors we don't have much control over are superstruct errors. We now wrap sensitive fields with our new decorator (superstruct.sensitive), but it's not widely used yet.
The other alternative is to wrap each steps of the migration with various try { ... } catch { throw Error('This step failed with ${text}'); }, but that would make any analysis still pretty difficult I guess...
Contributor
Author
There was a problem hiding this comment.
Ok, finally introduce a new pattern where we only report errors we know are "safe" to be reported.
It's not perfect, but I prefer this solution for now. Once we know why the migration is failing, we'll fix and eventually, remove those SafeErrors.
Though, I think the pattern is elegant enough that we could use it in place where we need it.
On a side note, we should over-abuse our new superstruct.sensitive too to avoid letting dynamic value escape through superstruct validation IMO. But for now, it's not widespread enough.
hmalik88
reviewed
Jul 30, 2026
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cc380a1. Configure here.
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
mathieuartu
reviewed
Jul 30, 2026
mathieuartu
reviewed
Jul 30, 2026
mathieuartu
reviewed
Jul 30, 2026
Co-authored-by: Mathieu Artu <mathieu.artu@consensys.net>
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
Right now, we only
console.logif the migration fails (which is not supposed to happen). We have seen some user reports (with the console logMigration failed during unlock), though, it's hard to assess what's the underlying error of this failure.We now want to instrument each steps and report those error properly so we can (hopefully) have a better understanding of what's the real failure.
References
N/A
Checklist
Note
Medium Risk
Touches mandatory unlock-time keyring migration and error reporting; behavior on failure is clearer but migration logic itself is unchanged aside from error wrapping.
Overview
Snap keyring v2 migration failures are now reported to Sentry instead of only logging to the console, with messages sanitized so addresses and other sensitive data are not sent upstream.
A new
errorsmodule introducesSafeError,withSafeError, andreportError. Migration steps (addNewKeyring,removeKeyring, and the overall unlock-time migration) run insidewithSafeError, which keepsKeyringControllerErrorand existingSafeErrors as-is, turns superstructStructErrors into path/type-only summaries, and replaces other errors with generic step-labeled messages. On unlock failure,reportErrorlogs and calls the messenger’s optionalcaptureExceptionwith a wrapper error whosecauseis the (sanitized) failure.Tests cover sanitization behavior, Sentry wiring, and migration error paths; the changelog documents the addition.
SnapTrackergains a small test that removal events for untracked Snaps are ignored.Reviewed by Cursor Bugbot for commit a1d0bbf. Bugbot is set up for automated code reviews on this repo. Configure here.