-
-
Notifications
You must be signed in to change notification settings - Fork 246
feat: shield-controller: check signature coverage #6501
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
Changes from all commits
bdc6576
93c68e6
8da1a15
5cef2e6
a3a2197
571817b
a8b3145
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import type { SignatureRequest } from '@metamask/signature-controller'; | ||
import type { TransactionMeta } from '@metamask/transaction-controller'; | ||
|
||
import type { CoverageResult, CoverageStatus, ShieldBackend } from './types'; | ||
|
@@ -16,6 +17,14 @@ export type InitCoverageCheckRequest = { | |
origin?: string; | ||
}; | ||
|
||
export type InitSignatureCoverageCheckRequest = { | ||
chainId: string; | ||
data: string; | ||
from: string; | ||
method: string; | ||
origin?: string; | ||
}; | ||
|
||
export type InitCoverageCheckResponse = { | ||
coverageId: string; | ||
}; | ||
|
@@ -59,9 +68,7 @@ export class ShieldRemoteBackend implements ShieldBackend { | |
this.#fetch = fetchFn; | ||
} | ||
|
||
checkCoverage: (txMeta: TransactionMeta) => Promise<CoverageResult> = async ( | ||
txMeta, | ||
) => { | ||
async checkCoverage(txMeta: TransactionMeta): Promise<CoverageResult> { | ||
const reqBody: InitCoverageCheckRequest = { | ||
txParams: [ | ||
{ | ||
|
@@ -76,22 +83,46 @@ export class ShieldRemoteBackend implements ShieldBackend { | |
origin: txMeta.origin, | ||
}; | ||
|
||
const { coverageId } = await this.#initCoverageCheck(reqBody); | ||
const { coverageId } = await this.#initCoverageCheck( | ||
'v1/transaction/coverage/init', | ||
reqBody, | ||
); | ||
|
||
return this.#getCoverageResult(coverageId); | ||
}; | ||
} | ||
|
||
async checkSignatureCoverage( | ||
signatureRequest: SignatureRequest, | ||
): Promise<CoverageResult> { | ||
if (typeof signatureRequest.messageParams.data !== 'string') { | ||
throw new Error('Signature data must be a string'); | ||
} | ||
|
||
const reqBody: InitSignatureCoverageCheckRequest = { | ||
chainId: signatureRequest.chainId, | ||
data: signatureRequest.messageParams.data, | ||
from: signatureRequest.messageParams.from, | ||
method: signatureRequest.type, | ||
origin: signatureRequest.messageParams.origin, | ||
}; | ||
|
||
const { coverageId } = await this.#initCoverageCheck( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please note that we can't always except Please check the swagger, as it has updated recently. (please select ![]() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The sample cases of 200 responses without
In such cases, we don't proceed to process the rulesets, we straight away return the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This case also applies to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we throw an error in that case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we please change the API backend. I do see several issues with this change. The expectation so far is that if we have a coverage result, either for signatures, or for transactions, it always has the same format. If we don't have that, a lot of complexity is added to the whole system. (Currently we can just treat the results within the same data structure. Then we might need separate data structure. Also, missing the |
||
'v1/signature/coverage/init', | ||
reqBody, | ||
); | ||
|
||
return this.#getCoverageResult(coverageId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Coverage ID Missing in API ResponseThe code assumes Additional Locations (1) |
||
} | ||
|
||
async #initCoverageCheck( | ||
reqBody: InitCoverageCheckRequest, | ||
path: string, | ||
reqBody: unknown, | ||
): Promise<InitCoverageCheckResponse> { | ||
const res = await this.#fetch( | ||
`${this.#baseUrl}/v1/transaction/coverage/init`, | ||
{ | ||
method: 'POST', | ||
headers: await this.#createHeaders(), | ||
body: JSON.stringify(reqBody), | ||
}, | ||
); | ||
const res = await this.#fetch(`${this.#baseUrl}/${path}`, { | ||
method: 'POST', | ||
headers: await this.#createHeaders(), | ||
body: JSON.stringify(reqBody), | ||
}); | ||
if (res.status !== 200) { | ||
throw new Error(`Failed to init coverage check: ${res.status}`); | ||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.