fix(bitcoin-wallet-snap): return Json directly from onKeyringRequest

[hmalik88]

Explanation

Bitcoin wallet snap v2.0.0 incorrectly wrapped all onKeyringRequest responses in the v1 { pending: false, result } envelope. MetaMask detects v2 snaps via the capabilities field in the manifest and skips envelope unwrapping, so callers received the wrapper as the result instead of the inner value. This broke signMessage, signPsbt, sendTransaction, and other signing operations.

Removes #toKeyringResponse and updates all handlers to return Json directly, as the v2 keyring protocol requires.

References

N/A

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've communicated my changes to consumers by updating changelogs for packages I've changed
• I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

[ccharly]

My bad for not paying attention on this one! Looks like we only use the Solana Snap to do submitRequest calls!

[ulissesferreira]

I have one concern with these type transformations, Json takes away a lot of information about what the objects actually have. Is this the only way? Isn't there a more fitting type?

[hmalik88]

be6d7f2

[ulissesferreira]

To add to my comment above: wouldn't it make sense that the Keyring respecting methods return a type that is all-encompassing, like Json but then the private methods inside actually return a specific Json of what is happening?

[ccharly]

To add to my comment above: wouldn't it make sense that the Keyring respecting methods return a type that is all-encompassing, like Json but then the private methods inside actually return a specific Json of what is happening?

IMO that makes total sense, and that's much better if we use stricter types! As long as those types are Json compatible, we should be good!

[taran-a]

@hmalik88 does it mean that we have to deprecate v2.0.0 of the bitcoin snap ?

Also I was testing preview build, including the send bitcoin flow, and that trx was signed, sent and confirmed ? I can see from the description that it should fail with v2.0.0

[hmalik88]

@hmalik88 does it mean that we have to deprecate v2.0.0 of the bitcoin snap ?

Also I was testing preview build, including the send bitcoin flow, and that trx was signed, sent and confirmed ? I can see from the description that it should fail with v2.0.0

Yes we should deprecate once the fix is released to avoid anyone pinning to the wrong version. Could you share which extension build/version you were testing against? Depending on that, it might explain why send appeared to work. The behavior of v2.0.0 differs based on whether the extension was doing envelope unwrapping or not

[hmalik88]

@metamaskbot publish-preview

[github-actions]

Preview builds have been published. Learn how to use preview builds in other projects.

Expand for full list of packages and versions.

@metamask-previews/bitcoin-wallet-snap@2.0.0-preview-be6d7f2c
@metamask-previews/snap-networks-utils@0.0.0-preview-be6d7f2c
@metamask-previews/tron-wallet-snap@2.0.0-preview-be6d7f2c

[taran-a]

@hmalik88 does it mean that we have to deprecate v2.0.0 of the bitcoin snap ?
Also I was testing preview build, including the send bitcoin flow, and that trx was signed, sent and confirmed ? I can see from the description that it should fail with v2.0.0

Yes we should deprecate once the fix is released to avoid anyone pinning to the wrong version. Could you share which extension build/version you were testing against? Depending on that, it might explain why send appeared to work. The behavior of v2.0.0 differs based on whether the extension was doing envelope unwrapping or not

I usually test with the latest main branch of the extension or mobile app so I can't tell which commit, sorry (