Conversation
Contributor
Author
|
@metamaskbot publish-previe |
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. |
ccharly
commented
Mar 25, 2026
| * @param error - The caught error value. | ||
| * @returns The error message string. | ||
| */ | ||
| export function toErrorMessage(error: unknown): string { |
Contributor
Author
There was a problem hiding this comment.
Added this mostly because it was hard to get coverage when re-using this pattern in the fix I made in formatGroupForUserStorageUsage.
Otherwise I would have to mock superstruct.mask just to throw non-Error errors, and that felt like a bit of an anti-pattern.
Delegating this coverage to this helper makes the code easier and keep the test focused on the new behavior fix (IMO)!
Contributor
Author
There was a problem hiding this comment.
(I used a similar pattern in the service too, toErrorMessage makes things a bit more readable IMO)
mathieuartu
reviewed
Mar 25, 2026
Contributor
mathieuartu
left a comment
mathieuartu
left a comment
There was a problem hiding this comment.
LGTM, just one curiosity question before approving 😛
| ); | ||
|
|
||
| // If anything goes wrong with this group, we use blank metadata for it. | ||
| return { groupIndex }; |
Contributor
There was a problem hiding this comment.
What's the rationale behind keeping groupIndex here? Does this help the account tree to be built properly?
Contributor
Author
There was a problem hiding this comment.
I just kept the semantic of the function (it couldn't fail/throw before we started introducing the mask call), so in case an error happens, I don't want to make it throw either.
IDK if we can do better than that actually 🤔 the way I see this is that, we need at least the groupIndex, everything else (for now) is optional, so worst case scenario, we omit all metadata and go with the groupIndex.
Would you prefer to throw instead, so this could bubble up and potentially make B&S fail entirely (I haven't analyzed the call sites TBH 🙊)
Contributor
There was a problem hiding this comment.
Thanks! No, I actually didn't think that far ahead, I just wanted to understand your intent. I think your logic is sound :)
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
2 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
Since the introduction of
lastSelectedmetadata (which should not be synced), backup & sync was not accepting the data coming back from user-storage. Mostly because the payload was like this:"{\"name\":{\"value\":\"Account 1\",\"lastUpdatedAt\":0},\"pinned\":{\"value\":false,\"lastUpdatedAt\":0},\"hidden\":{\"value\":false,\"lastUpdatedAt\":0},\"lastSelected\":0,\"groupIndex\":0}" "{\"name\":{\"value\":\"Account 2\",\"lastUpdatedAt\":0},\"pinned\":{\"value\":false,\"lastUpdatedAt\":0},\"hidden\":{\"value\":false,\"lastUpdatedAt\":0},\"lastSelected\":0,\"groupIndex\":1}"The extra
lastSelectedshouldn't have been part of that payload. This was making thesuperstructvalidation to fail when getting those data back, thus, preventing account syncing to finally restore accounts.We now
maskthe extra properties that are not part of backup & sync schema.References
N/A
Checklist
Note
Medium Risk
Touches backup/sync serialization and error handling; incorrect masking/fallback could cause some group metadata (e.g., name/pin/hide) not to persist or to be dropped in edge cases.
Overview
Fixes backup & sync group serialization to drop extra/invalid metadata fields before writing to user storage by masking group metadata against
UserStorageSyncedWalletGroupSchema(preventing fields likelastSelectedfrom breaking later reads/validation), with a safe fallback to minimal{ groupIndex }plus logging.Standardizes unknown-error stringification via a new
toErrorMessageutility and applies it across backup/sync error paths, alongside new/updated unit tests and a changelog entry documenting the user-storage filtering fix.Written by Cursor Bugbot for commit cfa55f2. This will update automatically on new commits. Configure here.