Skip to content

feat!: add KeyringRequest.origin #273

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat!: add KeyringRequest.origin #273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8b2c519
feat: add KeyringRequest.origin
ccharly Apr 2, 2025
d02787d
feat!: add new RestrictedKeyringClient
ccharly Apr 4, 2025
815d9ca
refactor: rename Restricted -> Public
ccharly Apr 4, 2025
8806bde
refactor: make KeyringPublicClient a true class
ccharly Apr 4, 2025
754a75f
test: re-use tests for the new KeyringPublicClient
ccharly Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/keyring-api/src/api/request.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ describe('KeyringRequest', () => {
id: '47d782ac-15c8-4c81-8bfe-759ae1be4a3e',
scope: 'eip155:1',
account: 'd6311e3c-a4ec-43fa-b341-592ffefd9797',
origin: 'metamask',
request: {
method: 'eth_personalSign',
params: {
Expand All @@ -24,6 +25,7 @@ describe('KeyringRequest', () => {
id: '47d782ac-15c8-4c81-8bfe-759ae1be4a3e',
scope: 'eip155:1',
account: 'd6311e3c-a4ec-43fa-b341-592ffefd9797',
origin: 'metamask',
request: {
method: 'eth_somethingElseWithNoParameters',
},
Expand All @@ -36,6 +38,7 @@ describe('KeyringRequest', () => {
request: {
scope: 'eip155:1',
account: 'd6311e3c-a4ec-43fa-b341-592ffefd9797',
origin: 'metamask',
request: {
method: 'eth_personalSign',
params: {
Expand All @@ -50,6 +53,7 @@ describe('KeyringRequest', () => {
request: {
id: '47d782ac-15c8-4c81-8bfe-759ae1be4a3e',
account: 'd6311e3c-a4ec-43fa-b341-592ffefd9797',
origin: 'metamask',
request: {
method: 'eth_personalSign',
params: {
Expand All @@ -64,6 +68,22 @@ describe('KeyringRequest', () => {
request: {
id: '47d782ac-15c8-4c81-8bfe-759ae1be4a3e',
scope: 'eip155:1',
origin: 'metamask',
request: {
method: 'eth_personalSign',
params: {
data: '0x00...',
},
},
},
expected: false,
},
// Missing origin
{
request: {
id: '47d782ac-15c8-4c81-8bfe-759ae1be4a3e',
scope: 'eip155:1',
account: 'd6311e3c-a4ec-43fa-b341-592ffefd9797',
request: {
method: 'eth_personalSign',
params: {
Expand All @@ -79,6 +99,7 @@ describe('KeyringRequest', () => {
id: '47d782ac-15c8-4c81-8bfe-759ae1be4a3e',
scope: 'eip155:1',
account: 'd6311e3c-a4ec-43fa-b341-592ffefd9797',
origin: 'metamask',
},
expected: false,
},
Expand Down
5 changes: 5 additions & 0 deletions packages/keyring-api/src/api/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ export const KeyringRequestStruct = object({
*/
account: UuidStruct,

/**
* Origin of the sender.
*/
origin: string(),

/**
* Inner request sent by the client application.
*/
Expand Down
50 changes: 50 additions & 0 deletions packages/keyring-snap-bridge/src/SnapKeyring.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1557,6 +1557,7 @@ describe('SnapKeyring', () => {
params: {
id: expect.any(String),
scope: expectedScope,
origin: 'metamask',
account: ethEoaAccount1.id,
request: {
method: 'eth_signTransaction',
Expand Down Expand Up @@ -1638,6 +1639,7 @@ describe('SnapKeyring', () => {
params: {
id: expect.any(String),
scope: expectedScope,
origin: 'metamask',
account: ethEoaAccount1.id,
request: {
method: 'eth_signTypedData_v1',
Expand Down Expand Up @@ -1671,6 +1673,7 @@ describe('SnapKeyring', () => {
params: {
id: expect.any(String),
scope: expectedScope,
origin: 'metamask',
account: ethEoaAccount1.id,
request: {
method: 'eth_signTypedData_v4',
Expand Down Expand Up @@ -1704,6 +1707,7 @@ describe('SnapKeyring', () => {
params: {
id: expect.any(String),
scope: expectedScope,
origin: 'metamask',
account: ethEoaAccount1.id,
request: {
method: 'eth_signTypedData_v1',
Expand Down Expand Up @@ -1751,6 +1755,7 @@ describe('SnapKeyring', () => {
// Without chainId alongside the typed message, we cannot
// compute the scope for this request!
scope: '', // Default value for `signTypedTransaction`
origin: 'metamask',
account: ethEoaAccount1.id,
request: {
method: 'eth_signTypedData_v4',
Expand Down Expand Up @@ -1815,6 +1820,7 @@ describe('SnapKeyring', () => {
KnownCaipNamespace.Eip155,
executionContext.chainId,
),
origin: 'metamask',
account: ethErc4337Account.id,
request: {
method: 'eth_prepareUserOperation',
Expand Down Expand Up @@ -1871,6 +1877,7 @@ describe('SnapKeyring', () => {
KnownCaipNamespace.Eip155,
executionContext.chainId,
),
origin: 'metamask',
account: ethErc4337Account.id,
request: {
method: 'eth_patchUserOperation',
Expand Down Expand Up @@ -1923,6 +1930,7 @@ describe('SnapKeyring', () => {
KnownCaipNamespace.Eip155,
executionContext.chainId,
),
origin: 'metamask',
account: ethErc4337Account.id,
request: {
method: 'eth_signUserOperation',
Expand Down Expand Up @@ -1983,6 +1991,7 @@ describe('SnapKeyring', () => {
params: {
id: expect.any(String),
scope: expect.any(String),
origin: 'metamask',
account: ethEoaAccount1.id,
request: {
method: 'eth_sign',
Expand Down Expand Up @@ -2368,12 +2377,15 @@ describe('SnapKeyring', () => {
};

it('submits a request', async () => {
const origin = 'test';

mockMessenger.handleRequest.mockResolvedValue({
pending: false,
result: null,
});

await keyring.submitRequest({
origin,
account: account.id,
method,
params,
Expand All @@ -2390,6 +2402,7 @@ describe('SnapKeyring', () => {
params: {
id: expect.any(String),
scope,
origin,
account: account.id,
request: {
method,
Expand All @@ -2408,6 +2421,7 @@ describe('SnapKeyring', () => {

await expect(
keyring.submitRequest({
origin: 'test',
account: account.id,
method,
params,
Expand All @@ -2421,6 +2435,7 @@ describe('SnapKeyring', () => {

await expect(
keyring.submitRequest({
origin: 'test',
account: unknownAccountId,
method,
params,
Expand All @@ -2436,6 +2451,39 @@ describe('SnapKeyring', () => {

await expect(
keyring.submitRequest({
origin: 'test',
account: account.id,
method: unknownAccountMethod,
params,
scope,
}),
).rejects.toThrow(
`Method '${unknownAccountMethod}' not supported for account ${account.address}`,
);
});

it('throws an error if origin is not being passed', async () => {
const unknownAccountMethod = EthMethod.PrepareUserOperation; // Not available for EOAs.

await expect(
keyring.submitRequest({
// No origin
account: account.id,
method: unknownAccountMethod,
params,
scope,
} as unknown as Parameters<SnapKeyring['submitRequest']>[0]),
).rejects.toThrow(
`Method '${unknownAccountMethod}' not supported for account ${account.address}`,
);
});

it('throws an error if origin is empty', async () => {
const unknownAccountMethod = EthMethod.PrepareUserOperation; // Not available for EOAs.

await expect(
keyring.submitRequest({
origin: '',
account: account.id,
method: unknownAccountMethod,
params,
Expand Down Expand Up @@ -2495,6 +2543,7 @@ describe('SnapKeyring', () => {
KnownCaipNamespace.Eip155,
executionContext.chainId,
),
origin: 'metamask',
account: ethErc4337Account.id,
request: {
method: 'eth_prepareUserOperation',
Expand Down Expand Up @@ -2565,6 +2614,7 @@ describe('SnapKeyring', () => {
KnownCaipNamespace.Eip155,
executionContext.chainId,
),
origin: 'metamask',
account: ethErc4337Account.id,
request: {
method: 'eth_patchUserOperation',
Expand Down
19 changes: 19 additions & 0 deletions packages/keyring-snap-bridge/src/SnapKeyring.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,18 +831,21 @@ export class SnapKeyring extends EventEmitter {
* This request cannot be an asynchronous keyring request.
*
* @param opts - Request options.
* @param opts.origin - Send origin.
* @param opts.account - Account ID.
* @param opts.method - Method to call.
* @param opts.params - Method parameters.
* @param opts.scope - Selected chain ID (CAIP-2).
* @returns Promise that resolves to the result of the method call.
*/
async submitRequest({
origin,
account: accountId,
method,
params,
scope,
}: {
origin: string;
// NOTE: We use `account` here rather than `id` to avoid ambiguity with a "request ID".
// We already use this same field name for `KeyringAccount`s.
account: string;
Expand All @@ -853,6 +856,7 @@ export class SnapKeyring extends EventEmitter {
const { account, snapId } = this.#getAccount(accountId);

return await this.#submitSnapRequest({
origin,
snapId,
account,
method: method as AccountMethod,
Expand All @@ -868,6 +872,7 @@ export class SnapKeyring extends EventEmitter {
* Submit a request to a Snap from an account address.
*
* @param opts - Request options.
* @param opts.origin - Sender origin.
* @param opts.address - Account address.
* @param opts.method - Method to call.
* @param opts.params - Method parameters.
Expand All @@ -876,12 +881,14 @@ export class SnapKeyring extends EventEmitter {
* @returns Promise that resolves to the result of the method call.
*/
async #submitRequest<Response extends Json>({
origin,
address,
method,
params,
scope = '',
noPending = false,
}: {
origin: string;
address: string;
method: string;
params?: Json[] | Record<string, Json>;
Expand All @@ -891,6 +898,7 @@ export class SnapKeyring extends EventEmitter {
const { account, snapId } = this.#resolveAddress(address);

return await this.#submitSnapRequest<Response>({
origin,
snapId,
account,
method: method as AccountMethod,
Expand All @@ -904,6 +912,7 @@ export class SnapKeyring extends EventEmitter {
* Submits a request to a Snap.
*
* @param options - The options for the Snap request.
* @param options.origin - The sender origin.
* @param options.snapId - The Snap ID to submit the request to.
* @param options.account - The account to use for the request.
* @param options.method - The Ethereum method to call.
Expand All @@ -914,13 +923,15 @@ export class SnapKeyring extends EventEmitter {
* @throws An error if the Snap fails to respond or if there's an issue with the request submission.
*/
async #submitSnapRequest<Response extends Json>({
origin,
snapId,
account,
method,
params,
scope,
noPending,
}: {
origin: string;
snapId: SnapId;
account: KeyringAccount;
method: AccountMethod;
Expand All @@ -947,6 +958,7 @@ export class SnapKeyring extends EventEmitter {
try {
const request = {
id: requestId,
origin,
scope,
account: account.id,
request: {
Expand Down Expand Up @@ -1123,6 +1135,7 @@ export class SnapKeyring extends EventEmitter {
});

const signedTx = await this.#submitRequest({
origin: 'metamask',
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: we are setting the origin to metamask here regardless of the real origin.

Do you have an idea how we are going to send the real origin? Also, will this new field break the existing Snaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 🤔 I wrongly assumed we were using those methods when interacting through the MetaMask UI, but this will also be called externally with call like eth_sign*, in which case, the true origin would not be metamask.

And yes, this new field might also break existing Snaps. I don't know else I could trick that. My only idea right now would be to "version the API", and have a runtime check on the version, something like:

getVersion() {
  const { version } = await this.#snapClient
    .withSnapId(snapId)
    .getVersion();
    
  if (!version) {
    return ...; // First version when we introduced the new `getVersion`?
  }
}

But even know, we cannot check which methods are being implemented on a Snap, which would result in a runtime error for this ⬆️.

And adding a version won't solve everything, cause we would have type-issue here to (here we need to pass origin, or we need to have another variant type for submitRequest with an optional origin, that won't scale well IMO..)

address,
method: EthMethod.SignTransaction,
params: [tx],
Expand Down Expand Up @@ -1177,6 +1190,7 @@ export class SnapKeyring extends EventEmitter {

return strictMask(
await this.#submitRequest({
origin: 'metamask',
address,
method,
params: toJson<Json[]>([address, data]),
Expand All @@ -1200,6 +1214,7 @@ export class SnapKeyring extends EventEmitter {
async signMessage(address: string, hash: any): Promise<string> {
return strictMask(
await this.#submitRequest({
origin: 'metamask',
address,
method: EthMethod.Sign,
params: toJson<Json[]>([address, hash]),
Expand All @@ -1221,6 +1236,7 @@ export class SnapKeyring extends EventEmitter {
async signPersonalMessage(address: string, data: any): Promise<string> {
return strictMask(
await this.#submitRequest({
origin: 'metamask',
address,
method: EthMethod.PersonalSign,
params: toJson<Json[]>([data, address]),
Expand All @@ -1244,6 +1260,7 @@ export class SnapKeyring extends EventEmitter {
): Promise<EthBaseUserOperation> {
return strictMask(
await this.#submitRequest({
origin: 'metamask',
address,
method: EthMethod.PrepareUserOperation,
params: toJson<Json[]>(transactions),
Expand Down Expand Up @@ -1271,6 +1288,7 @@ export class SnapKeyring extends EventEmitter {
): Promise<EthUserOperationPatch> {
return strictMask(
await this.#submitRequest({
origin: 'metamask',
address,
method: EthMethod.PatchUserOperation,
params: toJson<Json[]>([userOp]),
Expand All @@ -1297,6 +1315,7 @@ export class SnapKeyring extends EventEmitter {
): Promise<string> {
return strictMask(
await this.#submitRequest({
origin: 'metamask',
address,
method: EthMethod.SignUserOperation,
params: toJson<Json[]>([userOp]),
Expand Down
Loading
Loading