feature/SDK-8 #27

zoemaas · 2024-04-05T17:13:16Z

No description provided.

…to-extensions into feature/SDK-8

…ation

…id provider

nklomp

See remarks

nklomp · 2024-04-28T20:24:33Z

packages/did-provider-ebsi/src/services/EbsiRPCService.ts

+  const query = `?${[offset && `page[after]=${offset}`, size && `page[size]=${size}`, controller && `controller=${controller}`]
+    .filter(Boolean)
+    .join('&')}`
+  return await (await fetch(`https://api-pilot.ebsi.eu/did-registry/v5/identifiers${query}`)).json()


Please make sure that all the static links to EBSI are an optional parameter from a predefined set as well as string (string union). Then have a default value you can configure on the plugin for the EBSI environment. That way it will always use the default, and you can optionally provide a different env

Added query url to getUrls method

nklomp · 2024-04-28T20:27:11Z

packages/did-provider-ebsi/src/services/EbsiRPCService.ts

+ */
+export const listDidDocuments = async (args: GetDidDocumentsParams): Promise<GetDidDocumentsResponse> => {
+  const { offset, size, controller } = args
+  const query = `?${[offset && `page[after]=${offset}`, size && `page[size]=${size}`, controller && `controller=${controller}`]


This is a really confusing construct to read, especially because of the string interpolation happening from the beginning. Please make that better readable

Extracted it to if clauses

nklomp · 2024-04-28T20:27:52Z

packages/did-provider-ebsi/src/services/EbsiRPCService.ts

+  if (!did) {
+    throw new Error('did parameter is required')
+  }
+  const query = `?${validAt && `valid_at=${validAt}`}`


Do not use string interpolation within string interpolation

Extracted nested string interpolation into ternary

nklomp · 2024-04-28T20:28:57Z

packages/did-provider-ebsi/src/services/EbsiRPCService.ts

+  id: number
+}): Promise<Response> => {
+  const { params, id } = args
+  const options = {


Please extract these into a method. We now have the same things all over the place, with only a different method. If we need to change something in the future the chance of introducing bugs is high

moved the code to the buildFetchOptions(...) method

packages/did-provider-ebsi/src/services/EbsiRPCService.ts

nklomp · 2024-04-28T20:30:49Z

packages/did-provider-ebsi/src/services/EbsiRPCService.ts

+ * @constant {string} jsonrpc
+ */
+const jsonrpc = '2.0' // optional param of plugin?
+const baseUrl = 'https://api-pilot.ebsi.eu/did-registry/v5/jsonrpc' // optional param of plugin?


This should be part of a string union containing the different EBSI environments. Then we should concatenate the version and did-registry.

Created EbsiEnvironment type which is a string intersection of different environments and moved it to src/types.ts. Created a function getUrls to build the url and moved it to src/function.ts. Adjusted the service methods and the plugin constructor to receive an additional optional parameter: apiOpts

nklomp · 2024-04-28T20:33:18Z

packages/did-provider-ebsi/src/functions.ts

+      return SigningKey.computePublicKey(bytes, false)
+    }
+    case 'Secp256r1': {
+      const jwk: JsonWebKey = toJwk(key.publicKeyHex, type, { use: JwkKeyUse.Signature, key })


Why do we need to convert the public key hex to JWK and then back to public key hex?

In other words, what is the difference? If there is one, please describe it

The difference is explained in the documentation: "Public key as hex string. For an ES256K key, it must be in uncompressed format prefixed with "0x04". For other algorithms, it must be the JWK transformed to string and then to hex format."

Please reread the implementation (which will be wrong anyway, as that is not how you convert a jwk to hex to begin with. We have functions for these conversions). The comment reads it should be hex and the last thing you do is convert to base16 which is hex.

As mentioned why would you need to do it to begin with? If the comment is correct then you already had the hex key, if not then you need to adjust, but converting a jwk to base16 directly always is wrong, you need to get the public key component first (and we have functions for these)

Okay. It seems they really are doing a conversion on the JWK to hex, which is rather odd without a codec to begin with and thus prone to errors across platforms, but so be it. Please create a comment explaining what is happening and why, as I would immediately start working on this code if an error occurs, because it looks so of

Added a comment explaining what and why is happening.

nklomp · 2024-04-28T20:34:17Z

packages/did-provider-ebsi/package.json

    "debug": "^4.3.4",
    "did-resolver": "^4.1.0",
+    "ethers": "^6.11.1",
+    "jose": "^4.14.6",


Please merge other branch, because jose is still (or again?) listed as dependency and that for sure isn't compatible with RN

Removed jose dependency

…to-extensions into feature/SDK-8 # Conflicts: # packages/did-provider-ebsi/package.json # packages/did-provider-ebsi/src/EbsiDidProvider.ts # packages/did-provider-ebsi/src/types.ts # pnpm-lock.yaml

nklomp

Please see remarks

nklomp · 2024-05-10T08:07:02Z

packages/did-provider-ebsi/src/EbsiDidProvider.ts

@@ -53,6 +94,125 @@ export class EbsiDidProvider extends AbstractIdentifierProvider {
    throw Error(`Type ${type} not supported`)
  }

+  async createEbsiDid(
+    args: {


Please create an interface/type out of this like done elsewhere

extracted CreateEbsiDidParams interface

nklomp · 2024-05-10T08:10:48Z

packages/did-provider-ebsi/src/EbsiDidProvider.ts

+  async createEbsiDid(
+    args: {
+      identifier: Omit<IIdentifier, 'provider'>
+      secp256k1ManagedKeyInfo: ManagedKeyInfo


What is the reason we expect ManagedKeyInfos instead of kids for these? This is a public function and in most situations we would use a kid for them

Moved it to functions.ts as part of another comment

nklomp · 2024-05-10T08:12:18Z

packages/did-provider-ebsi/src/EbsiDidProvider.ts

+    )
+  }
+
+  private sendTransaction = async (args: { docTransactionResponse: Response; kid: string; id: number; apiOpts?: ApiOpts }, context: IContext) => {


Can we move a lot of these methods to functions elsewhere. They are now all in the Ebsi DID provider, which is following the Veramo contract. A lot of these methods are stateless and thus could become external functions, so they are reusable also outside the provider

Moved the Veramo unrelated methods to functions.ts

nklomp · 2024-05-10T08:13:16Z

packages/did-provider-ebsi/src/EbsiDidProvider.ts

    args: {
      did: string
      document: Partial<DIDDocument>
      options?: { [p: string]: any }
    },
-    context: IAgentContext<IKeyManager>
+    // TODO extract a type


Please create the type. The TODO was more work than creating the type itself

Extracted interface UpdateIdentifierParams

nklomp · 2024-05-10T08:15:10Z

packages/did-provider-ebsi/src/EbsiDidProvider.ts

@@ -117,14 +277,31 @@ export class EbsiDidProvider extends AbstractIdentifierProvider {
    throw Error(`Not (yet) implemented for the EBSI did provider`)
  }

-  updateIdentifier(
+  // TODO How does it work? Not inferable from the api: https://hub.ebsi.eu/apis/pilot/did-registry/v5/post-jsonrpc#updatebasedocument


What do you mean with not inferable? Of course I know what the did is. Also this is an optional method to implement once you want to work with passing in updated (partial) did documents. The regular way would be the add/remove service endpoint and add/remove keys methods

What will actually change in the did? That's the request body:

{ "jsonrpc": "2.0", "method": "updateBaseDocument", "params": [ { "from": "0xc9765eC58E49e6E386549CEAA34FB0d8bB69C320", "did": "did:ebsi:z23eve8qDNzYuNVqfSaErpuJ", "baseDocument": "{\"@context\":[\"https://www.w3.org/ns/did/v1\",\"https://w3id.org/security/suites/jws-2020/v1\"]}" } ], "id": 524 }

And there is no did document.

Thenit is likely this method should not have been implemented to begin with and throw an error instead. Veramo uses add and delete services and keys methods to manage updates to the identifier and did document. It recently added a method to pass in a new (partial) did document. If however a did provider cannot work with that it should throw an error.

Just throwing an error then

nklomp · 2024-05-10T08:16:54Z

packages/did-provider-ebsi/src/functions.ts

+  const { key, type } = args
+  switch (type) {
+    case 'Secp256k1': {
+      const bytes = getBytes('0x' + key.publicKeyHex, 'key')


Did you check whether we are not already storing the key in uncompressed form?

yes, our keys are 66 bytes long and start with 02 or 03. We are looking for a key that is 130 bytes long and start with 04

nklomp · 2024-05-10T08:18:51Z

packages/did-provider-ebsi/src/functions.ts

+      return SigningKey.computePublicKey(bytes, false)
+    }
+    case 'Secp256r1': {
+      const jwk: JsonWebKey = toJwk(key.publicKeyHex, type, { use: JwkKeyUse.Signature, key })


Okay. It seems they really are doing a conversion on the JWK to hex, which is rather odd without a codec to begin with and thus prone to errors across platforms, but so be it. Please create a comment explaining what is happening and why, as I would immediately start working on this code if an error occurs, because it looks so of

nklomp · 2024-05-10T08:20:22Z

packages/did-provider-ebsi/src/functions.ts

+  return base64url(sha256(data))
+}
+
+const check = (value: unknown, description: string) => {


Please make the name more descriptive like assertPresent.

renamed it to checkPresent

huh, checkPresent leaves the reader to guess whether it is returning booleans, doing some other logic or doing assertions. Why would you go with a less optimal name than suggested? The suggestion made really clear that the method would be throwing errors

renamed to assert present

nklomp · 2024-05-10T08:22:59Z

packages/did-provider-ebsi/src/services/EbsiRPCService.ts

+  token: string
+  apiOpts?: ApiOpts
+}): Promise<Response> => {
+  const { params, id, token, apiOpts } = args


Still the same comment as I made earlier about the method being different. We now have 4 or 5 times 3 lines which litteraly are the same except for the method value being different. Please create one single method and call that with the params, as this can only lead to bugs in th efuture

Refactored it to a single function callRpcMethod

nklomp · 2024-05-10T08:45:55Z

packages/did-provider-ebsi/package.json

-    "@sphereon/ssi-sdk-ext.did-resolver-ebsi": "workspace:*",
+    "@sphereon/ssi-sdk-ext.did-resolver-ebsi": "workspace:^",
+    "@sphereon/ssi-sdk-ext.key-utils": "workspace:^",
+    "@transmute/did-key-bls12381": "0.3.0-unstable.10",


Why is this dep added?

Not used anywhere, removed

…hrowing exception

zoemaas added 9 commits April 3, 2024 16:45

feat: Added secp256r1 key to createIdentifier() method

81fff51

chore: Associated the keys with the verification relationship

89968e5

refactor: Add trailing new line to types.ts

b988892

refactor: Prettier

e2b485b

chore: Added default verification relationships

d97e1eb

chore: Added tests and run prettier

0b44f54

refactor: Simplified structures, extracted more methods and run prettier

29e4f59

Merge branch 'develop' of github.com:Sphereon-Opensource/SSI-SDK-cryp…

99e8f39

…to-extensions into feature/SDK-8

feat: Implemented conversion of public keys, rpc service and document…

b0ac3b5

…ation

zoemaas marked this pull request as ready for review April 5, 2024 17:14

zoemaas requested a review from nklomp April 5, 2024 17:15

zoemaas self-assigned this Apr 5, 2024

zoemaas added 4 commits April 9, 2024 11:43

chore: Implemented the getDidDocument and listDids methods

36dc9b7

feat: Implemented integration of the ebsi rpc service with the ebsi d…

3c1ef0d

…id provider

fix: Bugfix creating eth transactions

1d2e04d

refactor: Added missing notBefore and notAfter properties to options

35f1f6b

nklomp requested changes Apr 28, 2024

View reviewed changes

zoemaas added 12 commits May 1, 2024 09:52

Merge branch 'develop' of github.com:Sphereon-Opensource/SSI-SDK-cryp…

32f3572

…to-extensions into feature/SDK-8 # Conflicts: # packages/did-provider-ebsi/package.json # packages/did-provider-ebsi/src/EbsiDidProvider.ts # packages/did-provider-ebsi/src/types.ts # pnpm-lock.yaml

refactor: Fixed merging issues

c91fdf2

refactor: Made token an argument of rpc functions

34b8d43

refactor: Refactored the formatEbsiPublicKey function

5d672b2

refactor: Refactored how the RPC url is built

12c1a14

refactor: Extracted a method from to build/populate fetch options

9ccacbf

refactor: Extracted nested string interpolation into ternary

74a1589

refactor: Extracted multiple if clauses to build the query string

317ac2d

refactor: Added query url to getUrls method

137504f

refactor: Run prettier

725dc37

refactor: Removed the jose dependency

773726f

refactor: updated pnpm-lock.yaml

7b141f8

nklomp requested changes May 10, 2024

View reviewed changes

zoemaas added 12 commits May 10, 2024 13:02

refactor: Added comment about the conversion of JWK in hex string.

dace576

refactor: Renamed function

615eb48

refactor: extracted interface from params of method createEbsiDid

a81ebfe

refactor: extracted interface from params of method updateIdentifier

f2efdcd

refactor: extracted constant baseDocument

9b2b5b7

refactor: removed the implementation of updateIdentifier method and t…

b2d685e

…hrowing exception

refactor: added new keyword to Errors that might be thrown

bb5e60f

refactor: moved Veramo unrelated methods to functions.ts

38a486f

refactor: Renamed checkPresent function to assertPresent

60602c4

refactor: Refactored the EBSI RPC methods

1c7680b

refactor: Removed unused dependency

dbb51ef

fix: Fixed broken tests

07d320a

nklomp approved these changes May 10, 2024

View reviewed changes

nklomp merged commit 381cc08 into develop May 10, 2024
2 checks passed

nklomp deleted the feature/SDK-8 branch May 10, 2024 19:04

feature/SDK-8 #27

feature/SDK-8 #27

Conversation

zoemaas commented Apr 5, 2024

nklomp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoemaas May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoemaas May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nklomp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoemaas May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nklomp May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoemaas May 1, 2024 •

edited

Loading

zoemaas May 1, 2024 •

edited

Loading

zoemaas May 10, 2024 •

edited

Loading

nklomp May 10, 2024 •

edited

Loading