759 sri accept eddsa signatures #781
Conversation
…-sri-accept-eddsa-signatures
biscuitdey
requested review from
skarred14,
ognjenkurtic,
Therecanbeonlyone1969,
Kasshern and
Ybittan
as code owners
|
Ecdsa - Used for login and signing messages @biscuitdey it was not possible to use a single key format for both purposes? |
If not, i would vote we introduce a new Prisma model called public key which would store the key type and all relevant information. BpiSubject could contain 1 or more PublicKeys. This way we do not need to place them into JSON blobs and perform hacks around the code to work with the keys. But i would really prefer if we could stick with a single key format for the time being. Requesting multiple key formats from the end user does not sound fun. |
We need both types of keys. Besides login & messaging, we need to request the ecdsa keys from the user for generating |
| @@ -0,0 +1,29 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
maybe to make scope of PR smaller and easier to figure out what is actual diff and what is changed because of linter, lets not add linter for frontend in this PR too?
| @@ -1,37 +1,50 @@ | |||
| import { Injectable } from '@nestjs/common'; | |||
| import { BpiSubject as BpiSubjectPrismaModel, BpiSubjectRole as BpiSubjectRolePrismaModel } from '@prisma/client'; | |||
| import { | |||
There was a problem hiding this comment.
@biscuitdey could you please remove linter changes from PR? it will make review easier, for example, i doubt prisma mapper changes are needed here, so its probably linter, but still required to check
we can do linter only changes in separate PR
There was a problem hiding this comment.
I will move the linter to a separate PR.
| } | ||
|
|
||
|
|
||
| const createEddsaPrivateKey = async (publicKeyOwner) => { |
There was a problem hiding this comment.
function name indicates it is creating private key, but function is creating and returning a signature
you have bellow createPublicKey which is creating private key and then public key out of it? why is this function needed?
There was a problem hiding this comment.
We needed to generate and store Eddsa keypair and signature for the circom circuit (as Ecdsa signatures are too big to handle). But currently the user is using Metamask for key management, which uses Ecdsa keys. So, as a workaround, Andreas suggested we utilise the Metamask public key, generate a signature and use that for our Eddsa private key. You can see the discussion here
| */ | ||
| -- AlterTable | ||
| ALTER TABLE "BpiSubject" DROP COLUMN "publicKey", | ||
| ADD COLUMN "publicKey" JSONB NOT NULL; |
There was a problem hiding this comment.
why storing json { pk1, pk2 } instead of having pk1 and pk2 as separate fields? storing json seems like it can just introduce not needed overhead to always parse and unparse
There was a problem hiding this comment.
I will remove the JSON and instead create another table for storing public keys.
|
Will update the code in smaller PR |
Description
This PR introduces the use of EDdsa keys and signature throughout the application.
Ecdsa - Used for login and signing messages
Eddsa - Used for signing transactions (as it needs to verified within a circuit)
Related Issue
#759
Motivation and Context
Eddsa signatures are required for circom circuits
How Has This Been Tested
All units tests have been updated accordingly.
Screenshots (if appropriate)
Types of changes
Checklist