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(pollux): update storage for static resources to be wrapped in an envelope #1283

Merged

Conversation

shotexa
Copy link
Contributor

@shotexa shotexa commented Aug 9, 2024

Description:

Summarize the changes you're submitting in a few sentences, including Jira ticket ATL-xxxx if applicable.
Link to any discussion, related issues and bug reports to give the context to help the reviewer understand the PR.

Alternatives Considered (optional):

Link to existing ADR (Architecture Decision Record), if any. If relevant, describe other approaches explored and the selected approach. Documenting why the methods were not selected will create a knowledge base for future reference, helping prevent others from revisiting less optimal ideas.

Checklist:

  • My PR follows the contribution guidelines of this project
  • My PR is free of third-party dependencies that don't comply with the Allowlist
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked the PR title to follow the conventional commit specification

@shotexa shotexa requested a review from a team as a code owner August 9, 2024 21:23
@shotexa shotexa marked this pull request as draft August 9, 2024 21:27
Copy link
Contributor

github-actions bot commented Aug 19, 2024

Unit Test Results

0 files   -  97  0 suites   - 97   0s ⏱️ - 22m 5s
0 tests  - 827  0 ✅  - 819  0 💤  - 8  0 ❌ ±0 
0 runs   - 834  0 ✅  - 826  0 💤  - 8  0 ❌ ±0 

Results for commit 1e3f2f2. ± Comparison against base commit 0a7579e.

♻️ This comment has been updated with latest results.

ErrorResponse,
CredentialSchemaResponse,
CredentialSchemaResponse | CredentialSchemaDidUrlResponse,
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to unpack the DIDURLResponse and use CredentialSchemaResponse everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you mean by unpack? I did not get it

}

override def getSchemaByGuid(guid: UUID)(implicit
override def getSchemaByGuid(config: AppConfig, guid: UUID)(implicit
Copy link
Member

Choose a reason for hiding this comment

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

Passing an entire AppConfig object is a code smell.
From a long-term perspective, it creates a coupling between the application conf and the schema API.
Passing the required parameters (or the object with these parameters) to the function would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense

@yshyn-iohk
Copy link
Member

yshyn-iohk commented Aug 22, 2024

@shotexa, good progress with schemas.
From the architectural point of view, the open-close principle would be better if all specifications for the schema CRUD operation were split into different endpoints, allowing for better modularization.

What do I propose:

  1. For a W3C specification, we use the W3C VerifiableCredentialSchema spec. I would use the path /cloud-agent/vc-json-schema/.....
  • for our VDR variation, I would use /cloud-agent/prism-vc-json-schema/... - pointing out that request/response is specific to our DID Linked resources specification
  1. for AnonCreds we support two methods

Having this segregation will make everything simple for usage and adoption. Business logic will be straightforward, and unit/e2e tests will be much simpler to write. The maintainability of this code will also be much better.

NOTE: The naming convention for the paths can be changed; the main idea is to segregate different specs for the different routes.

CC: @bvoiturier, @mineme0110

@bvoiturier
Copy link
Contributor

@yshyn-iohk Regarding the above comment, I think we are mixing different concepts here. From my point of view, the schema formats we support (JSON and AnonCreds) and their CRUD endpoints are orthogonal to the retrieval methods we want to support (direct http request to the schema or resolution through DID URL that contains a 'service' entry indicating the schema URL to fetch).

So, if deemed necessary, we could split schema CRUD operations into two sets of endpoints - one for JSON and one for AnonCreds - but I don't understand the reasoning behind having additional endpoints for a "VDR variation" or for "HTTP/PrismDID AnonCreds methods" 🤔

@shotexa shotexa self-assigned this Aug 22, 2024
@shotexa
Copy link
Contributor Author

shotexa commented Aug 22, 2024

  • HTTP AnonCreds Method

Yeah, so from what I understood, @yshyn-iohk is suggesting having 2 sets of endpoints for every schema operation, just like now I have 2 endpoints for creation, one for creating schema that is resolvable via HTTP, and one that is resolvable via DID URL, @yshyn-iohk you are suggesting to have all endpoints (get, update, get many) to have separate for HTTP URLs and DID URLs. do I understand this correctly?

Right now I only have the creation endpoint split into two, get endpoint for example, it might return schema as normal (like now), or wrapped in an envelope (as it was created to be resolvable via DID URL)

@bvoiturier

@yshyn-iohk
Copy link
Member

I'm suggesting to isolate the schema's APIs by type and by spec
w3c vc schema or the full name Verifiable Credentials JSON Schema - is a type
anoncreds schema is another type.
At the same time, there are a lot of specs for the CRUD operation: plain HTTP, did:prism, did:indy , etc
Having ALL type and all METHODS in under the same path will blow up sooner or later.

@yshyn-iohk
Copy link
Member

@bvoiturier, regarding but I don't understand the reasoning behind having additional endpoints for a "VDR variation" or for "HTTP/PrismDID AnonCreds methods
The current state of the ecosystem is highly deviated from a single standard.
Imagine that we need to support other VDR (Methods), for instance, cheqd or did:web.
We will get this requirement as soon as we start supporting the different DIDs, as it's a little bit odd to use DID did:web and not use did:web anon creds method.

@shotexa
Copy link
Contributor Author

shotexa commented Aug 22, 2024

I'm suggesting to isolate the schema's APIs by type and by spec w3c vc schema or the full name Verifiable Credentials JSON Schema - is a type anoncreds schema is another type. At the same time, there are a lot of specs for the CRUD operation: plain HTTP, did:prism, did:indy , etc Having ALL type and all METHODS in under the same path will blow up sooner or later.

I find it a bit hard to follow, specifically with differences between types of schemas and specs. I think it would be better if we could have a short call and you can explain to me what you mean exactly.

Shota Jolbordi added 5 commits August 29, 2024 16:45
@shotexa shotexa changed the title [WIP] feat(pollux): update storage for static resources to be wrapped in an envelope feat(pollux): update storage for static resources to be wrapped in an envelope Sep 2, 2024
@shotexa shotexa marked this pull request as ready for review September 2, 2024 12:11
Seq(),
ListMap(
"resourceService" -> Seq(publicEndpointServiceName),
"resourcePath" -> Seq(s"credential-definition-registry/definitions/${credentialDefinitionGUID.toString}/definition?resourceHash=$hash"),
Copy link
Member

Choose a reason for hiding this comment

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

Should the prefix cloud-agent be added to the path?
It should be a matter of configuration, so the agent should know the full external path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to add "cloud-agent" in the path. when we later construct an HTTP URL to resolve an actual resource from the DID URL, we will use resource service, which is going to be located inside the DID, we will resolve the did and this service will include the server base URL path, in the next ticket PR, when I add this service by default to every DID that issuer creates, the value of this service will be from the config, the value of public endpoint, and it does already include "cloud-agent" as you can see here

publicEndpointUrl = "https://host.docker.internal:8080/cloud-agent"

@@ -0,0 +1,10 @@
-- Create the enum type
CREATE TYPE resolution_method_enum AS ENUM ('http', 'did');
Copy link
Member

Choose a reason for hiding this comment

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

We need to rename did to did:prism as this resolution method is our domestic specification. What do you think, @shotexa ?

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'm not sure, when I created this enum I meant which type of URL to use, and the schema for either one is did or http, If I rename did to did:prism, this maybe assumes there might be did:somethingelse in the future that is why I'm making this more specific in my database.

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.

Excellent work, @shotexa!

@github-actions github-actions bot added the shared label Sep 5, 2024
Shota Jolbordi added 2 commits September 6, 2024 15:28
Signed-off-by: Shota Jolbordi <[email protected]>
Signed-off-by: Shota Jolbordi <[email protected]>
Copy link

@mattklepp mattklepp left a comment

Choose a reason for hiding this comment

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

should we add validation to the PrismEnvelopeResponse code to incorporate validation logic for the resource and url fields in case the url doesn't conform to the DID url format or string not in the correct format. Error handling.

Signed-off-by: Shota Jolbordi <[email protected]>
@shotexa
Copy link
Contributor Author

shotexa commented Sep 9, 2024

should we add validation to the PrismEnvelopeResponse code to incorporate validation logic for the resource and url fields in case the url doesn't conform to the DID url format or string not in the correct format. Error handling.

Validate at which point? it is a response type, so we are returning it, if it were a type we received in a request we would validate it ofc, but since we are returning it, it is assumed to be valid as long as we are creating it correctly. If we are not creating it correctly, this case is covered by tests.

@shotexa shotexa merged commit f78596f into ATL-6543-epic-vdr-phase-3 Sep 9, 2024
3 of 7 checks passed
@shotexa shotexa deleted the ATL-7415-update-storeage-statuc-resources branch September 9, 2024 11:50
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