Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions packages/shield-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Add two new controller state metadata properties: `includeInStateLogs` and `usedInUi` ([#6504](https://github.com/MetaMask/core/pull/6504))
- Add signature coverage checking ([#6501](https://github.com/MetaMask/core/pull/6501))
- Add transaction and signature logging ([#6633](https://github.com/MetaMask/core/pull/6633))

### Changed

Expand Down
182 changes: 179 additions & 3 deletions packages/shield-controller/src/ShieldController.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
import { deriveStateFromMetadata } from '@metamask/base-controller';
import type { SignatureControllerState } from '@metamask/signature-controller';
import type { TransactionControllerState } from '@metamask/transaction-controller';
import type { SignatureRequest } from '@metamask/signature-controller';
import {
SignatureRequestStatus,
type SignatureControllerState,
} from '@metamask/signature-controller';
import type { TransactionMeta } from '@metamask/transaction-controller';
import {
TransactionStatus,
type TransactionControllerState,
} from '@metamask/transaction-controller';

import { ShieldController } from './ShieldController';
import { createMockBackend } from '../tests/mocks/backend';
import { createMockBackend, MOCK_COVERAGE_ID } from '../tests/mocks/backend';
import { createMockMessenger } from '../tests/mocks/messenger';
import {
generateMockSignatureRequest,
Expand Down Expand Up @@ -218,6 +226,174 @@ describe('ShieldController', () => {
);
});

describe('logSignature', () => {
/**
* Run a test that logs a signature.
*
* @param components - An object containing the messenger and base messenger.
* @param options - An object containing optional parameters.
* @param options.updateSignatureRequest - A function that updates the signature request.
*/
async function runTest(
components: ReturnType<typeof setup>,
options?: {
updateSignatureRequest?: (signatureRequest: SignatureRequest) => void;
},
) {
const { messenger, baseMessenger } = components;

// Create a promise that resolves when the state changes
const stateUpdated = new Promise((resolve) =>
messenger.subscribe('ShieldController:stateChange', resolve),
);

// Publish a signature request
const signatureRequest = generateMockSignatureRequest();
baseMessenger.publish(
'SignatureController:stateChange',
{
signatureRequests: { [signatureRequest.id]: signatureRequest },
} as SignatureControllerState,
undefined as never,
);

// Wait for state to be updated
await stateUpdated;

// Update signature request
const updatedSignatureRequest = { ...signatureRequest };
updatedSignatureRequest.status = SignatureRequestStatus.Signed;
updatedSignatureRequest.rawSig = '0x00';
options?.updateSignatureRequest?.(updatedSignatureRequest);
baseMessenger.publish(
'SignatureController:stateChange',
{
signatureRequests: { [signatureRequest.id]: updatedSignatureRequest },
} as SignatureControllerState,
undefined as never,
);
}

it('logs a signature', async () => {
const components = setup();

await runTest(components);

// Check that backend was called
expect(components.backend.logSignature).toHaveBeenCalledWith({
coverageId: MOCK_COVERAGE_ID,
signature: '0x00',
status: 'shown',
});
});

it('does not log when coverageId is missing', async () => {
const components = setup();

components.backend.checkSignatureCoverage.mockResolvedValue({
coverageId: undefined,
status: 'unknown',
});

await runTest(components);

// Check that backend was not called
expect(components.backend.logSignature).not.toHaveBeenCalled();
});

it('does not log when signature is missing', async () => {
const components = setup();

await runTest(components, {
updateSignatureRequest: (signatureRequest) => {
signatureRequest.rawSig = undefined;
},
});

// Check that backend was not called
expect(components.backend.logSignature).not.toHaveBeenCalled();
});
});

describe('logTransaction', () => {
/**
* Runs a test that logs a transaction.
*
* @param components - An object containing the messenger and base messenger.
* @param options - Options for the test.
* @param options.updateTransaction - A function that updates the transaction.
*/
async function runTest(
components: ReturnType<typeof setup>,
options?: { updateTransaction: (txMeta: TransactionMeta) => void },
) {
const { messenger, baseMessenger } = components;
// Create a promise that resolves when the state changes
const stateUpdated = new Promise((resolve) =>
messenger.subscribe('ShieldController:stateChange', resolve),
);

// Publish a transaction
const txMeta = generateMockTxMeta();
baseMessenger.publish(
'TransactionController:stateChange',
{ transactions: [txMeta] } as TransactionControllerState,
undefined as never,
);

// Wait for state to be updated
await stateUpdated;

// Update transaction
const updatedTxMeta = { ...txMeta };
updatedTxMeta.status = TransactionStatus.submitted;
updatedTxMeta.hash = '0x00';
options?.updateTransaction(updatedTxMeta);
baseMessenger.publish(
'TransactionController:stateChange',
{ transactions: [updatedTxMeta] } as TransactionControllerState,
undefined as never,
);
}

it('logs a transaction', async () => {
const components = setup();
await runTest(components);

// Check that backend was called
expect(components.backend.logTransaction).toHaveBeenCalledWith({
coverageId: MOCK_COVERAGE_ID,
status: 'shown',
transactionHash: '0x00',
});
});

it('does not log when coverageId is missing', async () => {
const components = setup();

components.backend.checkCoverage.mockResolvedValue({
coverageId: undefined,
status: 'unknown',
});

await runTest(components);

// Check that backend was not called
expect(components.backend.logTransaction).not.toHaveBeenCalled();
});

it('does not log when hash is missing', async () => {
const components = setup();

await runTest(components, {
updateTransaction: (txMeta) => delete txMeta.hash,
});

// Check that backend was not called
expect(components.backend.logTransaction).not.toHaveBeenCalled();
});
});

describe('metadata', () => {
it('includes expected state in debug snapshots', () => {
const { controller } = setup();
Expand Down
75 changes: 72 additions & 3 deletions packages/shield-controller/src/ShieldController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import type {
RestrictedMessenger,
} from '@metamask/base-controller';
import {
SignatureRequestStatus,
SignatureRequestType,
type SignatureRequest,
type SignatureStateChange,
} from '@metamask/signature-controller';
import type {
TransactionControllerStateChangeEvent,
TransactionMeta,
import {
TransactionStatus,
type TransactionControllerStateChangeEvent,
type TransactionMeta,
} from '@metamask/transaction-controller';

import { controllerName } from './constants';
Expand Down Expand Up @@ -230,6 +232,17 @@ export class ShieldController extends BaseController<
(error) => log('Error checking coverage:', error),
);
}

// Log signature once the signature request has been fulfilled.
if (
signatureRequest.status === SignatureRequestStatus.Signed &&
signatureRequest.status !== previousSignatureRequest?.status
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanna verify, this might change when we wanna do log on the confirmation reject, right?

) {
this.#logSignature(signatureRequest).catch(
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the log logic will probably change after the discussion today.
But as of now, logging should be done only when the coverage result is available?

// istanbul ignore next
(error) => log('Error logging signature:', error),
);
}
Copy link

Choose a reason for hiding this comment

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

Bug: Async Logging Fails Due to Race Condition

Race condition between coverage checking and signature logging. When a new signature request is created and immediately signed, both checkSignatureCoverage and #logSignature are called asynchronously without waiting for each other. The #logSignature method may execute before checkSignatureCoverage completes, causing it to fail with "Coverage ID not found" since the coverage result hasn't been stored yet. While the error is caught and logged, this could result in signatures not being logged to the backend.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice find. I'm aware of this. The reasoning here is: If the state is not updated yet, it means we don't have a coverage result yet. In this case it's OK to not log as far as I understand.

Copy link

Choose a reason for hiding this comment

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

Bug: Premature Logging for New Items

The signature and transaction logging conditions trigger prematurely for new items initialized with a Signed or submitted status. This happens because the previousRequest?.status check evaluates to undefined, causing the condition to be met without an actual status transition. This can lead to errors, as coverage results may not be available yet.

Additional Locations (1)

Fix in Cursor Fix in Web

}
}

Expand All @@ -256,6 +269,17 @@ export class ShieldController extends BaseController<
(error) => log('Error checking coverage:', error),
);
}

// Log transaction once it has been submitted.
if (
transaction.status === TransactionStatus.submitted &&
transaction.status !== previousTransaction?.status
) {
this.#logTransaction(transaction).catch(
// istanbul ignore next
(error) => log('Error logging transaction:', error),
);
}
}
}

Expand Down Expand Up @@ -346,4 +370,49 @@ export class ShieldController extends BaseController<
}
});
}

async #logSignature(signatureRequest: SignatureRequest) {
const coverageId = this.#getLatestCoverageId(signatureRequest.id);
if (!coverageId) {
throw new Error('Coverage ID not found');
}

const sig = signatureRequest.rawSig;
if (!sig) {
throw new Error('Signature not found');
}

await this.#backend.logSignature({
coverageId,
signature: sig,
// Status is 'shown' because the coverageId can only be retrieved after
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the log logic will probably change after the discussion today.
But as of now, logging should be done only when the coverage result is available?

// the result is in the state. If the result is in the state, we assume
// that it has been shown.
status: 'shown',
});
}

async #logTransaction(txMeta: TransactionMeta) {
const coverageId = this.#getLatestCoverageId(txMeta.id);
if (!coverageId) {
throw new Error('Coverage ID not found');
}

const txHash = txMeta.hash;
if (!txHash) {
throw new Error('Transaction hash not found');
}
await this.#backend.logTransaction({
coverageId,
transactionHash: txHash,
// Status is 'shown' because the coverageId can only be retrieved after
Copy link
Contributor

Choose a reason for hiding this comment

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

// the result is in the state. If the result is in the state, we assume
// that it has been shown.
status: 'shown',
});
}

#getLatestCoverageId(itemId: string) {
return this.state.coverageResults[itemId]?.results[0]?.coverageId;
}
Copy link

Choose a reason for hiding this comment

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

Bug: Logging Errors on Missing Data

The #logSignature and #logTransaction methods throw errors when coverageId or the signature/transaction hash is missing. The expectation is to silently skip logging in these cases, but the current implementation logs an error instead.

Fix in Cursor Fix in Web

}
Loading
Loading