support PoL verification from synaps #1
Conversation
ilya-korotya
requested review from
martinsaporiti,
Kolezhniuk,
volodymyr-basiuk and
vmidyllic
| @@ -0,0 +1,26 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
could we reuse @iden3/eslint-config that we have already defined (common linter for all js projects) ?
There was a problem hiding this comment.
Aren't you still missing the extends here?
extends: ["@iden3/eslint-config"],
| "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand", | ||
| "test:e2e": "jest --config ./test/jest-e2e.json" | ||
| }, | ||
| "dependencies": { |
There was a problem hiding this comment.
i don't see @0xpolygonid/js-sdk in dependencies
| }; | ||
| } | ||
|
|
||
| private _getProposalMessage( |
There was a problem hiding this comment.
nit: we have functions like createProposal and createProposalRequest in sdk, that can create specific protocol message, so you could use them
There was a problem hiding this comment.
Okay. But I want to note that the from and to fields are optional in the iden3comm protocol, but in the JS sdk they are required fields. We need to fix these createSomeMessages ...
@vmidyllic @Kolezhniuk
There was a problem hiding this comment.
@Kolezhniuk also I need to possibility pass custom thid. This method from the js-sdk doens't okay for me.
There was a problem hiding this comment.
We need to fix these createSomeMessages
What do you mean when you say "we need to fix them"? It's ok that the types in the SDK make from and to required, even if they are optional in the more generic iden3comm protocol.
src/setup/sdk.ts
Outdated
| }); | ||
|
|
||
| const proofService = new ProofService( | ||
| {} as any, |
There was a problem hiding this comment.
it is not right to pass mocked wallets.. !
There was a problem hiding this comment.
Why? I don't want to initialise the wallet service because I don't generate any proofs. We need to have a flexible sdk and not ask the user to initialise parts they don't need.
There was a problem hiding this comment.
You can implement interface IProofService with implementation that only you need. It is not right to pass objects as 'any'
There was a problem hiding this comment.
Yes. But I don't need any thing from IProofService interface
There was a problem hiding this comment.
what is done?
you pass empty object still.
ilya-korotya
force-pushed
the
feature/example
branch
from
bfb1a1a to
76197e6
Compare
| RPC_URL=https://rpc-mainnet.privado.id | ||
| STATE_CONTRACT_ADDRESS=0x975556428F077dB5877Ea2474D783D6C69233742 | ||
|
|
||
| AGENT_URL=<NGROK_URL_TO_PROXY_REQUESTS_FROM_WORLD> |
There was a problem hiding this comment.
I'd say PUBLICLY_ACCESSIBLE_URL_TO_PROXY_REQUESTS
| @@ -0,0 +1,26 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
Aren't you still missing the extends here?
extends: ["@iden3/eslint-config"],
| private _validateBasicMessage(basicMessage: BasicMessage) { | ||
| if (!basicMessage.from || !basicMessage.to) { | ||
| throw new Error( | ||
| "Invalid basic message: 'from' and 'to' fields cannot be empty", |
There was a problem hiding this comment.
Wouldn't it make sense to make these required at the SDK? (I don't mean now, but at some point)
| private _validateProposalRequestMessage( | ||
| proposalMessageBody: ProposalRequestMessage, | ||
| ) { | ||
| if (!proposalMessageBody.body) { |
There was a problem hiding this comment.
Isn't this check unnecessary being body a required prop in a ProposalRequestMessage?
| } | ||
|
|
||
| private _validateProposalRequestMessage( | ||
| proposalMessageBody: ProposalRequestMessage, |
There was a problem hiding this comment.
This should be named proposalRequestMessage
| "baseUrl": "./", | ||
| "incremental": true, | ||
| "skipLibCheck": true, | ||
| "strictNullChecks": false, |
There was a problem hiding this comment.
We should change this to true and add:
"noUncheckedIndexedAccess": true,
| const credentialContext = proposalMessageBody.body.credentials[0].context; | ||
| if (credentialContext !== SupportedCredential.JsonLDSchema) { | ||
| throw new Error( | ||
| `Invalid credential context: '${credentialContext}' is not supported`, |
There was a problem hiding this comment.
This can lead to the msg
Invalid credential context: undefined is not supported
Instead, we should make sure the credential is available first.
I'd go this way:
const [credential, ...rest] = proposalMessageBody.body.credentials;
if (!credential || rest.length > 0) {
throw new Error(
"Invalid proposal message body: 'credentials' field must contain exactly one credential",
);
}
// credential is defined| const credentiaType = proposalMessageBody.body.credentials[0].type; | ||
| if (credentiaType !== SupportedCredential.Type) { | ||
| throw new Error( | ||
| `Invalid credential type: '${credentiaType}' is not supported`, |
There was a problem hiding this comment.
Same as before. credentiaType can be undefined here.
| `Invalid credential context: '${credentialContext}' is not supported`, | ||
| ); | ||
| } | ||
| const credentiaType = proposalMessageBody.body.credentials[0].type; |
| switch (unpackedMessage.type) { | ||
| case PROTOCOL_CONSTANTS.PROTOCOL_MESSAGE_TYPE | ||
| .PROPOSAL_REQUEST_MESSAGE_TYPE: | ||
| const proposalRequest = unpackedMessage as ProposalRequestMessage; |
There was a problem hiding this comment.
I don't expect any changes here now, but let me add a thought:
The SDK's packageMgr.unpack should return properly typed messages so we can switch/case its type and don't need these type-assertions in the client.
And while it doesn't, the client should be parsing the msg instead of type-asserting it.
| export interface ICreateCredentialRequest { | ||
| credentialSchema: string; | ||
| type: string; | ||
| credentialSubject: { [key: string]: any }; |
There was a problem hiding this comment.
This should use the type JSONObject from the SDK.
| IPacker, | ||
| ProvingParams, | ||
| } from '@0xpolygonid/js-sdk'; | ||
| // import { path } from 'path'; |
No description provided.