-
Notifications
You must be signed in to change notification settings - Fork 6
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
Feat v230223 v230310 special provider for photos taken from capture app #2667
Feat v230223 v230310 special provider for photos taken from capture app #2667
Conversation
…ApiSignatureProvider
…iSignatureProvider if any
For methods: - run - generateSignature - signMessage
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.
@sultanmyrza can you please share with me if photo upload is involved in this PR? According to the source code, I think it is mainly for the photos taken from capture app. However, it mentioned "uploadToCapture" so that I want to confirm.
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.
@tammyyang, In this PR I changed uploadToCapture parameters.
Now I pass extra parameter curCaptureCameraSource that can be PHOTO (when picked from gallery) or CAMERA (when taken photo from camera)
Then I use that parameter when making decision.
If curCaptureCameraSource = PHOTO => UploaderWebCryptoApiSignatureProvider
if curCaptureCameraSource = CAMERA => CaptureAppWebCryptoApiSignatureProvider
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.
Photo upload is involved in way that we pass extra param (PHOTO or CAMERA)
@@ -387,7 +402,7 @@ export interface IndexedProofView extends Tuple { | |||
*/ | |||
export interface SignedMessage { | |||
spec_version: string; | |||
recorder: string; | |||
recorder: RecorderType; | |||
created_at: number; | |||
location_latitude?: number; | |||
location_longitude?: number; |
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.
ChatGPT Plus code review with improvement suggestions:
- Add explicit return types to functions. For example,
generateSignedMessage
should have a return typePromise<SignedMessage>
. - Consider adding a documentation block for the
generateSignedMessage
function, explaining its purpose and describing therecorder
parameter. - Rename the
recorder
parameter torecorderType
in thegenerateSignedMessage
function to better indicate that it is an enum value and not an instance of a recorder. - Define an interface for the
SignedMessage
object to enforce a specific structure and improve code maintainability. - In the
RecorderType
enum, the value of theDEFAULT
key is'Capture'
. Consider renaming this key toCAPTURE
for consistency with the other keys.
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.
- Done
- Done
- N/A I think
recorder
is better - Done
- N/A I think
Capture
is better because others are camelCased (CaptureAppWebCryptoApiSignatureProvider etc)
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.
ChatGPT Plus code review with improvement suggestions:
- Add explicit return types to functions. For example,
generateSignature
should have a return typePromise<Proof>
. - Consider adding a documentation block for the
generateSignature
function, explaining its purpose and describing theproof
andsource
parameters. - Rename the
recorder
variable torecorderType
in thegenerateSignature
function to better indicate that it is an enum value and not an instance of a recorder. - Import
RecorderType
from theproof.ts
file and use it as a type for therecorderType
variable.
Here's the updated code with the suggested improvements:
import {
// Other imports...
Proof,
RecorderType, // Add this import
} from '../repositories/proof/proof';
import { FactsProvider } from './facts/facts-provider';
import { CaptureAppWebCryptoApiSignatureProvider } from './signature/capture-app-web-crypto-api-signature-provider/capture-app-web-crypto-api-signature-provider.service';
import { SignatureProvider } from './signature/signature-provider';
@Injectable({
providedIn: 'root',
})
export class CollectorService {
// Other code...
/**
* Generates a signature for the given proof using the specified camera source.
*
* @param proof - The Proof object to be signed
* @param source - The CameraSource used for signing the proof
* @returns A promise that resolves to the signed Proof object
*/
async generateSignature(proof: Proof, source: CameraSource): Promise<Proof> {
const recorderType: RecorderType = // Rename this variable
CaptureAppWebCryptoApiSignatureProvider.recorderFor(source);
const signedMessage = await proof.generateSignedMessage(recorderType);
const signatures = await this.signMessage(signedMessage, source);
proof.setSignatures(signatures);
return proof;
}
// Other code...
}```
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.
@tammyyang, regarding suggestions:
- N/A
- Done
- Skip
- N/A prefer
recorder
- N/A
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.
ChatGPT Plus code review with improvement suggestions:
- Add explicit return type to the
recorderFor
function:static recorderFor(source: CameraSource): RecorderType
. - Consider adding a documentation block for the
recorderFor
function, explaining its purpose and describing thesource
parameter. - Rename the
source
parameter in therecorderFor
function tocameraSource
for better clarity.
Here's the updated code with the suggested improvements:
// Other imports...
import { RecorderType, Signature } from '../../../repositories/proof/proof';
import { SignatureProvider } from '../signature-provider';
@Injectable({
providedIn: 'root',
})
export class CaptureAppWebCryptoApiSignatureProvider
extends SignatureProvider {
// Other code...
/**
* Determines the appropriate recorder type based on the camera source.
*
* @param cameraSource - The CameraSource used for determining the recorder type
* @returns The RecorderType associated with the given camera source
*/
static recorderFor(cameraSource: CameraSource): RecorderType {
switch (cameraSource) {
case CameraSource.Photos:
return RecorderType.UploaderWebCryptoApiSignatureProvider;
case CameraSource.Camera:
return RecorderType.CaptureAppWebCryptoApiSignatureProvider;
default:
return RecorderType.CAPTURE; // Update to the renamed key
}
}
// Other code...
}```
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.
@tammyyang regarding suggestions:
- Done
- Done
- N/A
source
is better because type referenceCameraSource
. Renaming tocameraSource
would make sense if it was pure JavaScript, but sinceCameraSource
type reference etc ... hope it explains why.
┆Issue is synchronized with this Asana task by Unito