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.

Sign up for GitHub

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

Jump to bottom

feat: add VC and VP functionality #494

Merged
merged 2 commits into from
Apr 8, 2022
Merged

feat: add VC and VP functionality #494

merged 2 commits into from
Apr 8, 2022

Conversation

Harasz
Copy link
Contributor

@Harasz Harasz commented Mar 1, 2022

No description provided.

@Harasz Harasz force-pushed the task/ICL-254-vc-poc branch 5 times, most recently from e83d7bd to 9c45618 Compare March 5, 2022 15:26
jrhender
jrhender reviewed Mar 7, 2022
View reviewed changes
Copy link
Collaborator

@jrhender jrhender left a comment

Choose a reason for hiding this comment

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

Great work! Some thoughts/questions

e2e/claims.service.e2e.ts Outdated Show resolved Hide resolved
e2e/fixtures/externalVC.ts Outdated Show resolved Hide resolved
src/modules/signer/signer.types.ts
@@ -33,3 +36,5 @@ export type AccountInfo = {

export const PUBLIC_KEY = 'PublicKey';
export const IS_ETH_SIGNER = 'isEthSigner';

export type SignerT = Signer & TypedDataSigner;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the relationship between Signer and TypedDataSigner? Could we just use TypedDateSigner?

What is the T meant to mean in SignerT?

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 couldn't find a good name for this type, so for now I left it as SignerT. Here is the code of the TypedDataSigner

export interface TypedDataSigner {
    _signTypedData(domain: TypedDataDomain, types: Record<string, Array<TypedDataField>>, value: Record<string, any>): Promise<string>;
}

And here is a comment for it in ethers lib

// @TODO: This is a temporary measure to preserse backwards compatibility
//        In v6, the method on TypedDataSigner will be added to Signer

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO we can call it just Signer

@jrhender
Copy link
Collaborator

jrhender commented Mar 7, 2022

@Harasz What would it take for this PR to be mergeable in your view?

@Harasz
Copy link
Contributor Author

Harasz commented Mar 7, 2022

@jrhender More tests. I am still checking it against Metamask and walletconnect. The next thing is to make it work on both Node and browser.

@Harasz
Copy link
Contributor Author

Harasz commented Mar 9, 2022

Here is the first working draft for VC

@Harasz Harasz self-assigned this Mar 9, 2022
@Harasz Harasz force-pushed the task/ICL-254-vc-poc branch 2 times, most recently from 23f448a to 1ce8c83 Compare March 9, 2022 12:38
@Harasz Harasz requested a review from JGiter March 10, 2022 10:41
@Harasz Harasz force-pushed the task/ICL-254-vc-poc branch 2 times, most recently from 83c8924 to a97b006 Compare March 24, 2022 10:02
@Harasz Harasz force-pushed the task/ICL-254-vc-poc branch 2 times, most recently from ad04894 to 29d8a36 Compare March 28, 2022 14:59
@Harasz Harasz changed the title chore: VC POC feat: add VC and VP functionality Mar 28, 2022
@Harasz Harasz requested a review from JGiter March 28, 2022 15:00
@Harasz
Copy link
Contributor Author

Harasz commented Mar 28, 2022

@jrhender @JGiter @nichonien Final work of the VP/VC features are ready for review

@jrhender jrhender marked this pull request as ready for review April 5, 2022 08:38
JGiter
JGiter reviewed Apr 6, 2022
View reviewed changes
src/modules/signer/signer.types.ts
@@ -33,3 +36,5 @@ export type AccountInfo = {

export const PUBLIC_KEY = 'PublicKey';
export const IS_ETH_SIGNER = 'isEthSigner';

export type SignerT = Signer & TypedDataSigner;
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO we can call it just Signer

src/modules/verifiable-credentials/eip712.types.ts Outdated Show resolved Hide resolved
src/modules/verifiable-credentials/verifiable-credentials-base.service.ts Outdated Show resolved Hide resolved
@jrhender
Copy link
Collaborator

jrhender commented Apr 7, 2022

@Harasz Can we remove the "DO NOT MERGE" label from your perspective? Or is that label still appropriate?

@Harasz
Copy link
Contributor Author

Harasz commented Apr 7, 2022

It will be ready to merge after applying review feedback and sync with develop. Will take care today

@Harasz Harasz force-pushed the task/ICL-254-vc-poc branch 2 times, most recently from 56ed14a to 8d83253 Compare April 7, 2022 16:24
@Harasz Harasz requested review from nichonien and JGiter April 7, 2022 16:25
jrhender
jrhender approved these changes Apr 8, 2022
View reviewed changes
Copy link
Collaborator

@jrhender jrhender left a comment

Choose a reason for hiding this comment

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

Some further suggestions but I think it looks great and we can make further incremental improvements as we go along 😄

e2e/fixtures/externalVC.ts Outdated Show resolved Hide resolved
src/modules/verifiable-credentials/index.ts Show resolved Hide resolved
src/modules/verifiable-credentials/verifiable-credentials-base.service.ts Outdated Show resolved Hide resolved
src/modules/verifiable-credentials/verifiable-credentials.types.ts Outdated Show resolved Hide resolved
src/modules/verifiable-credentials/verifiable-credentials.types.ts Outdated Show resolved Hide resolved
src/modules/verifiable-credentials/verifiable-credentials.types.ts Outdated Show resolved Hide resolved
src/modules/verifiable-credentials/verifiable-credentials-base.service.ts Outdated Show resolved Hide resolved
@Harasz Harasz force-pushed the task/ICL-254-vc-poc branch 2 times, most recently from e04f795 to 95bffec Compare April 8, 2022 08:41
@Harasz Harasz merged commit da13c0b into develop Apr 8, 2022
@Harasz Harasz deleted the task/ICL-254-vc-poc branch April 8, 2022 14:21
@ewf-devops
Copy link

🎉 This PR is included in version 5.1.0-alpha.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ewf-devops
Copy link

🎉 This PR is included in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JGiter JGiter JGiter approved these changes

@jrhender jrhender jrhender approved these changes

@nichonien nichonien Awaiting requested review from nichonien

Assignees

@Harasz Harasz

Labels
released on @canary released on @latest Verifiable Credentials
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Harasz @jrhender @ewf-devops @nichonien @JGiter