-
Notifications
You must be signed in to change notification settings - Fork 7
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
ionic - fixed the public key #2995
Comments
➤ Sam commented: Before starting this task nice to read Comment by @Ethan Wu on dashboard - fixed the public key ( https://app.asana.com/0/0/1205290188874237/1205381185960130/f ) |
➤ Tammy Yang commented: Waiting Provide feedback for frontend to implement ( https://app.asana.com/0/0/1205564438578232 ), move to sprint 23.10.09 |
➤ Bofu Chen commented: Sam Is there any blocker for the task? Please feel free to ask James' support if you need any help |
➤ Sam commented: Bofu Chen, okay. Currently I'm checking where it could go wrong. I want to check changes before and after this PR ( #2667 ) first. If I can not find the root cause I will definitely ask James for help. |
➤ Bofu Chen commented: Sam James knows the root cause well and can save your efforts much. Please collaborate with him closely. Thanks! |
➤ Sam commented: Kenny Hung is ✓ Backend verification should use integritySha as sign message ( https://app.asana.com/0/1201016280880508/1205789216127644 ) deployed to prod? |
➤ Kenny Hung commented: Sam (cc Scott Yan) No. This part haven't deployed yet. |
➤ Sam commented: James Chien I have a question can we add extra fields to Proof ( https://github.com/numbersprotocol/capture-lite/blob/master/src/app/shared/repositories/proof/proof.ts#L21 ) interface? Just extra information for context. In the past we had this task ✓ Special provider for photos "taken" from Capture App ( https://app.asana.com/0/1201016280880500/1203865135624457/f ) which was implemented in this PR ( #2667 ). In that PR I add cameraSource field to Media interface ( 71659da ) and it was working fine because I could pass cameraSource to this.generateSignature(proof, source); (
But now I see that during buildFormDataToCreateAsset(proof: Proof) ( capture-lite/src/app/shared/dia-backend/asset/dia-backend-asset-repository.service.ts Line 385 in 8165da9
|
➤ Sam commented: James Chien in capture cam there is Proof interface, what was the purpose of IndexedProofView (
|
➤ James Chien commented: Sam Sorry I forgot to answer your last question. Yes we could add new field to Proof if needed. I can’t really explain what was the purpose of IndexProofView or IndexedAssets because these are implemented over 2 years ago and I’m not the designer of these interfaces and classes. What I know it the original design of the Proof is different with the backend asset, and both these Indexed interfaces are created to map the Proof object to other forms in order to meet some requirements like interacting with backend or get data by index with O(1),etc. |
➤ Sam commented: James Chien, I got the draft code and ready to test. I have 1 question regarding '@numbersprotocol/nit' in TypeScript project (capture-lite) after installing via NPM it's throwing Could not find a declaration file for module '@numbersprotocol/nit'. I also tried the quick fix suggestion but didn't worked either. |
➤ James Chien commented: Sam I just checked that the published version of nit on npm does not include any type declaration files. Could you just surpress the warning for now? I'll create a new task for nit to update type declarations. |
➤ James Chien commented: Task created Add declaration files to support typechecking ( https://app.asana.com/0/1203248692432100/1205924884876234/f ) |
➤ Sam commented: Kenny Hung, just STATUS UPDATE. When I use nit module in capture cam I got some errors during build (as shown in attachment file). It's not related to ionic - fixed the public key ( https://app.asana.com/0/0/1205290188874236 ). However to fix ionic - fixed the public key ( https://app.asana.com/0/0/1205290188874236 ) I need to use nit module as mentioned here ( https://app.asana.com/0/0/1205290188874237/1204366590559491/f ). My plan (after re-releasing Update Capture Eye html to match new design ( https://app.asana.com/0/0/1205877976191503 )) I will
|
➤ Sam commented: James Chien here are some things I want to share regarding ionic - fixed the public key ( https://app.asana.com/0/0/1205290188874236 ).
|
➤ Sam commented: James Chien, just status update some errors can be resolved by changing configs and installing extra libs (more on GitHub ( #3093 (comment) )). I will work on 2nd part of errors and see if it can be resolved in acceptable way. |
➤ Sam commented: James Chien I have a proposal but before that I want to review some context.
So assuming the goal is to add extra field integritySha for Create an asset ( https://dia-backend-dev.numbersprotocol.io/api/v3/redoc/#operation/assets_list ) API call. Can I use "ethers" directly? Based on sample ( https://github.com/numbersprotocol/numbers-playground/blob/main/capture-signature/index.js#L5 )script here is what I mean. On ionic side instead of doing this import * as nit from "@numbersprotocol/nit"; import { ethers } from "ethers"; async function generateIntegritySha(proofMetadata) { const data = JSON.stringify(proofMetadata, null, 2); const dataBytes = ethers.toUtf8Bytes(data); const integritySha = await nit.getIntegrityHash(dataBytes); return integritySha; } I propose to do this as tmp WORKAROUND until @numbersprotocol/nit ( https://www.npmjs.com/package/@numbersprotocol/nit ) become "Browser-Friendly". import { ethers } from "ethers"; async function generateIntegritySha(proofMetadata) { const data = JSON.stringify(proofMetadata, null, 2); const dataBytes = ethers.toUtf8Bytes(data); const integritySha = await (ethers.utils.sha256(assetBytes)).substring(2) ( https://github.com/numbersprotocol/nit/blob/bbbb43fedd61d6cc80d19fe14394b0d5d124e208/src/nit.ts#L616C15-L616C15 ) return integritySha; }
James Chien, please let me know if I missed anything or if my understanding is correct 🙏. |
➤ James Chien commented: Sam It's good for me. Bofu Chen Since it's different from what we've discussed in ✓ Provide feedback for frontend to implement ( https://app.asana.com/0/0/1205564438578232 ) I'll involve you in the discussion. Sam has mentioned there are some issues using nit module in ionic because nit module depends on some modules that is not supported in browser, and using browserify and pollyfills to workaround it will make things more complext. Sam's proposal is not using nit module in the fix, just making sure the signature is correct. Then after nit module is updated to support direct use in browser, Capture Cam could switch back to use nit for integritySha generation. |
➤ Bofu Chen commented: James Chien Please help ensure that Sam's implementation is correct, and the generated signature can be verified correctly. Then the proposal is okay to me. You can even help implement the temporary solution directly if it's more efficient. Thanks |
➤ Sam commented: Kenny Hung I finished with ✓ Update Capture Eye html to match new design ( https://app.asana.com/0/0/1205877976191503 ). Now I can resume working on ionic - fixed the public key ( https://app.asana.com/0/0/1205290188874236 ). Last time there was issue using nit module and I proposed temporary workaround as mentioned here ( https://app.asana.com/0/0/1205290188874236/1205959803614866/f ). Today I will convert this bubble plugin code ( https://app.asana.com/0/0/1205290188874237/1205955332495842/f ) to ionic alternative and test if verification works. If everything works I can submit release for v231017-capture-cam-ionic-launch ( https://app.asana.com/0/0/1205657810319901 ) |
➤ James Chien commented: Sam For testing if the signature is correct, you can call the backend verification API for the asset.
Replace the cid, and replace the address with integrity wallet address. If you get results like {"signed_actions":{"action":"mint","method":"POST","signed_url":"https://dia-backend-qa.numbersprotocol.io/api/v3/assets/bafkreihbbx54uufapyhoa7pzpo5zwssjqsjjdro3eaara32worfbyovzp4/mint/?signature=None","expired_at":null}} it means the verification is successful. |
➤ Sam commented: Kenny Hung, let's aim release for tomorrow. |
➤ James Chien commented: fix: WIP: signature generation · e4a6c87 (github.com) ( e4a6c87 ) The branch is WIP, it mainly does:
It still needs to do:
|
➤ Sam commented: Kenny Hung, (cc: James Chien) I pull James Chienchanges ( e4a6c87 ) and will work on TODOs 2, 1, 3. James Chien, if you can finish faster than me please let me know. |
➤ Sam commented: James Chien, status update. I added this commit WIP: store CameraSource in Proof ( a47617a ) |
➤ James Chien commented: Sam 1 is pretty tricky, it might be related to some low level browser encoding behavior. I’ll check if this behavior happens on Android/iOS emulator first (I use web for testing yesterday). If unfortunately yes, we might need to find some creative way to fix it. I have a few in my minds. If you finish CameraSource part, you can help with fixing Proof test. Basically the hardcoded signature needs to be updated to the one generated using updated methods. |
➤ James Chien commented: Sam For 2, there are two FIXME comments added for the part that I hardcoded the CameraSource.Camera, could you fix those parts? |
➤ Sam commented: James Chien,
|
➤ James Chien commented: Some information about the CRLF (\r\n) issue: https://stackoverflow.com/questions/69835705/formdata-textarea-puts-r-carriage-return-when-sent-with-post ( https://stackoverflow.com/questions/69835705/formdata-textarea-puts-r-carriage-return-when-sent-with-post ) https://blog.whatwg.org/newline-normalizations-in-form-submission ( https://blog.whatwg.org/newline-normalizations-in-form-submission ) https://bugzilla.mozilla.org/show_bug.cgi?id=1651887 ( https://bugzilla.mozilla.org/show_bug.cgi?id=1651887 ) In short, there is some weird spec stuff, making this quirk when using multipart/form-data and fetch. I honestly don’t know why CRLF will be used in spec in 2023 but anyway we need to get around it. |
➤ James Chien commented: I do not understand why this task involves open source and why a open source proposal is needed in this task. Is the task not for fixing an issue in Capture Cam? |
➤ Tammy Yang commented: The public signature implementation and its verification will be open-sourced, I think this not only nature, match our brand value and Sherry Chung has mentioned it in the kickoff. It is a fix to the Capture Cam, yes, but we also need to make sure the verification can be transparent. Otherwise, what's the point of Numbers Protocol? 😅 |
➤ Tammy Yang commented: As long as it can be
Then I am fine. It's not me adding more features to this task, but the task is taking so long. It should have already out step #2 and enter the discussion of #3 and #4, but it was never happened because I was always told this task needed to be postponed (cc SamKenny HungScott Yan) |
➤ James Chien commented: Answered in daily sync: the workaround is applied to solve the issue Capture Cam communicate with backend, not changing anything related to the signature generation or verification part. We can open source that part with no problem. |
➤ James Chien commented: Olga Please help review the pull request fix(AssetDeserializer): handle incorrect signed_metadata during asset creation due to browser-based client behavior by shc261392 · Pull Request #1215 · numbersprotocol/storage-backend (github.com) ( https://github.com/numbersprotocol/storage-backend/pull/1215 ) |
➤ Sam commented: James Chien, just want to confirm backend apis for
are deployed to prod or QA only? Because I will test and prepare firebase release. In firebase release I usually use prod as a backend site. |
➤ James Chien commented: Sam Not deployed to anywhere yet. It should be deployed to QA site today or tomorrow and prod site next Monday. If you need a site for testing I just manually deployed dev site. The workaround applies when you uploading a new Capture, so please test on dev site again using new Capture instead of already uploaded ones. And don't use feature-add-integrity-sha-on-asset-creation-update-signed-metadata branch, because the update signed metadata is not working as mentioned Comment by @james Chien on ionic - fixed the public key ( https://app.asana.com/0/0/1205290188874236/1206009900878655/f ) So just base on feature-add-integrity-sha-on-asset-creation, and make sure the previous change to ProofMetadata, your change to CameraSource, Proof unit testing fix are included. Backend will handle the CRLF issue. If the backend deployment status blocks you from release, discuss with QA and see what's the best solution. |
➤ Kenny Hung commented: Sam (cc Scott YanTammy Yang) I suggest we should release ionic in backend-prod instead of backend-qa, please help to open the dependency card about which blocked verify in backend-prod, then we could make sure when backend deploy then ionic could release fast. |
➤ Sam commented: Kenny Hung, okay I will add dependency. So basically
James Chien, fix(AssetDeserializer): handle incorrect signed_metadata during asset creation due to browser-based client behavior ( https://app.asana.com/0/1201016280880508/1206009845131465/f ) will be part of ✓ v231109-storage-backend ( https://app.asana.com/0/0/1205877976191488 )? |
➤ Kenny Hung commented: Sam or James Chien (cc Tammy Yang) When this task is deliver to QA, please also provide the source code & the detailed steps to explain how it works. |
➤ James Chien commented: Kenny Hung The source code in Capture Cam, or do you need an individual source code that can be executed without building Capture Cam? If former, Sam should be able to handle it. If there's any part in the flow Sam is unsure, please ask me and I can provide explanation for that part. If it is the latter it will require extra effort to prepare such code. |
➤ Sam commented: Kenny Hung need to clarify Comment by @kenny Hung on ionic - fixed the public key ( https://app.asana.com/0/0/1205290188874236/1206017653303426/f ). To test ionic - fixed the public key ( https://app.asana.com/0/0/1205290188874236 )
This is expected test scenario right? Is my understanding correct Kenny Hung? Please let me know if anything is mising. Also Comment by @kenny Hung on ionic - fixed the public key ( https://app.asana.com/0/1201016280880500/1205290188874236/1206017653303426 ) is it more related to [gitbook updated] explain how signature can be verified ( https://app.asana.com/0/0/1205998583955318 )? |
➤ Sherry Chung commented: Sam Kenny is taking leave today. Please release what you think is match the the requirement & description. QA want to receive
If you need a suggestion about what should be released, please consult James Chien . What QA would do test is based on a. The way that our user will use Capture Cam b. the instruction that dev team provide (should also match what our user will used and also the information we provide in gitbook) So the testing scenario should be the dev team letting QA know what's the best scenario for user to use the new feature. |
➤ Sam commented: James Chien, ✓ fix(AssetDeserializer): handle incorrect signed_metadata during asset creation due to browser-based client behavior ( https://app.asana.com/0/1201016280880508/1206009845131465 ) is deployed to prod? Or how can I keep track if it's deployed or not yet. |
➤ James Chien commented: Sam Please check release for storage backend 231128 on reminder-released channel. New dev release = deploy to QA site New production release = deploy to prod site |
➤ James Chien commented: Sam I believe the version is currently deploying to prod. There will be a new production release message later. |
➤ Sam commented: James Chien, I tested and asset can be verified successfully which is good however I found another issue [issue] captures uploaded from gallery can not be fetched from backend ( https://app.asana.com/0/0/1206033018255018 ). I'm not sure if it's related when capture is uploaded from galley I set recorder id ( https://github.com/numbersprotocol/capture-lite/blob/master/src/app/shared/collector/signature/capture-app-web-crypto-api-signature-provider/capture-app-web-crypto-api-signature-provider.service.ts#L31 ) to UploaderWebCryptoApiSignatureProvider |
➤ James Chien commented: Sam It is related to the behavior introduced in ✓ scenarios for assetCreator on assetTree ( https://app.asana.com/0/1201016280880508/1205724158849003 ). The uploaded asset, if not set to claim_is_creator, its creator will be null. is_original_owner filter only returns result with condition owner==creator Originally, this filter is applied to distinguish uploaded Capture and received PostCapture. Uploaded Capture’s creator will be owner itself, while PostCapture is not. This is also a backend issue because backend origin_type field will give wrong information for such asset (consider unclaimed, no creator asset to be PostCapture) Sam My suggestion is let the QA know the current issue and see if from users perspective it is acceptable or not. And see if it is possible to workaround this. I will create the issue on backend side. Note that this should only happens on when you use UploaderWebCryptoApiSignatureProvider and does not set claim_is_creator to true. This shouldn’t be related to any change in this signature fix task. |
➤ Sam commented: James Chien, (cc: Kenny Hung, Tammy Yang) done fix: Set 'claim_is_creator' to true for capture cam uploads ( 9c05029 ). Now I can upload and verify an asset. Tested on real android, iOS device. Preparing firebase release. |
➤ Tammy Yang commented: Kenny HungSam please make sure the "claim_is_origin" can be a toggle shown in the Capture Cam in the NEXT release (similar to dashboard) |
➤ Kenny Hung commented: Sam (cc Ethan WuJames Chien) Just curious about integrityCid of asset, Why the integrityCid type are different? For example, asset from Cam the integrityCid is dictionary ( https://ipfs-pin.numbersprotocol.io/ipfs/bafkreibaoq6ewxrddu2zffnu7dfq52crmv565fs6ttbk22bgmrhleu7bzq ). (Nid: bafybeibaym7u4nlgsiq7ew5nkalkiuxl3uxf5hb7ujniqctgnyqrxug7kq) But from Dashboard, the integrityCid is just string ( https://ipfs-pin.numbersprotocol.io/ipfs/bafkreiedccg3v4uvzymmhv7nsjhuavpynrmda6rvwk2qzgpjerw54k4sme ). (Nid: bafybeibddiijpnaz5ytbnu7cnh7cnhdc36xz2oge7wurweqaratqwf6bmi) |
➤ Ethan Wu commented: maybe because of this comment: Comment by @james Chien on dashboard - fixed the public key ( https://app.asana.com/0/0/1205290188874237/1205946558097191/f ) need james to confirm |
➤ James Chien commented: Kenny HungEthan Wu When creating integrity_cid in backend, backend loads the signed_metadata into dict and write it to JSON. The final result should be JSON object like the Cam example. The dashboard example looks like did one more stringifiying. We should find out why and make it like the Capture Cam example. Also I found that backend didn’t update the integrity_cid creation to write JSON with indent=2. We should update it in backend in next release. |
➤ James Chien commented: Kenny HungEthan Wu Task created for dashboard: fix signed_metadata value when creating assets ( https://app.asana.com/0/1201016280880505/1206031417078018/f ) Task created for backend: Use the standard indent in metadata_cid creation ( https://app.asana.com/0/1201016280880508/1206031417078021/f ) |
➤ James Chien commented: Kenny Hung Also there is another task created for backend, mentioned in Comment by @james Chien on ionic - fixed the public key ( https://app.asana.com/0/0/1205290188874236/1204366590559528/f ), which the Capture Cam upload creator claim toggle is dependent on. Incorrect origin_type when uploaded asset does not claim creator ( https://app.asana.com/0/1201016280880508/1206031417078024/f ) |
┆Issue is synchronized with this Asana task by Unito
┆Created By: Kenny Hung
The text was updated successfully, but these errors were encountered: