Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Idemix Update #363

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Idemix Update #363

merged 1 commit into from
Aug 24, 2023

Conversation

adecaro
Copy link
Contributor

@adecaro adecaro commented Jun 29, 2023

Type of change

  • Improvement (improvement to code, performance, etc)

Description

This PR update the Idemix dependency to its latest version that is used by the Fabric Token SDK too.
Now, the SignerConfig struct also exports the RevocationHandle that is used by the Fabric Token SDK for audit purpose.
The way the revocation handle is encoded has changed to align to what the Idemix library expects.
The unit-tests have been updated.

Release Note

The changes to the encoding of the revocation handle shouldn't impact anyway given that Fabric is not currently using it.

@adecaro adecaro requested a review from a team as a code owner June 29, 2023 15:00
@adecaro
Copy link
Contributor Author

adecaro commented Jul 11, 2023

@denyeart @jkneubuh, please, could you help me here? It is not clear to me what's the issue. Thanks

@denyeart
Copy link
Contributor

@adecaro Sorry for the delay, just getting back this week.

It looks like the issue is that an updated cobra got vendored in which adds a new completion option for the CA server and client commands, e.g.:

     Available Commands:
       completion  Generate the autocompletion script for the specified shell
       help        Help about any command
       init        Initialize the fabric-ca server
       start       Start the fabric-ca server

This results in the new completion option showing up in the generated docs at:
docs/source/clientcli.rst
docs/source/servercli.rst

The error indicates that the generated docs don't match the existing docs. To fix the issue, you would need to do one of two things:

  1. run make docs to regenerate the docs and include the above files in your PR.
  2. suppress the new completion option by following the instructions at https://www.reddit.com/r/golang/comments/11evntz/how_to_disable_cobra_cli_default_commands/

In my opinion, the completion feature is decent enough to keep. See description of it at https://blog.devgenius.io/shell-completion-with-cobra-and-go-c8368074d8f7.

@denyeart
Copy link
Contributor

I've also opened a PR to clarify the confusing CI error message - #365

@adecaro
Copy link
Contributor Author

adecaro commented Jul 27, 2023

cool, thanks @denyeart. I'll run make docs and update the PR :)

@adecaro adecaro requested a review from a team as a code owner July 27, 2023 06:27
@adecaro
Copy link
Contributor Author

adecaro commented Aug 21, 2023

Hi @denyeart , anything missing that I need to address to get the PR merged? Thanks much :)

Copy link
Contributor

@denyeart denyeart left a comment

Choose a reason for hiding this comment

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

I don't completely understand the revocation handle changes, but I'll take your word for it.

So I've just added some nits from Go perspective.

Also, we typically prefer the PR author to squash their own commits into a single commit for a cleaner history upon merge.

Comment on lines 23 to 26
// RevocationHandle is the handle used to single out this credential and determine its revocation status
RevocationHandle string `protobuf:"bytes,7,opt,name=revocation_handle,json=revocationHandle,proto3" json:"revocation_handle,omitempty"`
// CurveID specifies the name of the Idemix curve to use, defaults to 'amcl.Fp256bn'
CurveID string `protobuf:"bytes,7,opt,name=curve_id,json=curveID" json:"curveID,omitempty"`
CurveID string `protobuf:"bytes,8,opt,name=curve_id,json=curveID" json:"curveID,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

New properties should always be added at the end to ensure compatibility (even if you expect there will be no functional impact I still think it is good practice for clean change history).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense

lib/client.go Outdated
@@ -33,13 +23,23 @@ import (
idemixcred "github.com/hyperledger/fabric-ca/lib/client/credential/idemix"
x509cred "github.com/hyperledger/fabric-ca/lib/client/credential/x509"
cidemix "github.com/hyperledger/fabric-ca/lib/common/idemix"
idemix2 "github.com/hyperledger/fabric-ca/lib/server/idemix"
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to have more expressive names than idemix and idemix2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sidemix should do the job

Copy link
Contributor Author

@adecaro adecaro left a comment

Choose a reason for hiding this comment

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

Comments addressed

- prepare the revocation handle as expected by the idemix lib
- idemix: export revocation handle in SignerConfig
- update idemix and mathlib dep
- add logs to idemix issuer
- doc update

Signed-off-by: Angelo De Caro <angelo.decaro@gmail.com>
@adecaro
Copy link
Contributor Author

adecaro commented Aug 23, 2023

Hi @denyeart , thanks for the review. I have addressed your comments :)

@adecaro adecaro requested a review from denyeart August 23, 2023 14:01
@denyeart denyeart merged commit d575f40 into hyperledger:main Aug 24, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants