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: remote proof bundles #1022

Merged

Conversation

wadeking98
Copy link
Contributor

@wadeking98 wadeking98 commented Nov 3, 2023

Summary of Changes

Added ability to fetch remote proof bundles rather than using hardcoded values.
Note: to use remote proof bundles set the following variable in your .env file PROOF_TEMPLATE_URL=https://raw.githubusercontent.com/wadeking98/bc-wallet-mobile/proof-template-bundle/

Related Issues

N/A

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.

}
return Promise.resolve(applyTemplateMarkers(templateData))
}
return this.remoteServer.get('proof-templates.json').then((response) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

how is this file being cached?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right now it's just saving the templates in temporary memory, I'm going to implement saving the templates to local storage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so right now it will fetch the templates once when is starts up, but I'll implement some caching

@jleach jleach self-requested a review November 3, 2023 23:48
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
@wadeking98 wadeking98 force-pushed the feat-remote-proof-bundles branch from 6a2deb2 to 942b833 Compare November 8, 2023 00:07
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #1022 (cd4241e) into main (6c7a4b3) will increase coverage by 0.19%.
The diff coverage is 67.44%.

@@            Coverage Diff             @@
##             main    #1022      +/-   ##
==========================================
+ Coverage   60.92%   61.12%   +0.19%     
==========================================
  Files         180      181       +1     
  Lines        5444     5520      +76     
  Branches     1559     1571      +12     
==========================================
+ Hits         3317     3374      +57     
- Misses       2103     2122      +19     
  Partials       24       24              
Files Coverage Δ
...ackages/legacy/core/App/contexts/configuration.tsx 100.00% <ø> (ø)
...ckages/legacy/core/App/screens/ProofRequesting.tsx 66.66% <50.00%> (ø)
...es/legacy/core/App/screens/ProofRequestDetails.tsx 73.80% <50.00%> (-0.39%) ⬇️
...s/legacy/core/App/hooks/proof-request-templates.ts 55.00% <50.00%> (-45.00%) ⬇️
packages/legacy/core/App/utils/proofBundle.ts 73.43% <73.43%> (ø)

... and 3 files with indirect coverage changes

@jleach
Copy link
Contributor

jleach commented Nov 8, 2023

Juat curious, should can this go in its own package.

@wadeking98
Copy link
Contributor Author

Juat curious, should can this go in its own package.

Personally I feel it's a bit small for it's own package and I don't think it would be used outside of bifold

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Copy link
Contributor

@bryce-mcmath bryce-mcmath left a comment

Choose a reason for hiding this comment

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

Just looking for a comment, otherwise looks good to me

packages/legacy/core/App/utils/proofBundle.ts Show resolved Hide resolved
@wadeking98 wadeking98 merged commit bf5cf8a into openwallet-foundation:main Nov 8, 2023
6 of 7 checks passed
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.

4 participants