Skip to content
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: Create & Store Presentation #794

Merged

Conversation

CryptoKnightIOG
Copy link
Contributor

@CryptoKnightIOG CryptoKnightIOG commented Nov 20, 2023

Overview

Fixes https://input-output.atlassian.net/browse/ATL-6019

Checklist

My PR contains...

  • No code changes (changes to documentation, CI, metadata, etc.)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes
  • are not breaking changes
  • If yes to above: I have updated the documentation accordingly

Documentation

  • My changes do not require a change to the project documentation
  • My changes require a change to the project documentation
  • If yes to above: I have updated the documentation accordingly

Tests

  • My changes can not or do not need to be tested
  • My changes can and should be tested by unit and/or integration tests
  • If yes to above: I have added tests to cover my changes
  • If yes to above: I have taken care to cover edge cases in my tests

Copy link
Contributor

github-actions bot commented Nov 20, 2023

Integration Test Results

11 files  ±0  11 suites  ±0   2s ⏱️ ±0s
24 tests ±0  24 ✔️ ±0  0 💤 ±0  0 ±0 
25 runs  ±0  25 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit ebae28a. ± Comparison against base commit 5376b74.

♻️ This comment has been updated with latest results.

Copy link
Contributor

Atala PRISM Test Results

  87 files  ±0    87 suites  ±0   21m 19s ⏱️ -9s
723 tests +1  715 ✔️ +1  8 💤 ±0  0 ±0 
729 runs  +1  721 ✔️ +1  8 💤 ±0  0 ±0 

Results for commit 911e2e6. ± Comparison against base commit 7e18b6a.

@CryptoKnightIOG CryptoKnightIOG changed the title Atl 6019 create store presentation 2 feat: Create & Store Presentation Nov 20, 2023
@CryptoKnightIOG CryptoKnightIOG force-pushed the ATL-6019-Create-Store-Presentation-2 branch 2 times, most recently from 474c547 to c4154df Compare November 20, 2023 14:39
Copy link
Contributor

github-actions bot commented Nov 20, 2023

Unit Test Results

  87 files  ±0    87 suites  ±0   21m 29s ⏱️ -6s
723 tests +1  715 ✔️ +1  8 💤 ±0  0 ±0 
729 runs  +1  721 ✔️ +1  8 💤 ±0  0 ±0 

Results for commit ebae28a. ± Comparison against base commit 5376b74.

♻️ This comment has been updated with latest results.

def createPresentation(
presentationRequest: PresentationRequest,
credentials: Seq[Credential],
credentialRequests: Seq[CredentialAndRequestedAttributesPredicates],
Copy link
Member

Choose a reason for hiding this comment

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

would it be a good naming:
presentationRequest: AnonCredsPresentationRequest?

@@ -159,8 +163,17 @@ object AnoncredLib {
)
}

// TODO FIX
Copy link
Member

Choose a reason for hiding this comment

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

@CryptoKnightIOG, can you create a test for this issue and a ticket in the jira or add any other documentation that describes this issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added by Fabio. Will follow up with him.

)

object CredentialAndRequestedAttributesPredicates {
given Conversion[CredentialAndRequestedAttributesPredicates, UniffiCredentialRequests] with {
Copy link
Member

Choose a reason for hiding this comment

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

in my personal opinion a good name for CredentialAndRequestedAttributesPredicates is CredentialRequests or AnonCredsV1.CredentialRequests (if we can isolate the types the corresponds to the V1)

for {
maybeRecord <- presentationRepository
.getPresentationRecord(recordId)
.mapError(RepositoryError.apply)
Copy link
Member

Choose a reason for hiding this comment

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

I think, it's a good practice to call logError as well before mapError, so we can track what exactly is wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

The best way to handle this should defined in the ADR part of the Error Management value brief. I'll add this specific case - call to mapError() - to the list 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted

Comment on lines 452 to 457
schemaMap <-
ZIO
.collectAll(schemaIds.map { schemaId =>
for {
uri <- ZIO.attempt(new URI(schemaId)).mapError(e => UnexpectedError(e.getMessage))
content <- uriDereferencer.dereference(uri).mapError(e => UnexpectedError(e.error))
vcSchema <- parseCredentialSchema(content).mapError(e => UnexpectedError(e.message))
anoncredSchema <- AnoncredSchemaSerDesV1.schemaSerDes
.deserialize(vcSchema.schema)
.mapError(e => UnexpectedError(e.error))
anoncredLibSchema =
SchemaDef(
schemaId,
anoncredSchema.version,
anoncredSchema.attrNames,
anoncredSchema.issuerId
)
} yield (schemaId, anoncredLibSchema)
})
.map(_.toMap)
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend extracting this logic into the method. Something like resolveSchema(schemaId)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

).toEither
)
.mapError((t: Throwable) => AnoncredPresentationCreationError(t))
} yield presentation
}
Copy link
Member

Choose a reason for hiding this comment

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

the method is tooooo long :)
try to refactor it to a sequence of smaller functions next time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noted

): ZIO[WalletAccessContext, PresentationError, PresentationRecord] = {
val anoncredPresentationRequestV1 = AnoncredPresentationRequestV1(
requested_attributes = Map(
"sex" -> AnoncredRequestedAttributeV1(
Copy link
Member

Choose a reason for hiding this comment

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

Probably, a good idea for refactoring is to create a package or object AnoncredsV1 and all corresponding domain classes. RequestedAttribute etc.

Copy link
Contributor Author

@CryptoKnightIOG CryptoKnightIOG Nov 21, 2023

Choose a reason for hiding this comment

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

Yep good idea

Copy link
Member

@yshyn-iohk yshyn-iohk left a comment

Choose a reason for hiding this comment

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

Good job, @CryptoKnightIOG! Keep codding 👍

mediaType = Some("prism/jwt")
presentation <-
credentialFormat match {
case CredentialFormat.JWT =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to what we did for the IssueBackgroundJobs class, I propose to move the logic for each case to the PresentationService implementation. It initially leaked to the background job class because Pollux had no access to the ManagedDIDService, which is now solved.

Note that it implies a bit of refactoring (also at the unit tests level!), so we could handle this as a tech-debt item if we don't want to hamper the delivery of this verification Epic 👍

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, I will do a refactor. In the end, because there are patterns forming.

// TODO FIX
// [info] uniffi.anoncreds.AnoncredsException$CreatePresentationException: Create Presentation: Error: Error: Invalid structure
// [info] Caused by: Predicate is not satisfied

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need any help with those errors (e.g. from Fabio)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bvoiturier these were added by Fabio. Could be great if someone can have a look at them.

for {
maybeRecord <- presentationRepository
.getPresentationRecord(recordId)
.mapError(RepositoryError.apply)
Copy link
Contributor

Choose a reason for hiding this comment

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

The best way to handle this should defined in the ADR part of the Error Management value brief. I'll add this specific case - call to mapError() - to the list 👍

@CryptoKnightIOG CryptoKnightIOG force-pushed the ATL-6019-Create-Store-Presentation-2 branch from 0ea2a88 to 62d3dee Compare November 22, 2023 13:11
Signed-off-by: Bassam Riman <[email protected]>
@CryptoKnightIOG CryptoKnightIOG force-pushed the ATL-6019-Create-Store-Presentation-2 branch from 62d3dee to ebae28a Compare November 22, 2023 16:20
Copy link

sonarcloud bot commented Nov 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@CryptoKnightIOG CryptoKnightIOG merged commit 1bf9287 into feat/ZKP-verification Nov 22, 2023
12 checks passed
@CryptoKnightIOG CryptoKnightIOG deleted the ATL-6019-Create-Store-Presentation-2 branch November 22, 2023 16:41
CryptoKnightIOG added a commit that referenced this pull request Nov 30, 2023
CryptoKnightIOG added a commit that referenced this pull request Dec 19, 2023
CryptoKnightIOG added a commit that referenced this pull request Dec 20, 2023
CryptoKnightIOG added a commit that referenced this pull request Jan 3, 2024
CryptoKnightIOG added a commit that referenced this pull request Jan 24, 2024
CryptoKnightIOG added a commit that referenced this pull request Feb 3, 2024
CryptoKnightIOG added a commit that referenced this pull request Feb 9, 2024
CryptoKnightIOG added a commit that referenced this pull request Feb 9, 2024
CryptoKnightIOG added a commit that referenced this pull request Feb 18, 2024
CryptoKnightIOG added a commit that referenced this pull request Feb 23, 2024
CryptoKnightIOG added a commit that referenced this pull request Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants