-
Notifications
You must be signed in to change notification settings - Fork 33
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
Upgrade to newest RN, add Expo SDK, general cleanup. #550
Conversation
@vonovak Thank you so much! We'll make sure to test it, and add any issues with repro steps in comments here. |
@vonovak Sounds like another TODO item is - update the github CI android & ios build actions, to use the new Development Build method. |
c9fd951
to
b659ad9
Compare
69f8325
to
c69ba85
Compare
c69ba85
to
02f5a64
Compare
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 |
@vonovak Thanks! Do you mind updating the README, with the exact steps to launch? (I'm not sure what the prebuild scripts entail) |
Will do. There are two prebuild scripts, one for each platform. They are in package.json |
@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 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:
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! :) |
ed75fe5
to
6f9f465
Compare
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... |
Ok, narrowed down the error to an issue in Which suggests something is going wrong with the |
@@ -5,16 +5,14 @@ | ||
"license": "BSD-3-Clause", | ||
"type": "module", | ||
"main": "./dist/cjs/index.cjs", |
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.
metro loaded this implementation, which is for node. That was the issue
@@ -4,5 +4,8 @@ const { getDefaultConfig } = require('expo/metro-config'); | |||
const defaultConfig = getDefaultConfig(__dirname); | |||
|
|||
defaultConfig.resolver.assetExts.push('cjs'); | |||
// this will be the default in the future, so let's be future-proof | |||
defaultConfig.resolver.enablePackageExports = true; |
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.
this is a major change in how metro resolves packages. It's not needed but it's going to be the default in future, so I figured I'd give it a try
@@ -30,6 +30,9 @@ | |||
} | |||
} | |||
}, | |||
"overrides": { | |||
"@digitalbazaar/http-client": "4.1.0" |
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.
there were 2 versions of this installed, because @interop/did-web-resolver also depends on a different version of http-client
Seeing this after the android prebuild step, |
please use the |
@alexfigtree I updated some scripts in package.json, you can try deleting ios and android directories and running |
@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 |
@kezike Thanks for testing this! Let's move that to a separate issue. (Meaning, adding credentials from outside is currently broken on |
@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. (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.) |
@vonovak @alexfigtree Any objection to changing the Running LCW instructions in the README from |
@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. |
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. |
Merging - huge thank you to @vonovak for doing the work, to @alexfigtree and others for testing! |
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 |
this PR does the following:
ios
andandroid
folders because we should be able to generate them usingexpo prebuild
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 :)