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

Trust registry and did:key #382

Merged
merged 36 commits into from
Jan 17, 2024
Merged

Trust registry and did:key #382

merged 36 commits into from
Jan 17, 2024

Conversation

olegnn
Copy link
Contributor

@olegnn olegnn commented Dec 12, 2023

No description provided.

@mike-parkhill mike-parkhill requested a review from lovesh December 13, 2023 14:16
lovesh
lovesh reviewed Dec 13, 2023
View reviewed changes
src/utils/did.js Outdated

export const DockDIDMethod = 'dock';
export const DockDIDQualifier = `did:${DockDIDMethod}:`;
export const DockDIDMethodKeyQualifier = `did:key:${DockDIDMethod}:`;
Copy link
Member

Choose a reason for hiding this comment

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

The full DID would be did:key:dock:<value> but as per spec https://w3c-ccg.github.io/did-method-key/, full DID should be did:key:<value>

lovesh
lovesh reviewed Dec 13, 2023
View reviewed changes
src/utils/did.js Outdated
}
}

const SECP256K1_PUBLIC_KEY_PREFIX = 'zQ3';
Copy link
Member

Choose a reason for hiding this comment

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

ZQ3 -> ZQ3s as per spec.

lovesh
lovesh reviewed Dec 13, 2023
View reviewed changes
src/utils/did.js Outdated
return createDidSig(this, keyRef, keySignature);
}

changeState(method, name, payload, keyRef) {
Copy link
Member

Choose a reason for hiding this comment

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

This is only used to create an extrinsic so better to name it accordingly.

lovesh
lovesh reviewed Dec 13, 2023
View reviewed changes
src/utils/did.js Outdated
const ED_25519_PUBLIC_KEY_PREFIX = 'z6Mk';

const DockDidMethodKeySecp256k1Prefix = `${DockDIDQualifier}${SECP256K1_PUBLIC_KEY_PREFIX}`;
const DockDidMethodKeyEd25519Prefix = `${DockDIDMethodKeyQualifier}${ED_25519_PUBLIC_KEY_PREFIX}`;
Copy link
Member

Choose a reason for hiding this comment

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

This and above should have key prefix and not dock

lovesh
lovesh reviewed Dec 13, 2023
View reviewed changes
tests/integration/trust-registry-module.test.js Outdated
@@ -0,0 +1,326 @@
import { randomAsHex } from "@polkadot/util-crypto";
Copy link
Member

Choose a reason for hiding this comment

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

This test mentions status list repeatedly in the comments. Need to be fixed.

@lovesh
Fix imports
e96424b 
Signed-off-by: lovesh <[email protected]>
@lovesh
Fix more tests
219b941 
Signed-off-by: lovesh <[email protected]>
lovesh
lovesh reviewed Dec 14, 2023
View reviewed changes
src/modules/did/did.js
@@ -49,7 +51,7 @@ class DIDModule {
* @returns {*}
*/
createNewOffchainTx(did, didDocRef) {
const hexId = getHexIdentifierFromDID(did);
const hexId = typedHexDID(this.api, did).asDid;
Copy link
Member

Choose a reason for hiding this comment

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

You know this did won't be a did:key so its more efficient to use getHexIdentifier(did, DockDIDQualifier, DockDIDByteSize) here than calling typedHexDID which creates an unnecessary object

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 don't think it will differ in efficiency despite an unnecessary object will be created.

lovesh
lovesh reviewed Dec 14, 2023
View reviewed changes
src/modules/did/did.js
@@ -76,7 +78,7 @@ class DIDModule {
* @returns {*}
*/
createSetOffchainDidRefTx(did, didDocRef) {
const hexId = getHexIdentifierFromDID(did);
const hexId = typedHexDID(this.api, did).asDid;
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment as above on this line and similar lines as below.

lovesh
lovesh reviewed Dec 14, 2023
View reviewed changes
src/modules/WithParamsAndPublicKeys.js
* @param withParams - If true, return the params referenced by the public key. It will throw an error if paramsRef is null
* or params were not found on chain which can happen if they were deleted after this public key was added.
* @returns {Promise<{bytes: string}|null>}
*/
async getPublicKey(did, keyId, withParams = false) {
const hexId = getHexIdentifierFromDID(did);
const hexId = typedHexDID(this.api, did);
Copy link
Member

Choose a reason for hiding this comment

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

did:key can't have an onchain public key so you can just use getHexIdentifier and avoid creating extra object in typedHexDID

lovesh
lovesh reviewed Dec 14, 2023
View reviewed changes
src/modules/schema.js Outdated
@@ -3,7 +3,6 @@ import { validate } from 'jsonschema';
import axios from 'axios';
Copy link
Member

Choose a reason for hiding this comment

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

axios was removed here.

lovesh
lovesh reviewed Dec 14, 2023
View reviewed changes
src/utils/vc/credentials.js Outdated
@@ -3,8 +3,7 @@ import jsigs from 'jsonld-signatures';
import { statusTypeMatches, checkStatus } from '@digitalbazaar/vc-status-list';
Copy link
Member

Choose a reason for hiding this comment

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

This file is missing some code in verifyCredential from master branch.

lovesh
lovesh reviewed Dec 21, 2023
View reviewed changes
src/modules/did/did.js
@@ -730,29 +682,35 @@ class DIDModule {
* Throws NoDID if the DID does not exist on chain.
* @param {string} did - The DID can be passed as fully qualified DID like `did:dock:<SS58 string>` or
* a 32 byte hex string
* @param getOffchainSigKeys
* @return {Promise<object>} The DID document.
*/
// eslint-disable-next-line sonarjs/cognitive-complexity
async getDocument(did, { getOffchainSigKeys = true } = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please update this method to use didDetails RPC so avoid multiple calls to the chain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be done separately.

lovesh
lovesh reviewed Jan 10, 2024
View reviewed changes
src/utils/did.js
const key = did.asDidMethodKey;

if (key.isSecp256k1) {
const hex = getHexIdentifier(
Copy link
Member

Choose a reason for hiding this comment

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

As per the implementation of DockDidMethodKey, the following 2 calls to it should fail. Is there a test for this branch of code?

@lovesh lovesh force-pushed the trust-registry-did-key branch from 945c5e9 to e8c615e Compare January 10, 2024 16:26
@lovesh lovesh changed the base branch from master to develop January 10, 2024 16:27
@lovesh lovesh changed the base branch from develop to master January 10, 2024 16:28
lovesh
lovesh previously approved these changes Jan 17, 2024
View reviewed changes
cykoder
cykoder previously approved these changes Jan 17, 2024
View reviewed changes
@lovesh lovesh dismissed stale reviews from cykoder and themself via bf124c3 January 17, 2024 16:34
@lovesh lovesh force-pushed the trust-registry-did-key branch from e8c615e to bf124c3 Compare January 17, 2024 16:34
@lovesh lovesh requested a review from mike-parkhill January 17, 2024 16:36
@lovesh lovesh force-pushed the trust-registry-did-key branch from bf124c3 to e8c615e Compare January 17, 2024 16:59
@lovesh lovesh merged commit 932af67 into master Jan 17, 2024
18 checks passed
@cykoder cykoder deleted the trust-registry-did-key branch January 17, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mike-parkhill mike-parkhill mike-parkhill approved these changes

@cykoder cykoder cykoder left review comments

@lovesh lovesh lovesh left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@olegnn @lovesh @cykoder @mike-parkhill