feat(proofs): proof negotiation #1131
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1131 +/- ##
==========================================
+ Coverage 88.55% 88.77% +0.21%
==========================================
Files 706 706
Lines 16468 16495 +27
Branches 2670 2672 +2
==========================================
+ Hits 14584 14643 +59
+ Misses 1877 1845 -32
Partials 7 7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
This is very interesting. But I'm curious about the usage of this negotiation in a loop that involves more than a single message of each type. I mean: is it possible for a proof exchange to follow the path: propose-presentation -> request-presentation -> propose-presentation -> request-presentation -> presentation? If so, what happens when we do agent.proofs.findRequestMessage or agent.proofs.findProposalMessage? As it is supposed to be a single message, would it be updated to the latest one?
|
|
||
| if (!proofRecord.connectionId) { | ||
| throw new AriesFrameworkError( | ||
| `No connectionId found for credential record '${proofRecord.id}'. Connection-less issuance does not support presentation proposal or negotiation.` |
There was a problem hiding this comment.
I think part of this message was a copy-paste from CredentialsApi. It should say 'proof exchange' instead of 'credential' and maybe Connection-less presentation (not issuance).
There was a problem hiding this comment.
Fixed. I copied it from the acceptProposal from the same file, so fixed it there as well.
|
|
||
| if (!proofRecord.connectionId) { | ||
| throw new AriesFrameworkError( | ||
| `No connectionId found for credential record '${proofRecord.id}'. Connection-less issuance does not support negotiation.` |
| } | ||
| const { message } = await service.createRequestAsResponse(this.agentContext, requestOptions) | ||
|
|
||
| const outbound = createOutboundMessage(connection, message) |
There was a problem hiding this comment.
createOutboundMessage has been replaced by the class OutboundMessageContext
Przytua
force-pushed
the
feature/proof-negotiation
branch
from
669b2c1 to
497c3e3
Compare
I assume (according to this diagram: https://github.com/hyperledger/aries-rfcs/blob/main/features/0037-present-proof/README.md#choreography-diagram) that it should be possible. With a small patch of AFJ and current implementation of acapy we were able to achieve
Yes. I added some more tests to make sure it returns latest message. |
Przytua
force-pushed
the
feature/proof-negotiation
branch
2 times, most recently
from
90527f8 to
7b6a221
Compare
| @@ -56,10 +59,12 @@ import { ProofRepository } from './repository/ProofRepository' | |||
| export interface ProofsApi<PFs extends ProofFormat[], PSs extends ProofService<PFs>[]> { | |||
| // Proposal methods | |||
| proposeProof(options: ProposeProofOptions<PFs, PSs>): Promise<ProofExchangeRecord> | |||
| proposeProofAsResponse(options: ProposeProofAsResponseOptions<PFs>): Promise<ProofExchangeRecord> | |||
There was a problem hiding this comment.
For the credentials module we use negotiateRequest and negotiateProposal
There was a problem hiding this comment.
Renamed to negotiateRequest and negotiateProposal accordingly in the ProofsApi. Functions in services were already implemented, and I just used them in the api. They're named createProposalAsResponse and createRequestAsResponse. Do you want me to rename them as well or is that fine?
There was a problem hiding this comment.
We probably want to rename them, but it's more for a different pr I think.
Przytua
force-pushed
the
feature/proof-negotiation
branch
2 times, most recently
from
1188d03 to
930702d
Compare
| @@ -0,0 +1,444 @@ | |||
| import type { Agent } from '../../../../../agent/Agent' | |||
There was a problem hiding this comment.
It seems there's thests both in the top level tests and also in the protocol specific directories. What's the difference between them?
There was a problem hiding this comment.
Yes, I added tests in the core/tests first (as it seemed to me more of a e2e test), and later I noticed, that there are test cases in the __tests__ folder of the protocol, that are covering similar scenarios, so added there as well. But true, they're almost no different
There was a problem hiding this comment.
Can we remove one of them in that case to keep this tidy? I think my preference would go to keep them in the credentials module folder
There was a problem hiding this comment.
I assume you mean proofs module folder. Anyway removed those in core/tests.
There was a problem hiding this comment.
Yeah I think this comes from the proofs module, where its root tests directory has some unit tests but protocol/vx includes e2e tests.
I agree on merge these tests and leaving one of each in modules/proofs/__tests__. They can be named 'xxx.e2e.test.ts' like it happens in other modules E2E tests. Later we should open a new PR to harmonize this structure.
There was a problem hiding this comment.
I assume you mean proofs module folder
yes 😬
|
sorry forgot about this one. Code LGTM, just one qeustion about the tests |
Przytua
force-pushed
the
feature/proof-negotiation
branch
2 times, most recently
from
37dddd5 to
0613f82
Compare
|
@Przytua if you can update the branch with main I can merge |
Signed-off-by: Łukasz Przytuła <[email protected]>
Przytua
force-pushed
the
feature/proof-negotiation
branch
from
0613f82 to
6c243ed
Compare
@TimoGlastra updated |
Signed-off-by: Łukasz Przytuła [email protected]
Allow proof negotiation by responding to presentation proposal with new presentation request, and responding to presentation request with new presentation proposal