Anoncreds create credential #3369
Conversation
|
Frustrating — sorry that got messy. The orginal AnonCreds implementation (in Indy) used “holder_did”, but semantically, that was wrong — the “did” had nothing to do with the operation. Clients were using the “DIDComm DID” of the relationship between the holder and issuer — but it was not checked and could be anything. We looked into the code, and it was determined that the purpose of the data was simply to add entropy into the operation. A very minor change, but not everyone got the memo, and all it does is create friction. Likely the best way to handle it is for the issuer to accept either, and pass it on to AnonCreds with what it expects. Painful - sorry about that. The “clarification” of the purpose does not make up for the interop problems this has caused. |
|
Need to look into failing tests... Thought I had tested this. (edit) Seems to be some compatibility issues with indy issuer to anoncreds holder. I'll get back to this in a bit. |
jamshale
force-pushed
the
anoncreds-issuance
branch
2 times, most recently
from
b60f20f to
e08c47f
Compare
jamshale
force-pushed
the
anoncreds-issuance
branch
from
e08c47f to
1afdd62
Compare
|
I think we can ignore the sonarcloud report. Most of the duplication is because of the extension of the registry base class and there's not really much to cover with unit tests, and it's covered by the scenario test. |
|
FWIW, i also ran into compatibility issues when testing VCX against cheqd's acapy fork/branch. The issue was that VCX (using anoncreds-rs) was creating cred request attachments that had Looks like this is the more permanent fix - cheers! question; is it still necessary to include both Maybe that's too aggressive, as i don't think the anoncreds spec explicitly denies usage of |
yeah i ran into this too. here's the source where all the validation rules are applied: https://github.com/hyperledger/anoncreds-rs/blob/main/src/data_types/cred_request.rs#L27 different behaviours depending on whether or not the cred-def-id is qualified ("legacy") or not |
jamshale
force-pushed
the
anoncreds-issuance
branch
2 times, most recently
from
8f250e6 to
5460ff5
Compare
jamshale
force-pushed
the
anoncreds-issuance
branch
from
9cc0f0a to
770df2d
Compare
|
These changes will get hit by the anoncreds scenario, right? |
|
Yes. The anocnreds scenario test is testing askar/indy (issuer) --> askar-anoncreds (holder) and askar-anoncreds (issuer) --> askar/indy (holder). And the verifier scenario. The anoncreds holder now uses entropy instead of prover_did, and the indy credx holder still uses prover_did. The only issue would be an old version of an indy issuer, issuing to a new anoncreds holder. In this case it wouldn't know what to do with the |
|
If it helps — the “prover_did” property that would come from an Indy request can be used as the value of “entropy” — they are the same value. |
|
Updated that last comment to be “prover_did” — |
|
@jamshale we need to |
👍 Ok. Thanks for pointing this out. I had tried to run through the presentation protocol as well with your branch but was having other issues so I never got through it. This looks like an obvious place to apply the method. Let me know if you see any others please. |
jamshale
force-pushed
the
anoncreds-issuance
branch
2 times, most recently
from
3e1fc6d to
38c5811
Compare
| @@ -1229,3 +1231,18 @@ async def txn_submit( | |||
| ) | |||
| except LedgerError as err: | |||
| raise AnonCredsRegistrationError(err.roll_up) from err | |||
|
|
|||
| async def get_schema_info_by_id(self, schema_id: str) -> AnoncredsSchemaInfo: | |||
There was a problem hiding this comment.
@jamshale I think we should use the existing methods instead of these new _info methods
- get_schema
- get_credential_definition
It's not guaranteed that the name, version or other attributes required are within the identifiers for every did method. For cheqd we have to make a ledger query inorder to get them. The above functions already handle this. I created a PR for the fix here
There was a problem hiding this comment.
That had been my original thought, but this is used only by the holder. The holder shouldn't necessarily need to go to the ledger to get the issuer id and schema version, etc., if the information is available in the schema id and cred def id itself.
For cheqd this get_schema_info_by_id method could call the the get_schema method and extract the correct info for the holder if it's required that the holder needs to contact the ledger to get the required info.
I'm not sure what did methods will be able to put the required info into the schema and cred_def id's themselves.
It'd be nice to get some feedback from others on this...
There was a problem hiding this comment.
I could change the handler method implementations like you did, as these are done by the verifier and they should be able to contact the ledger. My comment more applies to the usage in https://github.com/openwallet-foundation/acapy/blob/main/acapy_agent/anoncreds/holder.py#L224 when the holder saves the credential.
I could remove the get_cred_def_info_by_id method by changing the handler.
There was a problem hiding this comment.
I updated this to only use the get_schema_info_by_id method with the holder when saving the credential.
There was a problem hiding this comment.
In that case the profile should be one of the parameters within get_schema_info_by_id.
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
jamshale
force-pushed
the
anoncreds-issuance
branch
from
38c5811 to
3581b01
Compare
Signed-off-by: jamshale <jamiehalebc@gmail.com>
| @@ -175,7 +176,7 @@ async def create_pres( | |||
| async def receive_pres(self, message: V20Pres, pres_ex_record: V20PresExRecord): | |||
| """Receive a presentation and check for presented values vs. proposal request.""" | |||
|
|
|||
| def _check_proof_vs_proposal(): | |||
There was a problem hiding this comment.
This class should really get some refactoring but don't want to address it in this PR.
Signed-off-by: jamshale <jamiehalebc@gmail.com>
|
When
cheqdwas attempting anoncreds issuance they were getting aentropy requirederror when using the anoncreds library to create the credential. After some research it looked like we should be using the entropy parameter, but were using theprover_didparameter. https://hyperledger.github.io/anoncreds-spec/#constructing-the-credential-requestI don't know why this works for an indy credential request and not the cheqd. Maybe something internally in the anocnreds library. But it looks like using the holder_did value in the entropy works and I tested with indy and cheqd and issuance succeeds.
Then noticed some indy parsing when storing the credential that shouldn't be in the anoncreds holder. I couldn't figure out why we would need these extra tags so I removed them. If we did need them then the parsing should go into the actual registry.