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

credential-bbs #1408

Open
wants to merge 19 commits into
base: next
Choose a base branch
from

Conversation

veromassera
Copy link
Contributor

What issue is this PR fixing

This plugin implements a verifiable credential provider with BBS signature.
This provider enables the issuing of credentials and the generation of presentations from BBS signatures.

It also offers the ability to verify credentials and presentations.

This type of credential allows the holder to share credentials containing only a selected portion of the original VC data through selective disclosure, giving the holders real control over which data is shared.

The plugin extends the Veramo agent with the functionality of createSelectiveDisclosureCredentialBbs and verifyDerivedProofBbs.

This implementation is built on top of the jsonld-signatures-bbs libraries provided by mattrglobal https://github.com/mattrglobal/jsonld-signatures-bbs

Quality

Check all that apply:

  • I want these changes to be integrated
  • I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • I added integration tests.
  • I did not add automated tests because _________, and I am aware that a PR without tests will likely get rejected.

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 71.77673% with 359 lines in your changes missing coverage. Please review.

Project coverage is 88.91%. Comparing base (01de67d) to head (00c7a3f).

Files with missing lines Patch % Lines
packages/credential-bbs/src/custom-key-manager.ts 49.37% 122 Missing ⚠️
packages/credential-bbs/src/types.ts 0.00% 74 Missing ⚠️
...ckages/credential-bbs/src/CredentialProviderBBS.ts 81.81% 58 Missing ⚠️
...ckages/credential-bbs/src/bbs-credential-module.ts 80.29% 53 Missing ⚠️
packages/credential-bbs/src/index.ts 0.00% 16 Missing ⚠️
...kages/credential-bbs/src/suites/BbsBlsSignature.ts 90.74% 10 Missing ⚠️
.../credential-bbs/src/module-types/jsonld/index.d.ts 0.00% 8 Missing ⚠️
packages/credential-bbs/src/bbs-suites.ts 87.27% 7 Missing ⚠️
packages/credential-bbs/src/bbs-suite-loader.ts 87.50% 5 Missing ⚠️
...edential-bbs/src/bbs-selective-diclosure-plugin.ts 92.15% 4 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1408      +/-   ##
==========================================
- Coverage   89.72%   88.91%   -0.81%     
==========================================
  Files         177      189      +12     
  Lines       26818    28090    +1272     
  Branches     2220     2329     +109     
==========================================
+ Hits        24062    24977     +915     
- Misses       2756     3113     +357     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@veromassera
Copy link
Contributor Author

Hi @nickreynolds.
The tests ran successfully, however, other tests unrelated to this PR are failing.
There's a timeout in 'browser tests' and another failure in 'REST integration tests › DID discovery › should discover did by alias and profile vc'

How do we proceed?

@nickreynolds
Copy link
Contributor

@veromassera the test failure appears related to a dependency issue in browser.

Module not found: Error: Can't resolve 'vm' in '/home/runner/work/veramo/veramo/node_modules/.pnpm/[email protected]/node_modules/asn1.js/lib/asn1'

I see that the package you're relying on that brings in this dependency should work in all environments, but there might be some resolution issue that needs to be addressed. Perhaps @mirceanis has more insight

@veromassera
Copy link
Contributor Author

Hi @nickreynolds . You're right. I only saw the timeout log.

@veromassera
Copy link
Contributor Author

Hey @nickreynolds, any updates on this? Did you get a chance to look it over? Does the implementation and design of the plugin look good to you?"

@nickreynolds
Copy link
Contributor

@veromassera the implementation and design looks pretty much good, but we haven't had a chance to look into the dependency issue yet. Hopefully we can sometime this week, but if you make any progress on it, let us know

@nickreynolds
Copy link
Contributor

@veromassera I took a quick look. Didn't find the solution, but I did notice that your pnpm lockfile is generated with an incorrect version of pnpm. Make sure you're using pnpm v9

@veromassera
Copy link
Contributor Author

Hi @nickreynolds.

I've been reviewing the package.json configuration and I believe I've resolved the asn1 error. Please verify if the fix is correct.

However, another error still remains: 'Conflicting values for 'process.env.NODE_ENV'.
I believe this issue is beyond my scope and might be related to some pipeline configuration.
Regards

@veromassera
Copy link
Contributor Author

Hi @nickreynolds ,

I've tried everything I can think of to get the tests to pass, but I haven't been successful.

In fact, to try and isolate the issues, I created a new PR called 'PR generated to test if all tests work' that has no code differences from the Next branch. I ran the tests on GitHub and they're failing as well. :(

I would appreciate it if you could take a look at the tests when you have a chance, as I'm stuck at this point

@nickreynolds
Copy link
Contributor

@veromassera the tests failing on next have been fixed, so you should feel free to rebase this branch and see if your tests pass

@veromassera
Copy link
Contributor Author

veromassera commented Sep 5, 2024

Hi @nickreynolds , here's something I don't know why it's happening... just in case you can help me.

In the "PR generated to test if all tests work" there is an error that also occurs in the credential-bbs PR, related to process.node_env in the "browser_test" tests

image

image

The big difference is that after that error in your PR the entire test suite is then executed, but in my PR (credential-bbs) the tests are not executed.
image

Any suggestions? It must be related to React Native compatibility)

@nickreynolds
Copy link
Contributor

@veromassera the error about ReactDOM.render being deprecated does not cause issues and can be ignored (but I can put up a PR to remove that error anyways). I believe the process.env error is the only one you should be concerned with since that's the only difference in the logs

@veromassera
Copy link
Contributor Author

The process env error also occurs in the next branch... and in PR generated to test if all tests work
You can see it here
https://github.com/decentralized-identity/veramo/actions/runs/10721883324/job/29731621266?pr=1417

line 143 of "run browser test" log
image

However, after that error, the tests run on your branches and are not executed on the credential-bbs PR.

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.

3 participants