-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: auto accept proofs #367
feat: auto accept proofs #367
Conversation
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #367 +/- ##
==========================================
+ Coverage 86.42% 86.52% +0.10%
==========================================
Files 234 236 +2
Lines 4803 4891 +88
Branches 750 773 +23
==========================================
+ Hits 4151 4232 +81
- Misses 652 659 +7
Continue to review full report at Codecov.
|
Tests on Github are failing while local tests work. Will resolve this asap. |
Signed-off-by: Berend Sliedrecht <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice work @blu3beri!
…roof Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
…roof Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
…roof Signed-off-by: Berend Sliedrecht <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall correctly, we're waiting for additional followup to move the relevant validation changes into the service regardless of the automation configuration? Or am I remembering incorrectly here?
My understanding was first to merge this. And in a seprate PR abstract the functionality, add documentation stating the types of autoAccept. I think for now this will suffice, but I would not mind to change that stuff and merge it later. |
proofRecord: ProofRecord, | ||
messageContext: HandlerInboundMessage<RequestPresentationHandler> | ||
) { | ||
const indyProofRequest = proofRecord.requestMessage?.indyProofRequest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the handler kind of Indy specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix this when we support multiple credential types.
import { setupProofsTest, waitForProofRecord } from './helpers' | ||
import testLogger from './logger' | ||
|
||
describe('Auto accept present proof', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we have tests also for accept never?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we're testing just a happy path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I assumed the default is Never
, the proofs.test.ts will error if it does any automatic response. I could add the
never` clause as a test, but it would feel a bit redundant.
|
||
import { AgentConfig } from '../../agent/AgentConfig' | ||
|
||
import { AutoAcceptProof } from './ProofAutoAcceptType' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have two types, like AutoAcceptProof
for the holder and AutoAcceptProofRequest
request for the issuer. Similarly with credentials. But that's of course more of a future improvement idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason for this? Maybe it allows for some use cases that I don't know of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see 2 reasons:
The first is expressiveness. AutoAcceptProof
means auto acceptance proof from the verifier point of view but also auto acceptance of the proof request from the holder's perspective. Even now, I'm not sure if I wrote it correctly.
The second is a potential use case when someone uses an agent for more than one role. Let's say I want to accept proofs as a verifier but don't want to accept proof requests as a holder.
Merging. Additional improvements can be made in follow-up PRs |
This adds automatically accepting of proof requests. This functions the same as #336 to stay as consistent as possible, yet they work completely seperate to eachother.
It allows for an agent- and recordwide configuration where the record configuration has priority.
Three states are allowed to be set:
always
: accept any proof request/proposal no matter what the content might be.contentApproved
: accept any proof request/proposal as long as none of the content changes and it has to be accepted manually at least once.never
(DEFAULT) Does not automate anything and will just behave the same as before these changes.Note: I am not sure whether we want to mention this as a feature in the README, so if there is any problem with this I will remove it.