Change NodeId from encoded string to an opaque alias of Id which is a typed array from js-id

[tegefaulkes]

Description

This PR will focus on applying the new NodeId which is an opaque alias of Id (which itself is an alias of IdInternal & number).

1. Add some of the encoding wrappers into the IdInternal class, this requires a new PR in js-id. Include toMultibase and toUUID and toBuffer as instance methods, and the fromString, fromMultibase, fromUUID, and fromBuffer as static methods (these represent alternatives of creating the IdInternal). Make sure that these functions return Id as the type and not IdInternal.
2. This PR will focus on applying the new NodeId which is an opaque alias of Id (which itself is an alias of IdInternal & number).
3. All the areas which is currently using NodeId is expecting an encoded string. These will need to be changed to expect a sort of type NodeIdEncoded = string;, you still need a nodesUtils.encodeNodeId.
4. The encoding functions and decoding functions of NodeId should be inside nodes/utils.ts. This should be similar to how I'm doing it for VaultId. An alternative might be to create class extension or generics, but this seems complicated.
5. Raw usage of the NodeId will mostly occur inside the nodes domain, in particular the NodeGraph can use the Uint8Array directly. Note that the js-db currently doesn't yet allow direct usage of the Id, because it requires a Buffer type. However there is an issue Use bufferWrap to support ArrayBuffer, Uint8Array and Buffer js-db#3 to allow it to eventually take Id, so for now, just use idUtils.toBuffer and idUtils.fromBuffer when interacting with the DB.
6. Once this PR is merged into master, rebase Introducing vault sharing and the restrictions of pulling and cloning with vault permissions #266 and Extracting Node Connection Management out of NodeManager to NodeConnectionManager #310 on top to benefit from it.

Issues Fixed

• Fixes Refactor NodeId as a proxy/singleton instance #254
• Fixes Canonicalise node ID representation #261
• Related Review js-id usage for NodeId, VaultId, NotificationId, PermId, ClaimId, Gestalts and GenericIdTypes.ts #299

Tasks

1. Add some of the encoding wrappers into the IdInternal class, this requires a new PR in js-id. Include toMultibase and toUUID and toBuffer as instance methods, and the fromString, fromMultibase, fromUUID, and fromBuffer as static methods (these represent alternatives of creating the IdInternal). Make sure that these functions return Id as the type and not IdInternal. - Update to 3.2.0 of js-id
2. Update NodeId type to use an opaque alias of Id.
3. Update all uses of NodeId to use the new type where possible.
4. Where a encoded string of NodeId is required, use a NodeIdEncoded instead.
5. Don't get the NodeId from the Certificate. get the Public key from the KeyManager and convert it from that.
6. [ ] PolykeyAgent should store the NodeId. It should be updated via the event bus. ~ - stay with using this.keyManager.getNodeId().
7. [ ] Anything that needs the NodeId should take a lambda that fetches it from the PolykeyAgent. - stick with injecting KeyManager where it is needed
8. Add NodeId validation to RPC services and possibly bin commands.
9. review types and utils imports.

example of usage here.
#254 (comment)

Areas that need changes:

1. Status needs the string encoded to put into the status JSON file - Status file does indeed to use the encoded string, but StatusInfo should still use the NodeId and not NodeIdEncoded because we want to do any validation on the boundary and ensure that our internal types in the context of the program uses types that imply that the validation occurred already, and therefore the nodeId is indeed a valid node id.
2. GRPCClient needs the string encoded to pass as a URL parameter for the proxy
3. keymanager certificate needs to have string encoded node id
4. sigchain claims needs to have string encoded node id
5. GestaltGraph.ts needs to use the encoded string of the NodeId - its just because it is designed like this it's due to the GestaltKey
6. The NodeInfo for now has to use NodeIdEncoded because it is being put into the database as a value as part of a POJO which gets JSON encoded. Plus that GestaltGraph and Discovery uses it as a string, but in the future it may be better done as NodeId which follows the same reason as 1. about validation being done on the boundary.

@tegefaulkes please flesh the above out.

Guidelines:

1. Use NodeId binary form where possible
2. Use NodeId encoded as multibase when it goes to a file, to outside network, to the console/terminal
3. Use NodeId binary string if it's put into a private POJO... - do this only when you're really sure that this will never be seen by anything else
4. Use NodeId buffers when using DB.
5. when comparing two ids for equality convert them to strings with .toString()

Unresolved tests addressed in other PRs:

These were failing in the commit I branched off of. May be fix in #310 or #308

• ping.test.ts
• find.test.ts
• notificationsManager.test.ts

relevant to the vaults domain.

• vaultManager.test.ts

Part of nodes and #310

• NodeConnection.test.ts
• nodeManager.test.ts

Final checklist

• Domain specific tests
• Full tests
• Updated inline-comment documentation
• Lint fixed
• Squash and rebased
• Sanity check the final build

[tegefaulkes]

I think this PR will fix #261 as well.

[CMCDragonkai]

Are you going to create the PR on js-id too to include the additional static methods and instance methods for IdInternal there?

[CMCDragonkai]

Are we still going to use a lambda and pass that around instead of passing the key manager to get the node id? I have a feeling that this isn't providing enough benefit to bother doing it. So I'm keeping how we are passing KeyManager everywhere.

[CMCDragonkai]

I suggest these encoding and decoding utilities to be put into nodes/utils.ts:

function encodeNodeId(nodeId: NodeId): string {
  return idUtils.toMultibase(nodeId, 'base32hex');
}

function decodeNodeId(nodeIdString: string): NodeId | undefined {
  return idUtils.fromMultibase(nodeIdString) as NodeId | undefined;
}

Where type NodeId = Opaque<'NodeId', Id>;.

[CMCDragonkai]

Will need this merged for me to finish off #266. I'm already put the above functions in, but overall NodeId changes will be significant across the codebase.

[tegefaulkes]

One annoying quirk of using NodeId as a key to a map or object. it is seen as a string when iterated over.

const nodeIds: Record<NodeId, null>
for (const nodeId in nodeIds) {
    // nodeId is a string here.
    const newNodeId: NodeId = IdInternal.fromString(nodeId)!;
}

[CMCDragonkai]

That's to be expected. POJO keys are always strings.

[CMCDragonkai]

Is there any other database of nodes other than NodeGraph? Remember the node id should used as a buffer key during lookup.

The gestalt graph should also be using the encoded NodeIds because they are likely to be shown to the end user in the GUI.

[tegefaulkes]

I used toBuffer() in a few places where it was being used in the db. so far that has only been the ACL.

I've run into the gestalt graph already, I'll have a deeper look at it at a later stage.

[tegefaulkes]

Most of the changes are done.

• I need to take a closer look a GestaltGraph and make sure it's using the encoded form of NodeId.
• A lot of test may be failing due to comparing NodeIds with toEqual(). Should be using toStrictEqual().
• Something weird is going on with GRPCClientAgent failing to extend GRPCClient<AgentServiceClient> in some tests with TypeError: Class extends value undefined is not a constructor or null.

The error is very peculiar

    TypeError: Class extends value undefined is not a constructor or null

      19 | interface GRPCClientAgent extends CreateDestroy {}
      20 | @CreateDestroy()
    > 21 | class GRPCClientAgent extends GRPCClient<AgentServiceClient> {
         |                               ^
      22 |   /**
      23 |    * Creates GRPCClientAgent
      24 |    * This connects to the agent service

      at Object.<anonymous> (src/agent/GRPCClientAgent.ts:21:31)
      at Object.<anonymous> (src/agent/index.ts:2:1)

[CMCDragonkai]

The toEqual should work the same as toStrictEqual. Maybe you mean toBe.

Also is this PR rebased on master? It seems there are conflicting files.

[tegefaulkes]

My mistake. Anything comparing two objects such as the NodeId with toBe should use ToEqual or toStringEqual instead.
toBe will fail with;

Error: expect(received).toBe(expected) // Object.is equality

If it should pass with deep equality, replace "toBe" with "toStrictEqual"

Expected: [188, 56, 23, 20, 225, 85, 252, 109, 252, 39, 99, 174, 160, 84, 204, 189, 95, 29, 30, 165, 143, 100, 165, 65, 157, 237, 190, 55, 12, 47, 227, 72]
Received: serializes to the same string

[tegefaulkes]

Some problems with the sigChain that need sorting out. One small problem is that it expected strings for the NodeId in the ClaimData and did some shortcuts with that POJO internally. This makes it a little tricky to neatly convert the NodeIds into strings internally. so for simplicity sake I'm going to have it use NodeInEncoded for the NodeIds instead.

[CMCDragonkai]

Sigchain claims are JSON, so it will make sense to store them as encoded strings and not the original buffer. Unless of course there's an on-demand conversion when you read it out of the DB. But if the claims are stored as POJO values in the DB then storing it as an encoded string is fine.

On 24 January 2022 11:40:36 am AEDT, Brian Botha ***@***.***> wrote: Some problems with the sigChain that need sorting out. One small problem is that it expected strings for the `NodeId` in the `ClaimData` and did some shortcuts with that POJO internally. This makes it a little tricky to neatly convert the `NodeId`s into strings internally. so for simplicity sake I'm going to have it use `NodeInEncoded` for the `NodeIds` instead. -- Reply to this email directly or view it on GitHub: #318 (comment) You are receiving this because you commented. Message ID: ***@***.***>

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[tegefaulkes]

Most domains have been fixed up. currently failing test files are;

• NotificationsManager.test.ts
• bin/agent/status.test.ts
• notifications/utils.test.ts
• VaultOps.test.ts - Problem with mocking?
• VaultInternal.test.ts - Problem with mocking?
• vaultManager.test.ts
• notifications.test.ts
• bin/itentitites/identities.test.ts
• bin/nodes/claim.test.ts
• bin/keys/renew.test.ts
• bin/keys/reset
• client/service/keysKeyPairRenew.test.ts
• bin/nodes/add.test.ts
• client/service/keysKeyPairReset.test.ts
• client/rpcGestalts.test.ts
• client/service/notificationsSend.test.ts
• nodes/NodeGraph.test.ts - Random failures. fixed in nodes PR

[CMCDragonkai]

Just a reminder, if there are places where you need to decode an encoded node ID, and it may return undefined, but you know for sure that it won't return undefined because you created it or took it from a trusted source, like for example the database. Then you can just use ! to assert that it will be defined when you decode it.

We should however check for undefined whenever we are getting a NodeId from an untrusted source, and exceptions should be thrown on the boundary.

[CMCDragonkai]

Did you remove the encoding/decoding functions from NodeGraph, node that you have the ability to use NodeId as a buffer directly? This includes:

• nodeIdtoU8 in src/nodes/utils.ts

[tegefaulkes]

Yes, they have been removed.

[joshuakarp]

I need to continue to review the usage of decodeNodeId and the assertions made with respect to the broader context of nodes, gestalts, etc.

[tegefaulkes]

Barring any major that comes up in review. This should be ready to merge early tomorrow.

[CMCDragonkai]

MatrixAI/js-id#12 got merged and released as 3.3.0. There are some important changes there:

1. The ability to use toJSON and fromJSON when dealing with JSON marshalling Change NodeId from encoded string to an opaque alias of Id which is a typed array from js-id #318 (review) - note that there we don't need it for now, but it may be useful in the future if NodeInfo changes or other places require it.
2. The equals method is faster than doing string comparison. So any usage of id string comparison should use id.equals instead. Map will still use pointer comparison, POJOs will still use binary string comparison. The equals does not overload the == operator.
3. The fromBuffer and fromString both do not return undefined, so any usage of ! assertions should not be needed.

[CMCDragonkai]

What test failures are left to other PRs? Can you specify them in the PR description as well so we don't lose them @tegefaulkes.

[tegefaulkes]

NodeId validation is now done inside of nodesUtils.decodeNodeId(). I've fixed up all of the NodeIds in tests to use a random NodeId generator. just need to check the tests are still working.

[CMCDragonkai]

Fleshed out spec about how NodeId is used regarding StatusInfo and NodeInfo (and there being an exception for NodeInfo atm).

[tegefaulkes]

Tests that are not getting resolved here are;

These were failing in the commit I branched off of.

• ping.test.ts
• find.test.ts
• notificationsManager.test.ts

relevant to the vaults domain.

• vaultManager.test.ts

Part of nodes and #310

• NodeConnection.test.ts
• nodeManager.test.ts