-
-
Notifications
You must be signed in to change notification settings - Fork 6
feat: enable account address resolution to support dApps connectivity #564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| }); | ||
|
|
||
| export const SignMessageRequest = object({ | ||
| account: WalletAccountStruct, |
There was a problem hiding this comment.
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.
a041ba7 to
e0ea92e
Compare
There was a problem hiding this 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
snap-bitcoin-wallet/packages/snap/src/handlers/KeyringRequestHandler.test.ts
Lines 64 to 431 in 18900e3
| 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', | |
| }); |
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
resolveAccountAddressmethod inKeyringHandlerandKeyringRequestHandlerto determine the account address for signing requests.Note
Implement account address resolution and require
account.addressin BTC requests to enable routing of non-EVM signing, with validations, tests, and manifest update.resolveAccountAddress(scope, request)and routeskeyring_resolveAccountAddress; validates withNetworkStructandBtcWalletRequestStructand returns CAIP-10 address.WalletAccountStructandBtcWalletRequestStruct(union of BTC methods); requiresparams.account.addressacrossSignPsbt,FillPsbt,ComputeFee,BroadcastPsbt,SendTransfer,GetUtxo,SignMessage; implementsresolveAccountAddress(...)to select scoped account and build CAIP-10.NetworkStructfor scope validation.params.account: { address }for all BTC requests and add error when missing; adjust PSBT/fee/transfer/broadcast/sign flows accordingly.resolveAccountAddressin both handlers, including success, unsupported method, not found, invalid request, and no-scope cases.source.shasum.Written by Cursor Bugbot for commit 2aeb47d. This will update automatically on new commits. Configure here.