From 4a2ebe8b7d44d82706029c5ada73c97afb666799 Mon Sep 17 00:00:00 2001 From: Benjamin Date: Fri, 4 Sep 2026 14:39:05 +0000 Subject: [PATCH] Approve a dApp request on one screen, not two. Signing a dApp request meant reviewing it, pressing Sign, and then confirming again in a password dialog stacked on top of the review -- two screens for one decision, and the dialog covered the transaction the user was agreeing to. Collect the password in the approval footer instead. Pressing Sign is now the whole confirmation, with the transaction still on screen behind it. A wrong password is reported under the field and the request stays open for another try, rather than the popup deciding the attempt is over. Hardware accounts keep the dialog: those flows have device prompts and a QR exchange to run, and no password to collect. The button says "Sign with device" so it is clear which is coming. The new footer lives in one component shared by both approval screens, so the transaction and message flows cannot drift apart again. signTx and signData now route both the inline path and the device dialog through the same pair of return-to-dApp helpers. --- .../unit/ui/sign-data-page-render.test.js | 94 ++++++++--- src/test/unit/ui/sign-tx-refresh.test.js | 27 ++- src/ui/app/components/inlineSignAction.jsx | 156 ++++++++++++++++++ src/ui/app/pages/signData.jsx | 121 ++++++-------- src/ui/app/pages/signTx.jsx | 88 +++++----- 5 files changed, 340 insertions(+), 146 deletions(-) create mode 100644 src/ui/app/components/inlineSignAction.jsx diff --git a/src/test/unit/ui/sign-data-page-render.test.js b/src/test/unit/ui/sign-data-page-render.test.js index f2c21909..05e3fb0a 100644 --- a/src/test/unit/ui/sign-data-page-render.test.js +++ b/src/test/unit/ui/sign-data-page-render.test.js @@ -62,6 +62,7 @@ jest.mock('../../../api/loader', () => ({ })); const SignData = require('../../../ui/app/pages/signData').default; +const { ERROR } = require('../../../config/config'); const MESSAGE = 'Delegate to LUCEM pool'; const ORIGIN = 'https://magic-delegation.test'; @@ -104,6 +105,17 @@ const click = async (node) => { }); }; +const type = async (input, value) => { + const setter = Object.getOwnPropertyDescriptor( + window.HTMLInputElement.prototype, + 'value' + ).set; + await act(async () => { + setter.call(input, value); + input.dispatchEvent(new Event('input', { bubbles: true })); + }); +}; + describe('dApp sign message screen', () => { let closeSpy; @@ -171,38 +183,79 @@ describe('dApp sign message screen', () => { await act(async () => root.unmount()); }); - // Regression: signData called an undefined `capture(Events…)` after signing. - // confirmModal caught the ReferenceError and reported failure, so the dApp got - // an error instead of the signature the user had just approved. + test('the password is on the page, so signing takes no second dialog', async () => { + const { container, root } = await mount(); + + expect(byTestId(container, 'sign-data-password')).toBeTruthy(); + // Nothing to confirm in a dialog: the page itself is the confirmation. + expect( + [...document.querySelectorAll('button')].some( + (b) => b.textContent.trim() === 'Confirm' + ) + ).toBe(false); + // No password typed yet, so there is nothing to submit. + expect( + byTestId(container, 'sign-data-primary-action').hasAttribute('disabled') + ).toBe(true); + await act(async () => root.unmount()); + }); + + // Regression: signData called an undefined `capture(Events…)` after signing, + // and the caller reported that ReferenceError as failure — so the dApp got an + // error instead of the signature the user had just approved. test('a successful signature is returned to the dApp, not an error', async () => { const { container, root, controller } = await mount(); + await type(byTestId(container, 'sign-data-password'), 'pa$$word'); await click(byTestId(container, 'sign-data-primary-action')); - const input = document.querySelector('input[type="password"]'); - expect(input).toBeTruthy(); - await act(async () => { - const setter = Object.getOwnPropertyDescriptor( - window.HTMLInputElement.prototype, - 'value' - ).set; - setter.call(input, 'pa$$word'); - input.dispatchEvent(new Event('input', { bubbles: true })); - }); - - const confirm = [...document.querySelectorAll('button')].find( - (b) => b.textContent.trim() === 'Confirm' + expect(mockSignDataCIP30).toHaveBeenCalledWith( + expect.any(String), + expect.any(String), + 'pa$$word', + 0 ); - expect(confirm).toBeTruthy(); - await click(confirm); - - expect(mockSignDataCIP30).toHaveBeenCalled(); expect(controller.returnData).toHaveBeenCalledWith({ data: 'signed_cip30', }); expect(controller.returnData).not.toHaveBeenCalledWith( expect.objectContaining({ error: expect.anything() }) ); + expect(closeSpy).toHaveBeenCalled(); + await act(async () => root.unmount()); + }); + + test('a wrong password is reported in place, without ending the request', async () => { + mockSignDataCIP30.mockRejectedValueOnce(ERROR.wrongPassword); + const { container, root, controller } = await mount(); + + await type(byTestId(container, 'sign-data-password'), 'nope'); + await click(byTestId(container, 'sign-data-primary-action')); + + expect(byTestId(container, 'sign-data-wrong-password')).toBeTruthy(); + // The dApp is still waiting, and the popup stays open for another try. + expect(controller.returnData).not.toHaveBeenCalled(); + expect(closeSpy).not.toHaveBeenCalled(); + + mockSignDataCIP30.mockResolvedValueOnce('signed_cip30'); + await type(byTestId(container, 'sign-data-password'), 'pa$$word'); + await click(byTestId(container, 'sign-data-primary-action')); + expect(controller.returnData).toHaveBeenCalledWith({ + data: 'signed_cip30', + }); + await act(async () => root.unmount()); + }); + + test('a failure other than a wrong password is handed back to the dApp', async () => { + const boom = new Error('signing blew up'); + mockSignDataCIP30.mockRejectedValueOnce(boom); + const { container, root, controller } = await mount(); + + await type(byTestId(container, 'sign-data-password'), 'pa$$word'); + await click(byTestId(container, 'sign-data-primary-action')); + + expect(controller.returnData).toHaveBeenCalledWith({ error: boom }); + expect(closeSpy).toHaveBeenCalled(); await act(async () => root.unmount()); }); @@ -216,6 +269,7 @@ describe('dApp sign message screen', () => { expect( byTestId(container, 'sign-data-primary-action').hasAttribute('disabled') ).toBe(true); + expect(byTestId(container, 'sign-data-password')).toBeNull(); await act(async () => root.unmount()); }); }); diff --git a/src/test/unit/ui/sign-tx-refresh.test.js b/src/test/unit/ui/sign-tx-refresh.test.js index 079d4bbc..58d43f1b 100644 --- a/src/test/unit/ui/sign-tx-refresh.test.js +++ b/src/test/unit/ui/sign-tx-refresh.test.js @@ -22,6 +22,10 @@ const internalSrc = fs.readFileSync( path.join(__dirname, '../../../ui/indexInternal.jsx'), 'utf8' ); +const inlineActionSrc = fs.readFileSync( + path.join(__dirname, '../../../ui/app/components/inlineSignAction.jsx'), + 'utf8' +); describe('CIP-30 sign UI refresh — structural contracts', () => { test('uses themed page chrome instead of a leftover gray card', () => { @@ -63,14 +67,29 @@ describe('CIP-30 sign UI refresh — structural contracts', () => { test('Sign footer stays in the popup — same pattern as Send', () => { expect(signSrc).toContain('data-testid="sign-tx-footer"'); expect(signSrc).toContain('lucem-sign-footer'); - expect(signSrc).toContain('data-testid="sign-tx-primary-action"'); - expect(signSrc).toContain("bg=\"yellow.400\""); - expect(signSrc).toContain('fontWeight="black"'); - expect(signSrc).toContain('data-testid="sign-tx-cancel"'); expect(signSrc).toContain('safe-area-inset-bottom'); expect(stylesSrc).toMatch( /html\[data-layout=['"]extension['"]\] \.lucem-sign-footer/ ); + // The footer actions live in InlineSignAction, which derives the testids + // sign-tx-primary-action / sign-tx-cancel from this prefix. + expect(signSrc).toContain(' { + // Password entry is inline, so approving is one screen. Only hardware + // accounts, which have device prompts to run, open the dialog. + expect(signSrc).toMatch( + /isHw=\{isHW\(account\.index\)\}[\s\S]{0,800}onHwRequest=\{\(\) =>\s*ref\.current\.openModal\(account\.index\)\}/ + ); + expect(inlineActionSrc).toContain('${testId}-password'); + expect(inlineActionSrc).toContain('ERROR.wrongPassword'); + expect(inlineActionSrc).toContain('autoComplete="current-password"'); }); test('keeps origin, Details, and dApp decline/sign wiring', () => { diff --git a/src/ui/app/components/inlineSignAction.jsx b/src/ui/app/components/inlineSignAction.jsx new file mode 100644 index 00000000..b493cc1e --- /dev/null +++ b/src/ui/app/components/inlineSignAction.jsx @@ -0,0 +1,156 @@ +import React from 'react'; +import { + Button, + Input, + InputGroup, + InputRightElement, + Stack, + Text, +} from '@chakra-ui/react'; +import { ERROR } from '../../../config/config'; +import useSurfaceColors from '../hooks/useSurfaceColors'; + +/** + * Password entry and the confirm button, in the footer of a dApp approval + * screen. Approving a request is then a single screen rather than a review + * screen plus a password dialog stacked on top of it. + * + * Hardware accounts keep their own dialog: those flows are genuinely + * multi-step (device prompts, QR exchange) and have no password to collect. + */ +const InlineSignAction = ({ + testId, + label, + cancelLabel = 'Cancel', + isHw, + isDisabled, + sign, + onSigned, + onFailed, + onHwRequest, + onCancel, +}) => { + const { pageFg, mutedFg, inputBg, inputBorder, placeholder } = + useSurfaceColors(); + const [password, setPassword] = React.useState(''); + const [show, setShow] = React.useState(false); + const [busy, setBusy] = React.useState(false); + const [wrongPassword, setWrongPassword] = React.useState(false); + + const canSubmit = !isDisabled && !busy && (isHw || password.length > 0); + + const submit = async () => { + if (!canSubmit) return; + if (isHw) { + onHwRequest(); + return; + } + setBusy(true); + setWrongPassword(false); + try { + const result = await sign(password); + setPassword(''); + await onSigned(result); + } catch (e) { + if (e === ERROR.wrongPassword) setWrongPassword(true); + else await onFailed(e); + setBusy(false); + } + }; + + return ( + + {!isHw && !isDisabled ? ( + + + { + setWrongPassword(false); + setPassword(e.target.value); + }} + onKeyDown={(e) => { + if (e.key === 'Enter') submit(); + }} + /> + + + + + {wrongPassword ? ( + + Wrong password. + + ) : null} + + ) : null} + + + + ); +}; + +export default InlineSignAction; diff --git a/src/ui/app/pages/signData.jsx b/src/ui/app/pages/signData.jsx index 48a9ba53..6ed5abb8 100644 --- a/src/ui/app/pages/signData.jsx +++ b/src/ui/app/pages/signData.jsx @@ -7,16 +7,9 @@ import { } from '../../../api/extension'; import platform from '../../../platform'; import Account from '../components/account'; -import { - Box, - Button, - Flex, - Image, - Spinner, - Stack, - Text, -} from '@chakra-ui/react'; +import { Box, Flex, Image, Spinner, Stack, Text } from '@chakra-ui/react'; import ConfirmModal from '../components/confirmModal'; +import InlineSignAction from '../components/inlineSignAction'; import Loader from '../../../api/loader'; import { DataSignError } from '../../../config/config'; import useSurfaceColors from '../hooks/useSurfaceColors'; @@ -105,6 +98,37 @@ const SignData = ({ request, controller }) => { setIsLoading(false); }; + const signPayload = (password) => + request.data.CIP30 + ? signDataCIP30( + request.data.address, + request.data.payload, + password, + account.index + ) + : // deprecated soon + signData( + request.data.address, + request.data.payload, + password, + account.index + ); + + const returnSignature = async (signedMessage) => { + await controller.returnData({ data: signedMessage }); + window.close(); + }; + + const returnSignError = async (signError) => { + await controller.returnData({ error: signError }); + window.close(); + }; + + const decline = async () => { + await controller.returnData({ error: DataSignError.UserDeclined }); + window.close(); + }; + React.useEffect(() => { loadData(); }, []); @@ -280,76 +304,29 @@ const SignData = ({ request, controller }) => { {error} ) : null} - - + sign={signPayload} + onSigned={returnSignature} + onFailed={returnSignError} + onHwRequest={() => ref.current.openModal(account.index)} + onCancel={decline} + /> - request.data.CIP30 - ? signDataCIP30( - request.data.address, - request.data.payload, - password, - account.index - ) - : // deprecated soon - signData( - request.data.address, - request.data.payload, - password, - account.index - ) - } + sign={signPayload} onCloseBtn={() => {}} - onConfirm={async (status, signedMessage) => { - if (status === true) { - await controller.returnData({ data: signedMessage }); - } else { - await controller.returnData({ error: signedMessage }); - } - window.close(); - }} + onConfirm={async (status, signedMessage) => + status === true + ? returnSignature(signedMessage) + : returnSignError(signedMessage) + } /> ); diff --git a/src/ui/app/pages/signTx.jsx b/src/ui/app/pages/signTx.jsx index 80a4b2d8..2b32a895 100644 --- a/src/ui/app/pages/signTx.jsx +++ b/src/ui/app/pages/signTx.jsx @@ -6,6 +6,7 @@ import { getCurrentAccount, getSpecificUtxo, getUtxos, + isHW, signTx, signTxHW, } from '../../../api/extension'; @@ -39,6 +40,7 @@ import { useDisclosure, } from '@chakra-ui/react'; import AssetsModal from '../components/assetsModal'; +import InlineSignAction from '../components/inlineSignAction'; import { AnimatedQRCode, AnimatedQRScanner } from '@keystonehq/animated-qr'; import { URType } from '@keystonehq/keystone-sdk'; import KeystoneSDK from '@keystonehq/keystone-sdk'; @@ -676,6 +678,18 @@ const SignTx = ({ request, controller }) => { window.close(); }; + const returnSignedTx = async (signedTx) => { + await controller.returnData({ + data: Buffer.from(signedTx.to_bytes()).toString('hex'), + }); + window.close(); + }; + + const returnSignError = async (error) => { + await controller.returnData({ error }); + window.close(); + }; + React.useEffect(() => { getInfo(); }, []); @@ -1030,46 +1044,25 @@ const SignTx = ({ request, controller }) => { {isLoading.error} )} - - + + signTx( + request.data.tx, + keyHashes.key, + password, + account.index, + request.data.partialSign + ) + } + onSigned={returnSignedTx} + onFailed={returnSignError} + onHwRequest={() => ref.current.openModal(account.index)} + onCancel={declineRequest} + /> @@ -1107,16 +1100,11 @@ const SignTx = ({ request, controller }) => { request.data.partialSign ); }} - onConfirm={async (status, signedTx) => { - if (status === true) { - await controller.returnData({ - data: Buffer.from(signedTx.to_bytes()).toString('hex'), - }); - } else { - await controller.returnData({ error: signedTx }); - } - window.close(); - }} + onConfirm={async (status, signedTx) => + status === true + ? returnSignedTx(signedTx) + : returnSignError(signedTx) + } />