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: Default Backend API to Array Of Credential Schema #1366

Merged
merged 3 commits into from
Sep 26, 2024
Merged

Conversation

CryptoKnightIOG
Copy link
Contributor

Copy link
Contributor

github-actions bot commented Sep 18, 2024

Unit Test Results

101 files  ±0  101 suites  ±0   20m 16s ⏱️ +43s
869 tests ±0  861 ✅ ±0  8 💤 ±0  0 ❌ ±0 
876 runs  ±0  868 ✅ ±0  8 💤 ±0  0 ❌ ±0 

Results for commit 15c9f33. ± Comparison against base commit e532ba6.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Sep 18, 2024

Coverage Status

coverage: 48.532% (-0.03%) from 48.558%
when pulling 15c9f33 on ATL-7775
into e532ba6 on main.

@@ -69,7 +65,7 @@ class IssueControllerImpl(
pairwiseHolderDID = offerContext.pairwiseHolderDID,
kidIssuer = request.issuingKid,
thid = DidCommID(),
maybeSchemaId = request.schemaId,
maybeSchemaIds = Applicative[Option].map2(request.schemaIds, request.schemaId.map(List(_)))(_ ++ _),
Copy link
Member

Choose a reason for hiding this comment

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

@CryptoKnightIOG, two fields might contain schemaId: schemaId and schemaIds.
Remember, I recommended splitting VCDM 1.1 and 2.0 at the domain level, but I see that the API also should be segregated.
Otherwise, the result API is going to be overloaded and slightly weird.

Copy link
Member

Choose a reason for hiding this comment

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

If you are still going to go this way, use the same REST API for both models. I recommend deleting schemaIds and allowing schemaId to be either String or List[String].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it this PR #1368

@CryptoKnightIOG CryptoKnightIOG enabled auto-merge (squash) September 26, 2024 17:02
Copy link
Contributor

Integration Test Results

20 files  ±0  20 suites  ±0   2s ⏱️ ±0s
45 tests ±0  45 ✅ ±0  0 💤 ±0  0 ❌ ±0 
71 runs  ±0  71 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 15c9f33. ± Comparison against base commit e532ba6.

@CryptoKnightIOG CryptoKnightIOG merged commit 693dcc4 into main Sep 26, 2024
13 checks passed
@CryptoKnightIOG CryptoKnightIOG deleted the ATL-7775 branch September 26, 2024 17:07
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.

3 participants