Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

feat: CL Anoncreds tink primitives #3271

Merged
merged 1 commit into from
Jul 13, 2022
Merged

feat: CL Anoncreds tink primitives #3271

merged 1 commit into from
Jul 13, 2022

Conversation

ashcherbakov
Copy link
Contributor

@ashcherbakov ashcherbakov commented Jul 11, 2022 •
edited
Loading

Title:
CL Anoncreds tink primitives

Description:

  • This is the first PR to support CL Anoncreds in Aries-framework-go #180.
  • This PR addresses support of CL Anoncreds on the think crypto primitives level.
  • Next PRs will add support for CL crypto to the pkg/crypto/api API interface and pkg/kms (separate PRs for each).
  • CL Anoncreds support is based on the ursa-wrapper-go as an optional dependency (under the ursa build tag).
  • Please note, that ursa-wrapper-go required libursa lib to be installed, that's why ursa build tag has been introduced.

Summary:

  • Adds ursa-wrapper-go as an optional dependency (under the ursa build tag).
  • Adds tink crypto primitives and keys for the Issuance flow
  • Adds a separate make unit-test-ursa target
  • Adds a separate job in the CI pipeline to run unit tests with the ursa build tag. As libursa is required, the CI job for the ursa tag is run in a container (ghcr.io/hyperledger/ursa-wrapper-go/uwg-build for now) where libursa is already installed.

@ashcherbakov ashcherbakov marked this pull request as draft July 11, 2022 06:26
@ashcherbakov ashcherbakov force-pushed the cl-primitives-ursa branch 2 times, most recently from f663293 to d07dfab Compare July 11, 2022 06:35
@codecov
Copy link

codecov bot commented Jul 11, 2022 •
edited
Loading

Codecov Report

Merging #3271 (7c99059) into main (12e93a0) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 7c99059 differs from pull request most recent head 343be87. Consider uploading reports for the commit 343be87 to get more accurate results

@@            Coverage Diff             @@
##             main    #3271      +/-   ##
==========================================
+ Coverage   88.28%   88.32%   +0.04%     
==========================================
  Files         315      311       -4     
  Lines       42658    42398     -260     
==========================================
- Hits        37659    37448     -211     
+ Misses       3674     3638      -36     
+ Partials     1325     1312      -13
Impacted Files Coverage Δ
pkg/wallet/wallet.go 98.32% <0.00%> (-0.38%) ⬇️
pkg/client/vcwallet/client.go 100.00% <0.00%> (ø)
pkg/wallet/didcomm.go
pkg/didcomm/packer/legacy/anoncrypt/anoncrypt.go
pkg/didcomm/packer/legacy/anoncrypt/pack.go
pkg/didcomm/packer/legacy/anoncrypt/unpack.go
pkg/controller/command/vcwallet/command.go 95.66% <0.00%> (+2.99%) ⬆️

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 12e93a0...343be87. Read the comment docs.

baha-ai
baha-ai reviewed Jul 11, 2022
View reviewed changes
pkg/crypto/tinkcrypto/primitive/cl/api/issuer.go
Comment on lines +1 to +2
//go:build ursa
// +build ursa
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be helpful to have a make target for Ursa build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's in progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added make unit-test-ursa target.

baha-ai
baha-ai reviewed Jul 11, 2022
View reviewed changes
scripts/check_unit.sh Outdated
Comment on lines 62 to 64
#PKGS=$(go list -tags ursa github.com/hyperledger/aries-framework-go/pkg/... 2> /dev/null | grep -v /mocks | grep -v /aries-js-worker)
#$GO_TEST_CMD -tags ursa $PKGS -count=1 -race -coverprofile=profile.out -covermode=atomic -timeout=10m
#amend_coverage_file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#PKGS=$(go list -tags ursa github.com/hyperledger/aries-framework-go/pkg/... 2> /dev/null | grep -v /mocks | grep -v /aries-js-worker)
#$GO_TEST_CMD -tags ursa $PKGS -count=1 -race -coverprofile=profile.out -covermode=atomic -timeout=10m
#amend_coverage_file
PKGS=$(go list -tags ursa github.com/hyperledger/aries-framework-go/pkg/... 2> /dev/null | grep -v /mocks | grep -v /aries-js-worker)
$GO_TEST_CMD -tags ursa $PKGS -count=1 -race -coverprofile=profile.out -covermode=atomic -timeout=10m
amend_coverage_file

I don't mind adding ursa unit tests in the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a separate make unit-test-ursa target and a separate job in the CI pipeline to run unit tests with the ursa build tag.
There is a reason why it's not easily possible to get rid of ursa build tag and just run ursa/CL test as other unit tests: ursa-wrapper-go requires libursa to be installed. Current development process and CI unit tests are not based on Docker, so that it may be challenging to require libursa library to be installed for every developer.
CI job for the ursa tag is run in a container (ghcr.io/hyperledger/ursa-wrapper-go/uwg-build for now) where libursa is already installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the explanation

@baha-ai
Copy link
Contributor

baha-ai commented Jul 11, 2022

also another suggestion:
add a new target build file api_ursa.go in https://github.com/hyperledger/aries-framework-go/blob/main/pkg/kms/ to include CLIssuer and CLProver key KMS key types.

Eventually, it would be great to not limit this PR to an ursa build tag and have it part of afgo core once it's stable, for the benefit of everyone. (the less build tags, the better)

@ashcherbakov ashcherbakov changed the title CL Anoncreds tink primitives feat: CL Anoncreds tink primitives Jul 12, 2022
@ashcherbakov
Copy link
Contributor Author

add a new target build file api_ursa.go in https://github.com/hyperledger/aries-framework-go/blob/main/pkg/kms/ to include CLIssuer and CLProver key KMS key types.

Yes, we are planning to integrate CL crypto to the pkg/crypto/api API interface as well as pkg/kms. This PR is already not small, so our suggestion is to continue integration to the next layers in the next PRs (we've been already working on them).

Eventually, it would be great to not limit this PR to an ursa build tag and have it part of afgo core once it's stable, for the benefit of everyone. (the less build tags, the better)

The problem here is that libursa library needs to be installed for ursa-wrapper-go and CL anoncreds crypto. That's why we started with adding the ursa build tag to not break the current flows.
In the next PRs we are planning to have an interface for CL Anoncreds and two implementations: a real working one (with ursa tag on) and a stub one returning "Not Implemented" (with ursa tag off).
We can continue discussion and change the approach in the next PRs.

@ashcherbakov ashcherbakov marked this pull request as ready for review July 12, 2022 11:16
@ashcherbakov
Copy link
Contributor Author

BTW it seems there are some intermittent test failures in places not related to the PR changes (noticed for other PRs as well)...

@ashcherbakov ashcherbakov requested a review from baha-ai July 12, 2022 12:15
@baha-ai
Copy link
Contributor

baha-ai commented Jul 12, 2022

BTW it seems there are some intermittent test failures in places not related to the PR changes (noticed for other PRs as well)...

simply push an empty update to retrigger the build, the CI does this intermittently (we can't control GH Action runs)

@baha-ai
Copy link
Contributor

baha-ai commented Jul 12, 2022

it's bdd-tests that are now failing, you can push another empty update

@ashcherbakov ashcherbakov reopened this Jul 13, 2022
@ashcherbakov @fqutishat
CL Anoncreds tink primitives based on ursa-wrapper-go
343be87 
Signed-off-by: alexander.shcherbakov <alexander.shcherbakov@avast.com>
@fqutishat fqutishat force-pushed the cl-primitives-ursa branch from 3280407 to 343be87 Compare July 13, 2022 06:12
@ashcherbakov ashcherbakov reopened this Jul 13, 2022
@ashcherbakov ashcherbakov reopened this Jul 13, 2022
@ashcherbakov
Copy link
Contributor Author

@fqutishat can you please merge the PR?

@fqutishat fqutishat merged commit d5d31b7 into hyperledger-archives:main Jul 13, 2022
Abdulbois pushed a commit to Abdulbois/aries-framework-go that referenced this pull request Jul 16, 2022
@ashcherbakov @Abdulbois
CL Anoncreds tink primitives based on ursa-wrapper-go (hyperledger-ar…
891248e 
…chives#3271)

Signed-off-by: alexander.shcherbakov <alexander.shcherbakov@avast.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@baha-ai baha-ai baha-ai approved these changes

@fqutishat fqutishat fqutishat approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ashcherbakov @baha-ai @fqutishat