hyperledger-archives / aries-framework-go

Hyperledger Aries Framework Go provides packages for building Agent / DIDComm services.
https://wiki.hyperledger.org/display/ARIES/aries-framework-go
Apache License 2.0
241 stars 158 forks source link

Switch to using mathlib #3574

Closed ale-linux closed 1 year ago

ale-linux commented 1 year ago

Title: Use mathlib as the implementation for elliptic curve operations.

Description: This PR is an implementation of this proposal.

Summary:

This PR changes the dependency used to handle the operations on elliptic curves needed by the BBS+ implementation. Instead of directly using the kilic implementation. we recommend switching to mathlib: mathlib is a module that exposes a common set of API backed by a number of different libraries (amcl, ConsenSys/gnark-crypto and kilic). It currently supports the following curves: FP256BN, BN254, BLS12_377 and BLS12_381 (the latter in two different variants, standard and BBS compliant). mathlib is already being used by fabric, the idemix implementation used by fabric, the fabric token sdk and the fabric smart client.

codecov[bot] commented 1 year ago

Codecov Report

Merging #3574 (e4cd201) into main (20c4d4b) will increase coverage by 0.20%. The diff coverage is 99.20%.

:exclamation: Current head e4cd201 differs from pull request most recent head c6026db. Consider uploading reports for the commit c6026db to get more accurate results

@@            Coverage Diff             @@
##             main    #3574      +/-   ##
==========================================
+ Coverage   87.02%   87.23%   +0.20%     
==========================================
  Files         353      353              
  Lines       48666    48667       +1     
==========================================
+ Hits        42354    42457     +103     
+ Misses       4769     4672      -97     
+ Partials     1543     1538       -5     
Impacted Files Coverage Δ
...rypto/primitive/bbs12381g2pub/signature_message.go 100.00% <ø> (ø)
.../crypto/primitive/bbs12381g2pub/signature_proof.go 84.04% <97.43%> (ø)
...to/crypto/primitive/bbs12381g2pub/bbs12381g2pub.go 86.95% <100.00%> (-0.15%) :arrow_down:
...ent/kmscrypto/crypto/primitive/bbs12381g2pub/fr.go 100.00% <100.00%> (ø)
...t/kmscrypto/crypto/primitive/bbs12381g2pub/keys.go 91.66% <100.00%> (+6.06%) :arrow_up:
...ypto/primitive/bbs12381g2pub/proof_of_knowledge.go 97.98% <100.00%> (-0.10%) :arrow_down:
...crypto/crypto/primitive/bbs12381g2pub/signature.go 100.00% <100.00%> (ø)

... and 80 files with indirect coverage changes

ale-linux commented 1 year ago

@sudeshrshetty : as discussed last week, here's the PR and here's the associated proposal.

adecaro commented 1 year ago

Hi @sudeshrshetty , we would really appreciate your feedback on the PR. We think it can bring good value to the project overall. Please, let us know :)

sudeshrshetty commented 1 year ago

@DRK3 @fqutishat @Moopli please review this PR and provide your feedback. Let us know if it is going to impact anything we have implemented in TrustBloc.

sudeshrshetty commented 1 year ago

@ale-linux can you please squash your commits & re-push https://github.com/hyperledger/aries-framework-go/blob/main/.github/CONTRIBUTING.md#pull-request

ale-linux commented 1 year ago

Done - all tests run successfully on my node - unclear why they fail here.. maybe a flaky test?

ale-linux commented 1 year ago

Any news?

DRK3 commented 1 year ago

@sudeshrshetty I think @ale-linux is right, it's a flaky test issue. @Moopli is probably a bit more familiar with the tests, do you recognize those failures? If it's an ongoing flaky test issue, I think this PR is good to merge :)

ale-linux commented 1 year ago

@sudeshrshetty: I'm happy to discuss any further open points/necessary changes, but if you think it is ready, would you mind merging? Otherwise I need to keep rebasing/fixing conflicts with upstream...

ale-linux commented 1 year ago

...and the flaky test also passed, I think it's a sign that it's time to merge :wink:

ale-linux commented 1 year ago

another rebase and now the flakes strike again... :cry:

sudeshrshetty commented 1 year ago

Sorry, I am re-triggering these failed tests to make succeed CI, so that I can merge it.

ale-linux commented 1 year ago

Thanks much, @sudeshrshetty

ale-linux commented 1 year ago

@sudeshrshetty : All tests have passed now - thx!

ale-linux commented 1 year ago

Could we please merge now gents? @sudeshrshetty @fqutishat