[Feature] Productionize Record Scanner Interface #1182
Conversation
…ation endpoint + add tests of the registration flow
…ndpoint, and add ability to specify an account to the RecordScanner implementation
iamalwaysuncomfortable
requested review from
Roee-87,
marshacb and
niklaslong
iamalwaysuncomfortable
changed the title
marshacb
left a comment
marshacb
left a comment
There was a problem hiding this comment.
Looking great! Left a few questions/minor suggestions on naming conventions and type consistency. Let me know your thoughts :)
|
|
||
| constructor(message: string, uuid?: string, filter?: OwnedFilter) { | ||
| super(message); | ||
| this.name = "InvalidResponseError"; |
There was a problem hiding this comment.
Should we change the name here to "UUIDError" for consistency?
| } | ||
| return { ok: true, data }; | ||
| } catch (err) { | ||
| if (err instanceof RecordScannerRequestError) { |
There was a problem hiding this comment.
Nit: I see we use this error pattern a number of times. We could add a method to RecordScanner that could be reused for these cases.
e.g.
private handleRequestError(err: unknown): { ok: false; status: number; error: RecordScannerErrorBody } {
if (err instanceof RecordScannerRequestError) {
return {
ok: false,
status: err.status,
error: { message: err.message, status: err.status },
};
}
throw err;
}
So then we could just have one line each such as:
} catch (err) {
return this.handleRequestError(err);
}
| ); | ||
|
|
||
| if (!safeResult.ok) { | ||
| console.log(safeResult); |
There was a problem hiding this comment.
Are we keeping this log for debugging or should we remove it before merging?
| * @returns {Promise<TagsResult>} Map of Aleo Record tags and whether they appeared in any inputs on chain. If a boolean corresponding to the tag has a true value, that Record is considered spent by the Aleo Network. | ||
| */ | ||
| async checkTags(tags: string[]): Promise<Record<string, boolean>> { | ||
| async tags(tags: string[]): Promise<TagsResult> { |
There was a problem hiding this comment.
I noticed the safe vs throwing method naming isn't consistent (e.g., encrypted() vs encryptedRecords(), owned() vs findRecords()). Is the current naming intentional or should we align on a pattern? I was wondering if using suffixes could be a good option (e.g. registerSafe(), findRecordsOrThrow(), etc) but we could also choose to document them as is.
| this.uuid = uuidOrViewKey instanceof ViewKey ? this.computeUUID(uuidOrViewKey) : uuidOrViewKey; | ||
| private async refreshJwt(apiKey: string, consumerId: string): Promise<RecordScannerJWTData> { | ||
| const response = await post( | ||
| `https://api.provable.com/jwts/${consumerId}`, |
There was a problem hiding this comment.
Is the hardcoded https://api.provable.com/jwts/ intentional? Safe to assume JWT refresh always will go to the production auth service or do we need to possibly account for different environments?
|
|
||
| export interface EncryptedRecordsFailure { | ||
| ok: false; | ||
| status: number; |
There was a problem hiding this comment.
The new result types seem to use status: number but the SDK convention (per provingResponse.ts and others) is status: bigint | number. Should we align with the existing pattern for consistency or is this difference intentional?
2 participants
Motivation
The record scanner is currently adding encrypted registration flows to ensure sensitive material is protected before it gets to the scanner enclave, this PR adds encrypted registration flows to the RecordScanner object.
Test Plan
This PR adds tests to the Record Scanner to ensure these encrypted flows work.