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

credential api #177

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

credential api #177

wants to merge 4 commits into from

Conversation

AhmedAbdelmenam
Copy link
Contributor

Description

Please provide a brief summary of the changes in this PR.

Type of Change

  • Bug fix
  • New feature
  • Test case
  • Documentation update
  • Other (please describe)

Checklist

  • I have added tests that prove my fix is effective or that my feature works (optional)
  • I have added necessary documentation (if applicable)

4- steps to test ?

Please provide a brief summary of the steps required to test the changes in this PR.

5- results ?

Please provide a brief summary of the results after testing the changes in this PR.

7- screenshots ?

Please provide screenshots of the results after testing the changes in this PR.

"context" TEXT NOT NULL,
"validatorName" TEXT NOT NULL,
"validatorEmail" TEXT NOT NULL,
"claimId" INTEGER NOT NULL,
"validationClaimId" INTEGER,
"validationStatus" "ValidationStatus" NOT NULL DEFAULT 'PENDING',
"response" "ResponseStatus",
"response" "ResponseStatus" NOT NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

please do NOT do this allow null

@@ -34,12 +50,15 @@ CREATE TABLE "Image" (
-- CreateTable
CREATE TABLE "CandidUserInfo" (
"id" SERIAL NOT NULL,
"createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"updatedAt" TIMESTAMP(3) NOT NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is interesting are we repurposing the candid workflow for validations? why edit this table?

@@ -77,6 +77,56 @@ export const claimPostSchema = Joi.object({
}),
});

export const credentialPostSchema = Joi.object({
context: Joi.array().required(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

hey remove required from ALL of these please except the id

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note i think we might want to use the credential endpoint for OTHER types of credentials

Are we linking somewhere to the schema? its in the credential

"https://www.w3.org/2018/credentials/v1",
"https://purl.imsglobal.org/spec/ob/v3p0/context-3.0.3.json",

can we please REMOVE the Joi validation

and later we can add credential validation using the credential schema, which is the correct way to do it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we do NOT want to define the schema here in hardcode AND in the real schema itself

so lets remove here

@gvelez17
Copy link
Collaborator

hey @AhmedAbdelmenam i think i see what you are doing here, originally i did not want to do this (upload credentials into linkedtrust backend) but I think its valid since it accepts different kinds of credentials

Please REMOVE the changes to the candid validator and any Candid related fields

please REMOVE all the 'required' from the schema except ID

make sure we can retrieve by the id which is of this form: "id": "urn:uuid:965a7750-d953-4a4f-ad12-b853da388f15",

then we can do a follow up pr to add a normal Linked Claim that will wrap this credential so it can be inserted into our normal graph

I will think a little about how to wrap it.

cc: @EngAmal27 @Marwanfouz @omareloui @Agneskoinange this is sort of the 'profile' we're talking about in DIF labs ways to wrap other types of objects in LinkedClaims that are fully compliant.

what we want to think about is how other types of objects can be in the LinkedClaim graph, should there be a type field that interprets something, maybe a mapping, anyway lets think aobut this

For now @AhmedAbdelmenam please make the changes requested and ping me to rereview, then we'll follow up with another PR to get it embeded into the graph

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants