fix: show filled PSBT in signPsbt confirmation

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]
 

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<KeyringRequest>({
+        origin,
+        request: {
+          method: AccountCapability.SignPsbt,
+          params: {
+            ...accountParam,
+            psbt: 'psbtBase64',
+            feeRate: 3,
+            options: fillOptions,
+          },
+        },
+        account: 'account-id',
+      });
+
+      const filledPsbt = mock<Psbt>({
+        toString: jest.fn().mockReturnValue('filledPsbtBase64'),
+      });
+      const psbtForConfirmation = mock<Psbt>();
+      const psbtForSigning = mock<Psbt>();
+      mockAccountsUseCases.fillPsbt.mockResolvedValue(filledPsbt);
+      mockAccountsUseCases.signPsbt.mockResolvedValue({
+        psbt: 'signedPsbtBase64',
+        txid: mock<Txid>({
+          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<KeyringRequest>({
+        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);

packages/snap/src/handlers/KeyringRequestHandler.ts

@@ -120,22 +120,30 @@ export class KeyringRequestHandler {
     options: { fill: boolean; broadcast: boolean },
     feeRate?: number,
   ): Promise<KeyringResponse> {
-    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({