diff --git a/packages/snap/CHANGELOG.md b/packages/snap/CHANGELOG.md index bf02a33a..3b7863d5 100644 --- a/packages/snap/CHANGELOG.md +++ b/packages/snap/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Preserve original error message and class when wrapping unknown errors at the handler boundary ([#600](https://github.com/MetaMask/snap-bitcoin-wallet/pull/600)) +- Show the filled PSBT in the `signPsbt` confirmation dialog so the displayed transaction matches the one being signed ([#606](https://github.com/MetaMask/snap-bitcoin-wallet/pull/606)) ## [1.10.1] diff --git a/packages/snap/src/handlers/KeyringRequestHandler.test.ts b/packages/snap/src/handlers/KeyringRequestHandler.test.ts index 2792d4ba..a7265fd1 100644 --- a/packages/snap/src/handlers/KeyringRequestHandler.test.ts +++ b/packages/snap/src/handlers/KeyringRequestHandler.test.ts @@ -110,6 +110,7 @@ describe('KeyringRequestHandler', () => { SignPsbtRequest, ); expect(mockAccountsUseCases.get).toHaveBeenCalledWith('account-id'); + expect(mockAccountsUseCases.fillPsbt).not.toHaveBeenCalled(); expect(mockConfirmationRepository.insertSignPsbt).toHaveBeenCalledWith( mockAccount, mockPsbt, @@ -120,7 +121,7 @@ describe('KeyringRequestHandler', () => { 'account-id', mockPsbt, 'metamask', - mockOptions, + { fill: false, broadcast: true }, 3, ); expect(result).toStrictEqual({ @@ -129,6 +130,76 @@ describe('KeyringRequestHandler', () => { }); }); + it('fills the PSBT before showing the confirmation when options.fill is true', async () => { + const fillOptions = { fill: true, broadcast: true }; + const fillRequest = mock({ + origin, + request: { + method: AccountCapability.SignPsbt, + params: { + ...accountParam, + psbt: 'psbtBase64', + feeRate: 3, + options: fillOptions, + }, + }, + account: 'account-id', + }); + + const filledPsbt = mock({ + toString: jest.fn().mockReturnValue('filledPsbtBase64'), + }); + const psbtForConfirmation = mock(); + const psbtForSigning = mock(); + mockAccountsUseCases.fillPsbt.mockResolvedValue(filledPsbt); + mockAccountsUseCases.signPsbt.mockResolvedValue({ + psbt: 'signedPsbtBase64', + txid: mock({ + toString: jest.fn().mockReturnValue('txid'), + }), + }); + jest + .mocked(parsePsbt) + .mockReturnValueOnce(mockPsbt) + .mockReturnValueOnce(psbtForConfirmation) + .mockReturnValueOnce(psbtForSigning); + + await handler.route(fillRequest); + + const fillOrder = + mockAccountsUseCases.fillPsbt.mock.invocationCallOrder[0]; + const insertOrder = + mockConfirmationRepository.insertSignPsbt.mock.invocationCallOrder[0]; + const signOrder = + mockAccountsUseCases.signPsbt.mock.invocationCallOrder[0]; + + expect(fillOrder).toBeLessThan(insertOrder as number); + expect(insertOrder).toBeLessThan(signOrder as number); + + expect(mockAccountsUseCases.fillPsbt).toHaveBeenCalledWith( + 'account-id', + mockPsbt, + 3, + ); + expect(parsePsbt).toHaveBeenNthCalledWith(1, 'psbtBase64'); + expect(parsePsbt).toHaveBeenNthCalledWith(2, 'filledPsbtBase64'); + expect(parsePsbt).toHaveBeenNthCalledWith(3, 'filledPsbtBase64'); + expect(mockConfirmationRepository.insertSignPsbt).toHaveBeenCalledWith( + mockAccount, + psbtForConfirmation, + 'metamask', + fillOptions, + ); + expect(mockAccountsUseCases.signPsbt).toHaveBeenCalledWith( + 'account-id', + psbtForSigning, + 'metamask', + { fill: false, broadcast: true }, + 3, + ); + expect(psbtForSigning).not.toBe(psbtForConfirmation); + }); + it('returns null txid when signPsbt result has no txid', async () => { mockAccountsUseCases.signPsbt.mockResolvedValue({ psbt: 'psbtBase64', @@ -155,6 +226,30 @@ describe('KeyringRequestHandler', () => { expect(mockAccountsUseCases.signPsbt).not.toHaveBeenCalled(); }); + it('does not show confirmation or sign if fillPsbt fails', async () => { + const fillOptions = { fill: true, broadcast: true }; + const fillRequest = mock({ + origin, + request: { + method: AccountCapability.SignPsbt, + params: { + ...accountParam, + psbt: 'psbtBase64', + feeRate: 3, + options: fillOptions, + }, + }, + account: 'account-id', + }); + const error = new Error('fee rate too high'); + mockAccountsUseCases.fillPsbt.mockRejectedValue(error); + + await expect(handler.route(fillRequest)).rejects.toThrow(error); + + expect(mockConfirmationRepository.insertSignPsbt).not.toHaveBeenCalled(); + expect(mockAccountsUseCases.signPsbt).not.toHaveBeenCalled(); + }); + it('propagates errors from parsePsbt', async () => { const error = new Error('parsePsbt'); jest.mocked(parsePsbt).mockImplementationOnce(() => { @@ -166,7 +261,11 @@ describe('KeyringRequestHandler', () => { ...mockRequest, request: { ...mockRequest.request, - params: { ...accountParam, psbt: 'invalidPsbt' }, + params: { + ...accountParam, + psbt: 'invalidPsbt', + options: mockOptions, + }, }, }), ).rejects.toThrow(error); diff --git a/packages/snap/src/handlers/KeyringRequestHandler.ts b/packages/snap/src/handlers/KeyringRequestHandler.ts index bf1a3f4d..6375bc8b 100644 --- a/packages/snap/src/handlers/KeyringRequestHandler.ts +++ b/packages/snap/src/handlers/KeyringRequestHandler.ts @@ -120,22 +120,30 @@ export class KeyringRequestHandler { options: { fill: boolean; broadcast: boolean }, feeRate?: number, ): Promise { - const psbt = parsePsbt(psbtBase64); const account = await this.#accountsUseCases.get(id); + const psbtBase64ToSign = options.fill + ? ( + await this.#accountsUseCases.fillPsbt( + id, + parsePsbt(psbtBase64), + feeRate, + ) + ).toString() + : psbtBase64; + await this.#confirmationRepository.insertSignPsbt( account, - psbt, + parsePsbt(psbtBase64ToSign), origin, options, ); - // Creates a fresh PSBT from the original base64 because the original PSBT is mutated by the confirmation repository const { psbt: signedPsbt, txid } = await this.#accountsUseCases.signPsbt( id, - parsePsbt(psbtBase64), + parsePsbt(psbtBase64ToSign), origin, - options, + { ...options, fill: false }, feeRate, ); return this.#toKeyringResponse({