digitalcredentials / learner-credential-wallet

Learner Credential Wallet is a cross-platform iOS and Android mobile application for storing and sharing digital learner credentials.
https://lcw.app
MIT License
54 stars 28 forks source link

Upgrade to newest RN, add Expo SDK, general cleanup. #550

Closed vonovak closed 6 months ago

vonovak commented 7 months ago

this PR does the following:

If anyone could provide reproduction steps for issues, that would be highly appreciated, as I'm not very familiar with the app yet. To run the app, you need to check out this branch, install dependencies, run prebuild and then carry on as usual :)

dmitrizagidulin commented 7 months ago

@vonovak Thank you so much! We'll make sure to test it, and add any issues with repro steps in comments here.

dmitrizagidulin commented 7 months ago

@vonovak Sounds like another TODO item is - update the github CI android & ios build actions, to use the new Development Build method.

alexfigtree commented 7 months ago

I did a fresh clone, yarn install (no --legacy-peer-deps flag), then yarn start, and after installing on my Android device using Expo, I see the following:

Screenshot_20240205-113430 Screenshot_20240205-113439

vonovak commented 7 months ago

Hey! It's possible I messed something up as I was pushing today but:

I'm afk now but will get to this layer today again

dmitrizagidulin commented 7 months ago

@vonovak Thanks! Do you mind updating the README, with the exact steps to launch? (I'm not sure what the prebuild scripts entail)

vonovak commented 7 months ago

Will do. There are two prebuild scripts, one for each platform. They are in package.json

alexfigtree commented 7 months ago

I ran the prebuild for android and came across this error:

Screenshot 2024-02-05 at 2 30 59 PM

Tried running expo again but came across the same errors as before:

Screenshot 2024-02-05 at 2 32 19 PM
vonovak commented 7 months ago

@alexfigtree thank you for testing things out.

Firstly, please share your error information as text. Images are hard to work with mainly because it's harder to copy content from them and also because they are not searchable - if someone searches for some text in your screenshot, github won't find it.

I have pushed a change that fixes an android build issue, but it's unrelated to what you're seeing:

Looking at the first screenshot: you're using yarn, as seen in the log. Please use npm because that way you'll get the same dependencies installed as me and everyone else. Make sure that you don't have yarn.lock in the root, and generally make sure to use clean git state at https://github.com/digitalcredentials/learner-credential-wallet/pull/550/commits/da2959e095a7b5c96736d19634ca070bce9422ed

Secondly, I see "using current versions instead of the recommended...." - that's suspicious, such warning should not be present. Here's my output of the command:

/opt/homebrew/bin/npm run prebuild:android

> learner-credential-wallet@1.0.0 prebuild:android
> EXPO_NO_TELEMETRY=1 EXPO_NO_GIT_STATUS=1 npx expo prebuild --clean -p android && ./disableAndroidFlipper.sh

Git status is dirty but the command will continue because EXPO_NO_GIT_STATUS is enabled...
(node:86135) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
✔ Cleared android code
✔ Created native directory
✔ Updated package.json | no changes
✔ Finished prebuild
++ sed -i '' '/implementation("com.facebook.react:flipper-integration")/s/^/\/\/ /' ./android/app/build.gradle
++ sed -i '' '/ReactNativeFlipper.initializeFlipper(this, reactNativeHost.reactInstanceManager)/s/^/\/\/ /' ./android/app/src/main/java/edu/wallet/MainApplication.kt
++ sed -i '' '/import com.facebook.react.flipper.ReactNativeFlipper/s/^/\/\/ /' ./android/app/src/main/java/edu/wallet/MainApplication.kt

if you're having troubles getting the same output, it might be best to have a call about it where we debug what's going on.

Hope this helps! :)

dmitrizagidulin commented 7 months ago

@vonovak - Also, in case this affects your effort: https://github.com/digitalcredentials/jsonld-signatures/issues/5

dmitrizagidulin commented 6 months ago

Testing latest on iOS. Everything seems to be working, but the following VC is showing up as 'Revoked', which is incorrect:

{
  "@context": [
    "https://www.w3.org/2018/credentials/v1",
    "https://w3id.org/security/suites/ed25519-2020/v1",
    "https://w3id.org/dcc/v1",
    "https://w3id.org/vc/status-list/2021/v1"
  ],
  "type": [
    "VerifiableCredential",
    "Assertion"
  ],
  "issuer": {
    "id": "did:key:z6MkhVTX9BF3NGYX6cc7jWpbNnR7cAjH8LUffabZP8Qu4ysC",
    "name": "Example University",
    "url": "https://cs.example.edu",
    "image": "https://user-images.githubusercontent.com/947005/133544904-29d6139d-2e7b-4fe2-b6e9-7d1022bb6a45.png"
  },
  "issuanceDate": "2020-08-16T12:00:00.000+00:00",
  "credentialSubject": {
    "id": "did:key:z6MkhVTX9BF3NGYX6cc7jWpbNnR7cAjH8LUffabZP8Qu4ysC",
    "name": "Kayode Ezike",
    "hasCredential": {
      "type": [
        "EducationalOccupationalCredential"
      ],
      "name": "GT Guide",
      "description": "The holder of this credential is qualified to lead new student orientations."
    }
  },
  "expirationDate": "2025-08-16T12:00:00.000+00:00",
  "credentialStatus": {
    "id": "https://digitalcredentials.github.io/credential-status-playground/JWZM3H8WKU#2",
    "type": "StatusList2021Entry",
    "statusPurpose": "revocation",
    "statusListIndex": 2,
    "statusListCredential": "https://digitalcredentials.github.io/credential-status-playground/JWZM3H8WKU"
  },
  "proof": {
    "type": "Ed25519Signature2020",
    "created": "2022-08-19T06:55:17Z",
    "verificationMethod": "did:key:z6MkhVTX9BF3NGYX6cc7jWpbNnR7cAjH8LUffabZP8Qu4ysC#z6MkhVTX9BF3NGYX6cc7jWpbNnR7cAjH8LUffabZP8Qu4ysC",
    "proofPurpose": "assertionMethod",
    "proofValue": "z4EiTbmC79r4dRaqLQZr2yxQASoMKneHVNHVaWh1xcDoPG2eTwYjKoYaku1Canb7a6Xp5fSogKJyEhkZCaqQ6Y5nw"
  }
}

(It's showing up as valid on https://verifierplus.org/ )

Investigating...

dmitrizagidulin commented 6 months ago

Ok, narrowed down the error to an issue in @digitalcredentials/security-document-loader, [Error: NotFoundError loading "https://digitalcredentials.github.io/credential-status-playground/JWZM3H8WKU": Cannot read property 'get' of undefined]

Which suggests something is going wrong with the http-client import.

alexfigtree commented 6 months ago

Seeing this after the android prebuild step, yarn start, then using Expo Go app: Error: react-native-quick-crypto is not supported in Expo Go! Use EAS (expo prebuild) or eject to a bare workflow instead., js engine: hermes

vonovak commented 6 months ago

Seeing this after the android prebuild step, yarn start, then using Expo Go app: Error: react-native-quick-crypto is not supported in Expo Go! Use EAS (expo prebuild) or eject to a bare workflow instead., js engine: hermes

please use the development build, not Expo Go. When you run yarn start you should receive instructions on how to do it (alternatively, use Google please). thank you :)

vonovak commented 6 months ago

@alexfigtree I updated some scripts in package.json, you can try deleting ios and android directories and running yarn ios and yarn android. Please let me know if that helps :)

kezike commented 6 months ago

@vonovak I was able to build and run the app in Android. However, as you predicted, there are new issues when adding credentials from outside of the wallet. I can work toward debugging them, but here they are in the meantime:

  1. The app rebundles when control is returned from a browser-based credential claiming portal to the app. Here is a screenshot of this issue: Screenshot_20240228-093820_LearnerCredentialWallet-w_orig-h_570

  2. After the app rebundles, it fails around the same library that @alexfigtree reported about earlier. Here is the full stack trace:

    ERROR  Error: Failed to install react-native-quick-crypto: The native `QuickCrypto` Module could not be found.
    * Make sure react-native-quick-crypto is correctly autolinked (run `npx react-native config` to verify)
    * Make sure gradle is synced.
    * Make sure you ran `expo prebuild`.
    * Make sure you rebuilt the app.
    ERROR  Invariant Violation: "main" has not been registered. This can happen if:
    * Metro (the local dev server) is run from the wrong folder. Check if Metro is running, stop it and restart it in the current project.
    * A module failed to load due to an error and `AppRegistry.registerComponent` wasn't called.
vonovak commented 6 months ago

@vonovak I was able to build and run the app in Android. However, as you predicted, there are new issues when adding credentials from outside of the wallet. I can work toward debugging them, but here they are in the meantime:

hello and thanks for reporting @kezike. I can help if necessary, but I'll need clear repro steps, thank you.

As for the react-native-quick-crypto error, I think that's a different error from the one reported previously. I'd say I'd follow the advice that the error message is giving.

dmitrizagidulin commented 6 months ago

as you predicted, there are new issues when adding credentials from outside of the wallet

@kezike Thanks for testing this! Let's move that to a separate issue. (Meaning, adding credentials from outside is currently broken on main also, unrelated to the RN upgrade.) So, we'll tackle it separately.

dmitrizagidulin commented 6 months ago

@kezike your (and all of ours) main goal at this point is just 1) Make sure the wallet builds and runs on iOS, and 2) Wallet builds and runs on Android. Then we can merge this PR to main and proceed from there.

(I've tested it and it builds & VCs verify on iOS. I need to reinstall Android Studio tho, to make sure it works on android.)

dmitrizagidulin commented 6 months ago

@vonovak @alexfigtree Any objection to changing the Running LCW instructions in the README from yarn start to npm start? (Since we're using npm install anyways, and we don't include yarn in the dependencies, and one can't assume devs have it globally installed.)

alexfigtree commented 6 months ago

@dmitrizagidulin No objections from me, I prefer npm too. Also, I found this error while running on the dev server in ios:

ERROR ViewPropTypes will be removed from React Native, along with all other PropTypes. We recommend that you migrate away from PropTypes and switch to a type system like TypeScript. If you need to continue using ViewPropTypes, migrate to the 'deprecated-react-native-prop-types' package.

alexfigtree commented 6 months ago

Still seeing the above error in Android as well, reported here: https://github.com/digitalcredentials/learner-credential-wallet/issues/575 I suggest merging this PR, but fixing #575 before deploying to any app stores.

vonovak commented 6 months ago

That error likely comes from some 3rd party dep. Let's do the improvements step by step, in different PRs. If we were to put all the fixes in this PR it'd become bloated.

dmitrizagidulin commented 6 months ago

Merging - huge thank you to @vonovak for doing the work, to @alexfigtree and others for testing!

fabrii commented 2 weeks ago

Hi @vonovak.

We can see that you updated the library used for database encryption, and also changed a number there from 256 to 32. Does this means that previous generated wallets will stop working, and we have to reset them?

Thanks