Skip to content
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

Conversation

sultanmyrza
Copy link
Contributor

@sultanmyrza sultanmyrza commented Mar 22, 2023

┆Issue is synchronized with this Asana task by Unito

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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;
Copy link
Contributor Author

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:

  1. Add explicit return types to functions. For example, generateSignedMessage should have a return type Promise<SignedMessage>.
  2. Consider adding a documentation block for the generateSignedMessage function, explaining its purpose and describing the recorder parameter.
  3. Rename the recorder parameter to recorderType in the generateSignedMessage function to better indicate that it is an enum value and not an instance of a recorder.
  4. Define an interface for the SignedMessage object to enforce a specific structure and improve code maintainability.
  5. In the RecorderType enum, the value of the DEFAULT key is 'Capture'. Consider renaming this key to CAPTURE for consistency with the other keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tammyyang,

  1. Done
  2. Done
  3. N/A I think recorder is better
  4. Done
  5. N/A I think Capture is better because others are camelCased (CaptureAppWebCryptoApiSignatureProvider etc)

Copy link
Contributor Author

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:

  1. Add explicit return types to functions. For example, generateSignature should have a return type Promise<Proof>.
  2. Consider adding a documentation block for the generateSignature function, explaining its purpose and describing the proof and source parameters.
  3. Rename the recorder variable to recorderType in the generateSignature function to better indicate that it is an enum value and not an instance of a recorder.
  4. Import RecorderType from the proof.ts file and use it as a type for the recorderType 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...
}```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tammyyang, regarding suggestions:

  1. N/A
  2. Done
  3. Skip
  4. N/A prefer recorder
  5. N/A

Copy link
Contributor Author

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:

  1. Add explicit return type to the recorderFor function: static recorderFor(source: CameraSource): RecorderType.
  2. Consider adding a documentation block for the recorderFor function, explaining its purpose and describing the source parameter.
  3. Rename the source parameter in the recorderFor function to cameraSource 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...
}```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tammyyang regarding suggestions:

  1. Done
  2. Done
  3. N/A source is better because type reference CameraSource. Renaming to cameraSource would make sense if it was pure JavaScript, but since CameraSource type reference etc ... hope it explains why.

@bafu bafu merged commit 0fb04af into milestone-v230223-with-v230310 Apr 2, 2023
@sultanmyrza sultanmyrza deleted the feat-v230223-v230310-special-provider-for-photos-taken-from-capture-app branch June 16, 2023 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants