fix(vc): change pubKey input from Buffer to Uint8Array

[karimStekelenburg]

This PR addresses an issue that occurred when running the deriveProof test case in the W3cCredentialService tests. Strangely enough this issue only occurred when running in a NodeJS environment (which uses node-bbs-signatures) instead of the WASM fallback.

Huge thanks to @blu3beri for helping debug this issue. This one wasn't easy to catch.

Signed-off-by: Karim [email protected]

[codecov-commenter]

Codecov Report

Merging #935 (49c35d4) into 0.3.0-pre (65579dd) will decrease coverage by 1.95%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           0.3.0-pre     #935      +/-   ##
=============================================
- Coverage      86.65%   84.69%   -1.96%     
=============================================
  Files            515      517       +2     
  Lines          12552    12573      +21     
  Branches        2132     2136       +4     
=============================================
- Hits           10877    10649     -228     
- Misses          1580     1793     +213     
- Partials          95      131      +36     

Impacted Files	Coverage Δ
...c/signature-suites/bbs/BbsBlsSignatureProof2020.ts	9.52% <ø> (-68.26%)	⬇️
...les/vc/signature-suites/bbs/BbsBlsSignature2020.ts	6.61% <0.00%> (-81.00%)	⬇️
packages/core/src/crypto/BbsService.ts	38.23% <0.00%> (-52.95%)	⬇️
packages/core/src/modules/vc/jsonldUtil.ts	42.50% <0.00%> (-27.50%)	⬇️
...src/modules/vc/signature-suites/bbs/deriveProof.ts	16.21% <0.00%> (-16.22%)	⬇️
packages/core/src/crypto/WalletKeyPair.ts	78.72% <0.00%> (-8.52%)	⬇️
packages/core/src/utils/validators.ts	60.71% <0.00%> (-7.15%)	⬇️
packages/core/src/wallet/IndyWallet.ts	57.76% <0.00%> (-6.38%)	⬇️
...ckages/core/src/modules/vc/W3cCredentialService.ts	71.42% <0.00%> (-4.52%)	⬇️
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65579dd...49c35d4. Read the comment docs.

[karimStekelenburg]

Tests seem to fail for Node 17. It doesn't look like it's related to the changes in this PR. @TimoGlastra, does this error look familiar to you by any chance?

[berendsliedrecht]

I have ran the tests again, and node 18 now failed. Locally it works (m1 Mac), so it might be some issue with the node-addon.

[TimoGlastra]

We're having some issues with flaky tests from the credentials tests with events listener being started after the action has been performed resulting in a race condition. This is often if the CI randomly fails. I made a start at it here, but there's more test we need to fix: #893

Not sure if this is it, but migth be related

[TimoGlastra]

Hmm after looking at it again, this looks like a different issue than the flaky tests as it's only failing on BBS tests. Maybe a linux + node 18 issue?

[karimStekelenburg]

As discussed in the WG call today, we will work around this issue for now by forcing the library to use the WASM environment. We can do this by setting an environment variable called BBS_SIGNATURES_MODE to WASM. I will add this to the CI for now, which should make sure it succeeds.

However, this does open up the possibility for confusion. If an unknowing developer runs AFJ in Node 17 or 18, it will use node-bbs-signatures instead of the WASM build and cause errors. I'm not totally sure how we should deal with this, I guess documenting it is a start...

[karimStekelenburg]

Ok, I've added the environment variable in 49c35d4 which should hopefully make sure the tests succeed for all Node versions (waiting on CI to finish atm, wish me luck 😄). One thing I'd like to double check: in order to not have to overhaul the entire workflow structure, I've added the environment variable top level. This means that the tests for Node 14 and 16 (which did succeed using node-bbs-signatures) will also use the WASM build.

Please let me know if there are any objections regarding this.

@TimoGlastra, @blu3beri, would love to hear your thoughts on this.

[TimoGlastra]

Documenting would be good. In addition I think there's two approaches here (maybe more):

• set the environment variable by default before importing the bbs library
• logging a warning if the node version is used and we're in node 17/18. The hard part is, how do we know which version is used? We could check if the node bbs signatures install succeeded (by trying to import it) and then logging the warning
• logging a warning if the env variable is not set and we're in node 17/18. This will push the user to always set the env variable explicitly

[TimoGlastra]

Could you maybe open an issue in the node bbs signatures repo so the maintainers are aware of the issue?

[karimStekelenburg]

Documenting would be good. In addition I think there's two approaches here (maybe more):

• set the environment variable by default before importing the bbs library
• logging a warning if the node version is used and we're in node 17/18. The hard part is, how do we know which version is used? We could check if the node bbs signatures install succeeded (by trying to import it) and then logging the warning
• logging a warning if the env variable is not set and we're in node 17/18. This will push the user to always set the env variable explicitly

Since usability and simplicity are primary goals of AFJ, I'm personally in favour of the first option. I'd like to minimise the amount of edge cases a user has to handle by themselves, as that increases the amount of gotcha's.

Could you maybe open an issue in the node bbs signatures repo so the maintainers are aware of the issue?

Good point, I will raise this issue to make them aware.

---

Just to be sure before I move on to the next issue, how do you feel about the top level environment variable in the CI? If we go with the first option you mentioned this isn't needed anymore, but just in case we take the logging & documentation route..

[karimStekelenburg]

Ahh great! Now we broke Node 14 again! 💯

[TimoGlastra]

Ahh great! Now we broke Node 14 again! 💯

Lol. Maybe we should take the first route and only set it for node 17 & 18?

[karimStekelenburg]

Ahh great! Now we broke Node 14 again! 💯

Lol. Maybe we should take the first route and only set it for node 17 & 18?

Yeah... I tried running the tests on my machine (M1) locally with 14 and it succeeds. I will try to run it with Node 14 on an Ubuntu system now, just to get a better understanding where the problem lies, but I agree, that solution is probably better.

However, should we really set this environment variable just before importing the package? Isn't it cleaner to do this more 'top level', e.g. in the Agent class?

[TimoGlastra]

Ahh great! Now we broke Node 14 again! 💯

Lol. Maybe we should take the first route and only set it for node 17 & 18?

Yeah... I tried running the tests on my machine (M1) locally with 14 and it succeeds. I will try to run it with Node 14 on an Ubuntu system now, just to get a better understanding where the problem lies, but I agree, that solution is probably better.

However, should we really set this environment variable just before importing the package? Isn't it cleaner to do this more 'top level', e.g. in the Agent class?

If we do it in the agent class the bbs library is probably already imported in which case setting the env variable doesn't matter anymore. So we need to make sure it happens before the import of bbs library

[berendsliedrecht]

I will try to debug the issue tomorrow with the node library, however a temporary fix would be best for now.

WASM should not be that much slower so it's for sure not the end of the world.

[karimStekelenburg]

Ahh great! Now we broke Node 14 again! 💯

Lol. Maybe we should take the first route and only set it for node 17 & 18?

Yeah... I tried running the tests on my machine (M1) locally with 14 and it succeeds. I will try to run it with Node 14 on an Ubuntu system now, just to get a better understanding where the problem lies, but I agree, that solution is probably better.
However, should we really set this environment variable just before importing the package? Isn't it cleaner to do this more 'top level', e.g. in the Agent class?

If we do it in the agent class the bbs library is probably already imported in which case setting the env variable doesn't matter anymore. So we need to make sure it happens before the import of bbs library

Even if we do this at the very top of the Agent class (before importing any of the containers)? I'm OK with anything tho, but this seems like a more evident place. I just want to avoid forgetting this in the future because it's being done in a deeply nested file.

Anyhow, will do it right before the import for now. We can change it in a follow up PR :)

[TimoGlastra]

Maybe the index.ts file is a better suited place? That's the entry point of the framework. I'm almost certain the agent class won't work

[karimStekelenburg]

Ok, I ran some local tests. I ran it on Ubuntu hoping to reproduce the error, and on macOs (M1) for future reference. On both platforms all tests succeeded. This is going to be fun!

Ubuntu

Specs

Results for Node v14.19.3

Results for Node v16.16.0

Results for Node v17.9.1

Results for Node v18.5.0

macOs (M1)

Specs

Results for Node v14.20.0

Results for Node v16.16.0

Results for Node v17.9.1

Results for Node v18.5.0

[TimoGlastra]

Ok, I ran some local tests. I ran it on Ubuntu hoping to reproduce the error, and on macOs (M1) for future reference. On both platforms all tests succeeded. This is going to be fun!

Ubuntu

Specs

Results for Node v14.19.3

Results for Node v16.16.0

Results for Node v17.9.1

Results for Node v18.5.0

macOs (M1)

Specs

Results for Node v14.20.0

Results for Node v16.16.0

Results for Node v17.9.1

Results for Node v18.5.0

But isn't that expected with WASM? We're having issues with the node js bbs package Or do I mistunderstand the issue?

[karimStekelenburg]

Since we have a dedicated issue for the CI failure now, can we merge this in? This problem seems to be beyond the scope of this PR alone.