feat: request verifiable presentation from user (#509)

[AndrewCLu]

Explanation

This change implements Verifiable Presentations, a way for users to present Verifiable Credentials to verifiers in an authentic way. The lifecycle is as follows:

1. Web application requests Verifiable Credentials from a user, with a certain request message detailing what type of credentials the service is looking for.
2. A popup allows users to select Verifiable Credentials they wish to share with the application.
3. Cryptkeeper generates a Verifiable Presentation from the selected credentials.
4. Users may either send the Verifiable Presentation without a signature or use their Metamask Wallet to sign the presentation with an Ethereum keypair.
5. The web application verifies that the Verifiable Presentation is authentic and that it contains the credentials it is looking for (not implemented in Cryptkeeper).

• Create popup allowing users to select Verifiable Credentials
• Generate Verifiable Presentation from selected credentials
• Allow users to sign the Verifiable Presentation
• Connect Verifiable Presentation request and submission to event lifecycle
• Integrate Verifiable Presentations into demo
• Add tests for components and hooks
• Make Verifiable Credential Items in popup similar to IdentityList Items

More Information

See this change for reference on integration with displaying Verifiable Credentials.

Screenshots/Screencaps

View a request for Verifiable Credentials and select ones you want to share:

Choose to either sign a Verifiable Presentation or send it in the clear:

View a Verifiable Presentation request in full page window:

Sign your Verifiable Presentation with metamask:

Manual Testing Steps

1. Load the demo
2. Add some Verifiable Credentials to your wallet
3. Click on the button to "Generate a Verifiable Presentation
4. Select the credentials you would like to share
5. Connect your Metamask
6. Authorize the signature
7. Check that the demo has received your signed Verifiable Presentation

Pre-Merge Checklist

• PR template is filled out
• Pre-commit and pre-push hook checks are passed
• E2E tests are passed locally
• IF this PR fixes a bug, a test that would have caught the bug has been added
• PR is linked to the appropriate GitHub issue
• PR has been added to the appropriate release Milestone

PR template source from github.com/MetaMask

[AndrewCLu]

@0xisk @0xmad I finished adding tests for the new hooks/components as well as updating the VerifiableCredentialItem to match the style of IdentityItem, as we discussed in the meeting. It should be ready for a look now.

[0xisk]

@AndrewCLu Since I have been working on the documentation part, I noticed that we have already two functions published in the provider which are:

• crypt-keeper-extension/packages/providers/src/sdk/CryptKeeperInjectedProvider.ts

  Lines 366 to 377 in 29b211b

  	/**
  	* Requests user to add a verifiable credential.
  	*
  	* @param {string} serializedVerifiableCredential - The json string representation of the verifiable credential to add.
  	* @returns {void}
  	*/
  	async addVerifiableCredentialRequest(serializedVerifiableCredential: string): Promise<void> {
  	await this.post({
  	method: RPCAction.ADD_VERIFIABLE_CREDENTIAL_REQUEST,
  	payload: serializedVerifiableCredential,
  	});
  	}

• crypt-keeper-extension/packages/providers/src/sdk/CryptKeeperInjectedProvider.ts

  Lines 379 to 389 in 29b211b

  	/**
  	* Requests user to reveal a connected identity commitment.
  	*
  	* @returns {Promise<void>}
  	*/
  	async revealConnectedIdentityRequest(): Promise<void> {
  	await this.post({
  	method: RPCAction.REVEAL_CONNECTED_IDENTITY_COMMITMENT_REQUEST,
  	});
  	}
  	}

  
  And now in this PR we have another function:

• crypt-keeper-extension/packages/providers/src/sdk/CryptKeeperInjectedProvider.ts

  Lines 393 to 407 in 29d4676

  	/**
  	* Requests user to provide a verifiable presentation.
  	*
  	* @param {IVerifiablePresentationRequest} verifiablePresentationRequest - The information provided to the user when requesting a verifiable presentation.
  	* @returns {void}
  	*/
  	async generateVerifiablePresentationRequest(
  	verifiablePresentationRequest: IVerifiablePresentationRequest,
  	): Promise<void> {
  	await this.post({
  	method: RPCAction.GENERATE_VERIFIABLE_PRESENTATION_REQUEST,
  	payload: verifiablePresentationRequest,
  	});
  	}
  	}

My worry is to not confuse developers by having non production ready functions in the client. I am thinking of adding a flag inside the provider to distinguish the development features from the others. This is the idea I got but you can thinking of a better way:

export class CryptKeeperInjectedProvider {
 const IS_UNDER_DEVELOPMENT = true;

  /**
   * Requests user to add a verifiable credential.
   * NOTE: THIS FUNCTION IS UNDER DEVELOPMENT AND NOT READY FOR PRODUCTION USE
   * 
   * @param {string} serializedVerifiableCredential - The json string representation of the verifiable credential to add.
   * @returns {void}
   */
  async addVerifiableCredentialRequest(serializedVerifiableCredential: string): Promise<void> {
    if (IS_UNDER_DEVELOPMENT) {
      console.warn('This function is under development and not ready for production use.');
      return;
    }
  
    // ...
  }
}

We can maybe add _ prefix before the function name. 🤔

[AndrewCLu]

@AndrewCLu Since I have been working on the documentation part, I noticed that we have already two functions published in the provider which are:

• crypt-keeper-extension/packages/providers/src/sdk/CryptKeeperInjectedProvider.ts

  Lines 366 to 377 in 29b211b

  	/**
  	* Requests user to add a verifiable credential.
  	*
  	* @param {string} serializedVerifiableCredential - The json string representation of the verifiable credential to add.
  	* @returns {void}
  	*/
  	async addVerifiableCredentialRequest(serializedVerifiableCredential: string): Promise<void> {
  	await this.post({
  	method: RPCAction.ADD_VERIFIABLE_CREDENTIAL_REQUEST,
  	payload: serializedVerifiableCredential,
  	});
  	}

• crypt-keeper-extension/packages/providers/src/sdk/CryptKeeperInjectedProvider.ts

  Lines 379 to 389 in 29b211b

  	/**
  	* Requests user to reveal a connected identity commitment.
  	*
  	* @returns {Promise<void>}
  	*/
  	async revealConnectedIdentityRequest(): Promise<void> {
  	await this.post({
  	method: RPCAction.REVEAL_CONNECTED_IDENTITY_COMMITMENT_REQUEST,
  	});
  	}
  	}

  And now in this PR we have another function:

• crypt-keeper-extension/packages/providers/src/sdk/CryptKeeperInjectedProvider.ts

  Lines 393 to 407 in 29d4676

  	/**
  	* Requests user to provide a verifiable presentation.
  	*
  	* @param {IVerifiablePresentationRequest} verifiablePresentationRequest - The information provided to the user when requesting a verifiable presentation.
  	* @returns {void}
  	*/
  	async generateVerifiablePresentationRequest(
  	verifiablePresentationRequest: IVerifiablePresentationRequest,
  	): Promise<void> {
  	await this.post({
  	method: RPCAction.GENERATE_VERIFIABLE_PRESENTATION_REQUEST,
  	payload: verifiablePresentationRequest,
  	});
  	}
  	}

My worry is to not confuse developers by having non production ready functions in the client. I am thinking of adding a flag inside the provider to distinguish the development features from the others. This is the idea I got but you can thinking of a better way:

export class CryptKeeperInjectedProvider {
 const IS_UNDER_DEVELOPMENT = true;

  /**
   * Requests user to add a verifiable credential.
   * NOTE: THIS FUNCTION IS UNDER DEVELOPMENT AND NOT READY FOR PRODUCTION USE
   * 
   * @param {string} serializedVerifiableCredential - The json string representation of the verifiable credential to add.
   * @returns {void}
   */
  async addVerifiableCredentialRequest(serializedVerifiableCredential: string): Promise<void> {
    if (IS_UNDER_DEVELOPMENT) {
      console.warn('This function is under development and not ready for production use.');
      return;
    }
  
    // ...
  }
}

We can maybe add _ prefix before the function name. 🤔

@0xisk @0xmad So right now the VERIFIABLE_CREDENTIALS feature flag we use is just for the app package. We could either change it to set up a repo-level config or create a separate flag just for the sdk, but we would have to make sure they are in sync.

I was thinking that perhaps it isn't necessary to enforce the feature flag at the sdk level, if we do the check in the browser extension anyways. I just renamed the sdk functions with the prefix DEV_ and added comments. If we think this makes sense, then I will also gate the VC RPC calls with a feature flag check. Let me know what you think.

Also updated the PR with screenshots based on the new UI.

[0xmad]

@AndrewCLu thanks, there are some comments.
In addition there is some code which isn't covered with unit tests. It's related to all layers (ui, background, ducks and etc).

[0xmad]

Regarding ui testing:

1. These styles doesn't match with our identity list styles. It should be aligned to the left side.
   

2. The buttons doesn't match with the previous screen
   

3. Why do we need select screen and sign screen if we can use only one screen to complete user action?

4. If I open these pages in a separate tab the styles are broken.
   
   

5. The same style error is related to add verifiable credential page
   

[AndrewCLu]

Regarding ui testing:

@0xmad Thanks for the feedback!

1. Fixed the alignment. Updated the screenshots above.
2. Fixed the button styling differences. Updated the screenshots above.
3. I think the select screen is very important due to the "Reject" button. Being asked to submit credentials is a very intrusive popup, so I think it makes sense to have the user immediately have an option to reject without worrying about selection or signing. If we wanted to include them all on the same page I think it would get too complicated.
   4 & 5). Similar to the comment above about URLs, the Verifiable Presentations flow will not ever be experienced by a user in another window. It's just a popup screen requested by the website, and never produced from within the extension. I'm also not quite sure what styling errors you are referring to.

[0xmad]

@AndrewCLu thanks, there are still some issues:

1. Proceed Without Signing button still doesn't match the height style.
2. If you take a look at ConnectIdentity page it works with the same select pattern but here we need to add dropdown button component to handle multiple options for signing. With this approach we can get rid of additional step and keep reject button.
3. If you open two pages you implemented in a separate tab they both have some default page styles, overlay and modal. It's not correct if user works in a separate window. I have a task to support event propagation between the tabs and it allows users to use cryptkeeper between all active tabs where identity is connected. We are not sure if it's always be as-is with popup opening so it's better to fix style errors for this cases. For example, we didn't think we need to create identity from connect screen because there was no connection approach. If we decide to sign VCs using link, we need to support separate screen. It's not a big task and it doesn't require to rewrite the whole logic.

[AndrewCLu]

@AndrewCLu thanks, there are still some issues:

1. Proceed Without Signing button still doesn't match the height style.
2. If you take a look at ConnectIdentity page it works with the same select pattern but here we need to add dropdown button component to handle multiple options for signing. With this approach we can get rid of additional step and keep reject button.
3. If you open two pages you implemented in a separate tab they both have some default page styles, overlay and modal. It's not correct if user works in a separate window. I have a task to support event propagation between the tabs and it allows users to use cryptkeeper between all active tabs where identity is connected. We are not sure if it's always be as-is with popup opening so it's better to fix style errors for this cases. For example, we didn't think we need to create identity from connect screen because there was no connection approach. If we decide to sign VCs using link, we need to support separate screen. It's not a big task and it doesn't require to rewrite the whole logic.

@0xmad Thanks again for the feedback, I made some changes:

1. Refactored the page so that signing and rejecting happens on the same screen. Now, users can use a dropdown to select which signing method they would like to use, or reject the request.
2. Created an option to sign with Cryptkeeper. This follows the same UX as CreateIdentity.
3. Updated the styles for the popup and separate window. The buttons should now be aligned, and I tested the view on fullscreen. See screenshots for reference.
4. Removed createModalRoot from tests, as you suggested.
5. Updated tests to get 100% coverage for the code in these changes (background, ducks, and UI).

[0xisk]

Thanks @AndrewCLu, I had my review and left my thoughts and requested changes in this PR #867

• Another point:
  Thinking of the PCD idea, why do not we start having a generic interface for defining what are the components of a VC? You can take a look at this package (https://github.com/proofcarryingdata/zupass/blob/main/packages/pcd-types/src/pcd.ts) to get the idea. More precisely PCD and PCDPackage

[AndrewCLu]

@0xmad I updated the styling so that the buttons are now at the bottom. See the new screenshots for reference.

@0xisk I agree with all of your comments and changes in #867, but I think its possible for me to separate them into a new PR distinct from this one, since this is centered around verifiable presentations. Perhaps I can combine it with adding comments to all the functions.

[0xmad]

@AndrewCLu thanks, just some minor comments.

[0xmad]

@AndrewCLu regarding the test coverage: there are some missing tests for src/background/services/credentials folder and for new pages and related components (AddVerifiableCredential, PresentVerifiableCredential). Usually, I use test:coverage report and cover the missing statements, branches according to this report.

[0xisk]

@0xmad I updated the styling so that the buttons are now at the bottom. See the new screenshots for reference.

@0xisk I agree with all of your comments and changes in #867, but I think its possible for me to separate them into a new PR distinct from this one, since this is centered around verifiable presentations. Perhaps I can combine it with adding comments to all the functions.

@AndrewCLu I agree with you I created a PR for this #876

[0xisk]

Thanks @AndrewCLu I can approve from my end, just please finishing fixing the other requested changes from Anton.

[AndrewCLu]

@AndrewCLu regarding the test coverage: there are some missing tests for src/background/services/credentials folder and for new pages and related components (AddVerifiableCredential, PresentVerifiableCredential). Usually, I use test:coverage report and cover the missing statements, branches according to this report.

@0xmad Addressed your comments, only caveat is I will include the browser push changes in my refactor PR since I will be modify all the event related code there anyways.

With regard to test coverage, I just updated all the tests with regards to verifiable presentations (PresentVerifiableCredential, those in services/credential, etc) to be 100% coverage. I am currently getting my other VC-related code coverage to 100% in the refactor PR which I linked above, since I think it makes sense to keep this change to Verifiable Presentations.

[0xmad]

@AndrewCLu ok, I'm good with fixing these changes in refactoring PR.

[0xmad]

@AndrewCLu thanks!