Skip to content

Conversation

@baptiste-marchand
Copy link

@baptiste-marchand baptiste-marchand commented Nov 14, 2025

Implements account address resolution for Bitcoin requests, allowing MetaMask to route non-EVM dapp signing requests correctly. This is a requirement for Bitcoin dApps connectivity

  • Introduces the resolveAccountAddress method in KeyringHandler and KeyringRequestHandler to determine the account address for signing requests.
  • Adds necessary data structures and validation logic for Bitcoin request parameters.

Note

Implements BTC account address resolution and request validation, adds send transfer confirmation (with origin), and updates handlers/tests to require account in BTC request params.

  • Keyring/Request Handling:
    • Add resolveAccountAddress to KeyringHandler (routes keyring_resolveAccountAddress) and KeyringRequestHandler (returns CAIP-10 address from BTC requests).
    • Introduce BTC request structs (BtcWalletRequestStruct, per-method structs) and validate incoming requests; add NetworkStruct for scope validation.
    • Require account object in params for BTC methods (SignPsbt, FillPsbt, ComputeFee, BroadcastPsbt, SendTransfer, GetUtxo, SignMessage).
  • Use Cases:
    • Update sendTransfer to enforce exactly one recipient, build PSBT with fee rate and frozen UTXOs, and show a confirmation before signing/broadcasting.
  • Confirmation/UI:
    • Extend ConfirmationRepository with insertSendTransfer and implement in JSXConfirmationRepository using UnifiedSendFormView; display request origin.
    • Wire chainClient into confirmation repository; UnifiedSendFormView now shows dynamic origin.
  • Tests:
    • Expand unit/integration tests to cover address resolution, new validation, required account param, and updated flows.

Written by Cursor Bugbot for commit 62f251f. This will update automatically on new commits. Configure here.

@baptiste-marchand baptiste-marchand requested a review from a team as a code owner November 14, 2025 17:40
@baptiste-marchand baptiste-marchand changed the title feat: enables account address resolution to support dApps connectivity feat: enable account address resolution to support dApps connectivity Nov 14, 2025
});

export const SignMessageRequest = object({
account: WalletAccountStruct,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Parameter Redundancy Breaks Backward Compatibility.

Adding the required account field to all Bitcoin request parameter structs breaks backward compatibility. The existing route method already receives the account ID from the KeyringRequest wrapper object, and all existing requests (as shown in integration tests) don't include an account field in their params. The assert statements in the route method will now reject all existing requests that lack this field, breaking all current integrations. The account field in params is redundant since the account is already available from the KeyringRequest wrapper.

Fix in Cursor Fix in Web

@baptiste-marchand baptiste-marchand force-pushed the feat/bitcoin-address-resolution branch from a041ba7 to e0ea92e Compare November 17, 2025 15:19
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Account Field Mandate Breaks Tests

Existing unit tests create request params without the required account field, but the request struct changes now mandate this field. Tests for signPsbt, computeFee, fillPsbt, broadcastPsbt, sendTransfer, getUtxo, and signMessage will fail during assertion validation because their params lack the required account: { address: string } object.

packages/snap/src/handlers/KeyringRequestHandler.test.ts#L64-L431

const mockRequest = mock<KeyringRequest>({
origin,
request: {
method: AccountCapability.SignPsbt,
params: {
psbt: 'psbtBase64',
feeRate: 3,
options: mockOptions,
},
},
account: 'account-id',
});
it('executes signPsbt', async () => {
mockAccountsUseCases.signPsbt.mockResolvedValue({
psbt: 'psbtBase64',
txid: mock<Txid>({
toString: jest.fn().mockReturnValue('txid'),
}),
});
const result = await handler.route(mockRequest);
expect(assert).toHaveBeenCalledWith(
mockRequest.request.params,
SignPsbtRequest,
);
expect(mockAccountsUseCases.signPsbt).toHaveBeenCalledWith(
'account-id',
mockPsbt,
'metamask',
mockOptions,
3,
);
expect(result).toStrictEqual({
pending: false,
result: { psbt: 'psbtBase64', txid: 'txid' },
});
});
it('propagates errors from parsePsbt', async () => {
const error = new Error('parsePsbt');
jest.mocked(parsePsbt).mockImplementationOnce(() => {
throw error;
});
await expect(
handler.route({
...mockRequest,
request: { ...mockRequest.request, params: { psbt: 'invalidPsbt' } },
}),
).rejects.toThrow(error);
expect(mockAccountsUseCases.signPsbt).not.toHaveBeenCalled();
});
it('propagates errors from signPsbt', async () => {
const error = new Error();
mockAccountsUseCases.signPsbt.mockRejectedValue(error);
await expect(handler.route(mockRequest)).rejects.toThrow(error);
expect(mockAccountsUseCases.signPsbt).toHaveBeenCalled();
});
});
describe('computeFee', () => {
const mockRequest = mock<KeyringRequest>({
request: {
method: AccountCapability.ComputeFee,
params: {
psbt: 'psbtBase64',
feeRate: 3,
},
},
account: 'account-id',
});
it('executes computeFee', async () => {
mockAccountsUseCases.computeFee.mockResolvedValue(
mock<Amount>({
// eslint-disable-next-line @typescript-eslint/naming-convention
to_sat: () => BigInt(1000),
}),
);
const result = await handler.route(mockRequest);
expect(assert).toHaveBeenCalledWith(
mockRequest.request.params,
ComputeFeeRequest,
);
expect(mockAccountsUseCases.computeFee).toHaveBeenCalledWith(
'account-id',
mockPsbt,
3,
);
expect(result).toStrictEqual({
pending: false,
result: { fee: '1000' },
});
});
it('propagates errors from parsePsbt', async () => {
const error = new Error('parsePsbt');
jest.mocked(parsePsbt).mockImplementationOnce(() => {
throw error;
});
await expect(
handler.route({
...mockRequest,
request: { ...mockRequest.request, params: { psbt: 'invalidPsbt' } },
}),
).rejects.toThrow(error);
expect(mockAccountsUseCases.computeFee).not.toHaveBeenCalled();
});
it('propagates errors from computeFee', async () => {
const error = new Error();
mockAccountsUseCases.computeFee.mockRejectedValue(error);
await expect(handler.route(mockRequest)).rejects.toThrow(error);
expect(mockAccountsUseCases.computeFee).toHaveBeenCalled();
});
});
describe('fillPsbt', () => {
const mockRequest = mock<KeyringRequest>({
request: {
method: AccountCapability.FillPsbt,
params: {
psbt: 'psbtBase64',
feeRate: 3,
},
},
account: 'account-id',
});
it('executes fillPsbt', async () => {
const mockFilledPsbt = mock<Psbt>({
toString: jest.fn().mockReturnValue('filledPsbtBase64'),
});
mockAccountsUseCases.fillPsbt.mockResolvedValue(mockFilledPsbt);
const result = await handler.route(mockRequest);
expect(assert).toHaveBeenCalledWith(
mockRequest.request.params,
FillPsbtRequest,
);
expect(mockAccountsUseCases.fillPsbt).toHaveBeenCalledWith(
'account-id',
mockPsbt,
3,
);
expect(result).toStrictEqual({
pending: false,
result: { psbt: 'filledPsbtBase64' },
});
});
it('propagates errors from parsePsbt', async () => {
const error = new Error('parsePsbt');
jest.mocked(parsePsbt).mockImplementationOnce(() => {
throw error;
});
await expect(
handler.route({
...mockRequest,
request: { ...mockRequest.request, params: { psbt: 'invalidPsbt' } },
}),
).rejects.toThrow(error);
expect(mockAccountsUseCases.fillPsbt).not.toHaveBeenCalled();
});
it('propagates errors from fillPsbt', async () => {
const error = new Error();
mockAccountsUseCases.fillPsbt.mockRejectedValue(error);
await expect(handler.route(mockRequest)).rejects.toThrow(error);
expect(mockAccountsUseCases.fillPsbt).toHaveBeenCalled();
});
});
describe('broadcastPsbt', () => {
const mockRequest = mock<KeyringRequest>({
origin,
request: {
method: AccountCapability.BroadcastPsbt,
params: {
psbt: 'psbtBase64',
feeRate: 3,
},
},
account: 'account-id',
});
it('executes broadcastPsbt', async () => {
const mockTxid = mock<Txid>({
toString: jest.fn().mockReturnValue('txid'),
});
mockAccountsUseCases.broadcastPsbt.mockResolvedValue(mockTxid);
const result = await handler.route(mockRequest);
expect(assert).toHaveBeenCalledWith(
mockRequest.request.params,
BroadcastPsbtRequest,
);
expect(mockAccountsUseCases.broadcastPsbt).toHaveBeenCalledWith(
'account-id',
mockPsbt,
origin,
);
expect(result).toStrictEqual({
pending: false,
result: { txid: 'txid' },
});
});
it('propagates errors from parsePsbt', async () => {
const error = new Error('parsePsbt');
jest.mocked(parsePsbt).mockImplementationOnce(() => {
throw error;
});
await expect(
handler.route({
...mockRequest,
request: { ...mockRequest.request, params: { psbt: 'invalidPsbt' } },
}),
).rejects.toThrow(error);
expect(mockAccountsUseCases.broadcastPsbt).not.toHaveBeenCalled();
});
it('propagates errors from fillPsbt', async () => {
const error = new Error();
mockAccountsUseCases.broadcastPsbt.mockRejectedValue(error);
await expect(handler.route(mockRequest)).rejects.toThrow(error);
expect(mockAccountsUseCases.broadcastPsbt).toHaveBeenCalled();
});
});
describe('sendTransfer', () => {
const recipients = [
{
address: 'bcrt1qstku2y3pfh9av50lxj55arm8r5gj8tf2yv5nxz',
amount: '1000',
},
];
const mockRequest = mock<KeyringRequest>({
origin,
request: {
method: AccountCapability.SendTransfer,
params: {
recipients,
feeRate: 3,
},
},
account: 'account-id',
});
it('executes sendTransferq', async () => {
const mockTxid = mock<Txid>({
toString: jest.fn().mockReturnValue('txid'),
});
mockAccountsUseCases.sendTransfer.mockResolvedValue(mockTxid);
const result = await handler.route(mockRequest);
expect(assert).toHaveBeenCalledWith(
mockRequest.request.params,
SendTransferRequest,
);
expect(mockAccountsUseCases.sendTransfer).toHaveBeenCalledWith(
'account-id',
recipients,
origin,
3,
);
expect(result).toStrictEqual({
pending: false,
result: { txid: 'txid' },
});
});
it('propagates errors from sendTransfer', async () => {
const error = new Error();
mockAccountsUseCases.sendTransfer.mockRejectedValue(error);
await expect(handler.route(mockRequest)).rejects.toThrow(error);
expect(mockAccountsUseCases.sendTransfer).toHaveBeenCalled();
});
});
describe('getUtxo', () => {
const mockLocalOutput = mock<LocalOutput>();
const mockAccount = mock<BitcoinAccount>({
getUtxo: () => mockLocalOutput,
network: 'bitcoin',
});
const mockRequest = mock<KeyringRequest>({
origin,
request: {
method: AccountCapability.GetUtxo,
params: {
outpoint: 'mytxid:0',
},
},
account: 'account-id',
});
it('executes getUtxo', async () => {
const expectedUtxo = {
derivationIndex: 0,
outpoint: 'mytxid:0',
value: '1000',
scriptPubkey: 'scriptPubkey',
scriptPubkeyHex: 'scriptPubkeyHex',
};
mockAccountsUseCases.get.mockResolvedValue(mockAccount);
jest.mocked(mapToUtxo).mockReturnValue(expectedUtxo);
const result = await handler.route(mockRequest);
expect(assert).toHaveBeenCalledWith(
mockRequest.request.params,
GetUtxoRequest,
);
expect(mockAccountsUseCases.get).toHaveBeenCalledWith('account-id');
expect(result).toStrictEqual({
pending: false,
result: expectedUtxo,
});
});
});
describe('listUtxos', () => {
const mockLocalOutput = mock<LocalOutput>();
const mockAccount = mock<BitcoinAccount>({
listUnspent: () => [mockLocalOutput, mockLocalOutput],
network: 'bitcoin',
});
const mockRequest = mock<KeyringRequest>({
origin,
request: {
method: AccountCapability.ListUtxos,
},
account: 'account-id',
});
it('executes listUtxos', async () => {
const mockUtxo = mock<Utxo>({
derivationIndex: 0,
outpoint: 'mytxid:0',
value: '1000',
scriptPubkey: 'scriptPubkey',
scriptPubkeyHex: 'scriptPubkeyHex',
});

Fix in Cursor Fix in Web


request: {
method: AccountCapability.SendTransfer,
params: {
account: { address: account.address },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Integration test incompatible with new single-recipient validation

The integration test "sends funds successfully" for sendTransfer passes two recipients in the recipients array and expects a successful response with a txid. However, the new implementation in AccountUseCases.sendTransfer now validates that there is exactly one recipient, throwing a ValidationError when recipients.length > 1. The test will fail because the implementation rejects the request before processing it.

Additional Locations (1)

Fix in Cursor Fix in Web

@baptiste-marchand baptiste-marchand force-pushed the feat/bitcoin-address-resolution branch from 105bfe9 to 62f251f Compare December 9, 2025 16:08
throw new ValidationError('There should be exactly one recipient', {
recipients,
});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: SendTransfer rejects multiple recipients but test expects success

The new sendTransfer implementation throws a ValidationError when recipients.length > 1, but the integration test at keyring-request.test.ts line 638-646 passes two recipients and expects the operation to succeed. This change breaks backward compatibility - the previous implementation supported multiple recipients by iterating over all of them in a loop. The test will fail because the code now rejects any request with more than one recipient.

Additional Locations (1)

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants