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: OpenID4VP Proof request Base work and UI #1296

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

MosCD3
Copy link
Contributor

@MosCD3 MosCD3 commented Oct 23, 2024

Summary of Changes

This PR introducing the flow of accepting a proof request from an issuer using OIDC flow

Related Issues

Please reference here any issue #'s that are relevant to this PR, or simply enter "N/A" if this PR does not relate to any existing issues.

Pull Request Checklist

Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.

  • All commits contain a DCO Signed-off-by line (we use the DCO GitHub app to enforce this);
  • Updated LICENSE-3RD-PARTY.md for any added dependencies or vendored components;
  • Updated documentation as needed for changed code and new or modified features;
  • Added sufficient tests so that overall code coverage is not reduced.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

Pro Tip 🤓

  • Read our contribution guide at least once; it will save you a few review cycles!
  • Your PR will likely not be reviewed until all the above boxes are checked and all automated tests have passed.

PR template adapted from the Python attrs project.

Signed-off-by: Mostafa Gamal <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]>
@MosCD3 MosCD3 added the work in progress This issue is being worked on label Oct 29, 2024
@MosCD3 MosCD3 marked this pull request as ready for review October 31, 2024 17:16
Mostafa Youssef and others added 12 commits October 31, 2024 13:19
Signed-off-by: Mostafa Youssef <[email protected]>
Signed-off-by: Mostafa Youssef <[email protected]>
Signed-off-by: Mostafa Youssef <[email protected]>
Signed-off-by: Mostafa Youssef <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]>
@jleach jleach marked this pull request as draft November 4, 2024 20:00
@@ -197,14 +199,14 @@
"react-native-fs": "^2.16.6",
"react-native-gesture-handler": "^1.10.3",
"react-native-get-random-values": "^1.7.0",
"react-native-gifted-chat": "*",
Copy link
Contributor

@cvarjao cvarjao Nov 4, 2024

Choose a reason for hiding this comment

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

I can understand that * is super generic, but any particular reason for setting to a very specific peer dependencies? Usually keep compatibility at major level (starting with ^) is fine. Similar comment for react-native-qrcode-svg, and react-native-reanimated changes below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "*" caused installing latest stable version, and it caused a whole lot of conflicts with other packages that are mot updated, and to do that in this PR is a big mess. I had to fix the version to get things aligned together and get the app to build and tests to succeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, I believe leaving yarn to pickup latest updated versions for any packages is recipe for bugs. We have to dedicate PRs to solely update specific packages.

Copy link
Contributor

@cvarjao cvarjao Nov 4, 2024

Choose a reason for hiding this comment

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

we can be specific at dependencies but more liberal with peer dependencies. Also, dependencies should not be updated as a side-effect, it should be explicit and managed by the lock file.

Copy link
Contributor

Choose a reason for hiding this comment

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

keep in mind that yarn install MAY produce unexpected dependency change. yarn install --immutable is the safest command to use and void unexpected changes to the lock file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I never meant to touch any of that :). It NEVER succeeded until I fixed versions for those packages. Let me know if you have another idea and we can hop on a call and discuss

@MosCD3 MosCD3 removed the work in progress This issue is being worked on label Nov 4, 2024
@MosCD3 MosCD3 marked this pull request as ready for review November 4, 2024 21:03
@MosCD3 MosCD3 force-pushed the feat/moscd3/openid-presentation branch from 1b04783 to 3c80171 Compare November 4, 2024 22:32
Copy link

sonarcloud bot commented Nov 4, 2024

@MosCD3 MosCD3 requested a review from cvarjao November 5, 2024 00:01
@jleach jleach changed the title feat: OpenId4VP Proof request Base work and UI feat: OPENID4VP Proof request Base work and UI Nov 5, 2024
@jleach jleach changed the title feat: OPENID4VP Proof request Base work and UI feat: OpenID4VP Proof request Base work and UI Nov 5, 2024
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.

2 participants