Growing the Gestalt Graph and Implementing Social Discovery

[emmacasolin]

Description

The PR will change the way that the gestalt graph grows and shrinks by exposing this to the CLI. The identities trust and vaults share commands will add the trusted node/identity to the gestalt graph and the discovery domain crawl these to build the gestalt graph. This will involve converting the Discovery domain from CreateDestroy to CreateDestroyStartStop, such that it has underlying state (a queue of nodes/identities to crawl) and constantly discovers gestalts in the background.

This PR will also expose indentitiesInfoGetConnected to the CLI such that users can search providers for identities, which can then subsequently be trusted and crawled.

Issues Fixed

• Fixes Implement Social Discovery to add found DIs to the Gestalt Graph #316
• Allows CLI and Client & Agent Service test splitting #311 to continue with splitting gestalts tests

Tasks

1. Create a CLI command for identitiesInfoGetConnected
2. Create separate handlers for identities trust which are injected with the GestaltGraph to allow trusted nodes/identities to be added to the gestalt graph
3. Inject GestaltGraph into the handler for vaults share to allow trusted nodes/identities to be added to the gestalt graph (wait until Introducing vault sharing and the restrictions of pulling and cloning with vault permissions #266 is merged)
4. Convert the Discovery domain to CreateDestroyStartStop and add a queue of nodes/identities to discover
5. Implement the ability for discovery to happen continuously in the background once start is called
6. Full tests for identitiesInfoGetConnected and discovery tests

Final checklist

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

[emmacasolin]

Previously I had been confused as to why some of the Provider methods have both an authIdentityId and identityId parameter but I've just figured it out - authIdentityId is always your own, authenticated identity id (since you need to be authenticated in order to access sensitive data) and identityId is the identity that you want to retrieve data about. So authIdentityId and identityId are only the same if you're trying to retrieve data about yourself.

[emmacasolin]

While I had originally thought that we would need to call discoverGestaltByIdentity() in order to add the claims on found identities to our gestalt graph, I realise now that we could get around this by simply including the claims when we set the identity into our gestalt graph since this method takes an IdentityInfo as its parameter, which is a conjunction of IdentityData:

type IdentityData = {
  providerId: ProviderId;
  identityId: IdentityId;
  name?: string;
  email?: string;
  url?: string;
};

and IdentityClaims (a map of claim ids to identity -> keynode claims). We can get these claims straight from the provider by calling getClaims().

The benefit to doing it this way is that we don't need to inject Discovery into identitiesInfoGetConnected or set up an event to call it automatically, however, the down-side is that getClaims() does not verify the signatures on the claims (as opposed to discoverGestaltByIdentity(), which does).

[emmacasolin]

In terms of the CLI command for identitiesInfoGetConnected, I think that the current command for identitiesInfoGet should be repurposed for identitiesInfoGetConnected. identitiesInfoGet is currently called by CommandSearch, the description of which is 'Searches a Provider for any Connected Identities', however, this name and description is better suited for identitiesInfoGetConnected. identitiesInfoGet simply returns the first authenticated identity id on the provider, so what would be an appropriate CLI command for that? Is one even necessary?

[emmacasolin]

Since we want to check that an identity is authenticated (similarly for nodes being valid) before adding it to our gestalt graph, we would need to throw some sort of error back to the client if the provided identity is invalid/unauthenticated.

There is an existing error ErrorClientInvalidProvider for if the given provider is not supported, so would ErrorClientInvalidIdentity be an appropriate error to throw if the identity you want to trust is not authenticated?

[emmacasolin]

Since identities trust is now setting nodes/identities into the gestalt graph, should identities untrust unset them? Otherwise it's not a reversible operation.

[CMCDragonkai]

Previously I had been confused as to why some of the Provider methods have both an authIdentityId and identityId parameter but I've just figured it out - authIdentityId is always your own, authenticated identity id (since you need to be authenticated in order to access sensitive data) and identityId is the identity that you want to retrieve data about. So authIdentityId and identityId are only the same if you're trying to retrieve data about yourself.

Yep this is worth giving a comment on.

[CMCDragonkai]

Since identities trust is now setting nodes/identities into the gestalt graph, should identities untrust unset them? Otherwise it's not a reversible operation.

No, untrusting doesn't delete it from the GG, it just removes the trust relationship. GG garbage collection can be a separate issue.

[CMCDragonkai]

Since we want to check that an identity is authenticated (similarly for nodes being valid) before adding it to our gestalt graph, we would need to throw some sort of error back to the client if the provided identity is invalid/unauthenticated.

There is an existing error ErrorClientInvalidProvider for if the given provider is not supported, so would ErrorClientInvalidIdentity be an appropriate error to throw if the identity you want to trust is not authenticated?

Not sure what you mean by checking if it is authenticated. Yes you need to check if the identity exists. And in some cases you need to check if cryptolink claim that led you there is valid. If if someone is specifically pointing to a given identity to add to the gestalt graph, there is no cryptolink to check.

As for exceptions, new exceptions could be added into the identities domain when checking for the existence of an identity. Perhaps a new method on our identity providers? I reckon getIdentityInfo is sufficient. Also in this case no exception is needed if you're returning the info only, simply return the sentinel value. Only have exceptions if it the returned value is expected to exist and carried to another procedure.

[CMCDragonkai]

In terms of the CLI command for identitiesInfoGetConnected, I think that the current command for identitiesInfoGet should be repurposed for identitiesInfoGetConnected. identitiesInfoGet is currently called by CommandSearch, the description of which is 'Searches a Provider for any Connected Identities', however, this name and description is better suited for identitiesInfoGetConnected. identitiesInfoGet simply returns the first authenticated identity id on the provider, so what would be an appropriate CLI command for that? Is one even necessary?

Can you show this through possible PK CLI commands. It's a bit hard to visualise the workflow.

[CMCDragonkai]

While I had originally thought that we would need to call discoverGestaltByIdentity() in order to add the claims on found identities to our gestalt graph, I realise now that we could get around this by simply including the claims when we set the identity into our gestalt graph since this method takes an IdentityInfo as its parameter, which is a conjunction of IdentityData:

type IdentityData = {
  providerId: ProviderId;
  identityId: IdentityId;
  name?: string;
  email?: string;
  url?: string;
};

and IdentityClaims (a map of claim ids to identity -> keynode claims). We can get these claims straight from the provider by calling getClaims().

The benefit to doing it this way is that we don't need to inject Discovery into identitiesInfoGetConnected or set up an event to call it automatically, however, the down-side is that getClaims() does not verify the signatures on the claims (as opposed to discoverGestaltByIdentity(), which does).

I think we discussed over Skype. The discovery module is like a background spidering process. So we should be able to push a job/task onto the queue which will be iterated on in the background.

[emmacasolin]

Not sure what you mean by checking if it is authenticated. Yes you need to check if the identity exists. And in some cases you need to check if cryptolink claim that led you there is valid. If if someone is specifically pointing to a given identity to add to the gestalt graph, there is no cryptolink to check.

Yeah just checking that an identity is valid. If we want to trust an identity we first need to add it to the gestalt graph, but we need to check it's a valid identity before doing so since it's just user-entered data.

As for exceptions, new exceptions could be added into the identities domain when checking for the existence of an identity. Perhaps a new method on our identity providers? I reckon getIdentityInfo is sufficient. Also in this case no exception is needed if you're returning the info only, simply return the sentinel value. Only have exceptions if it the returned value is expected to exist and carried to another procedure.

There's already a method called getIdentityData() which does this, however it just returns undefined if the identity can't be found. I could make it return an error instead though? It's only used inside discovery but I could just catch the error there instead of checking if identityData == null. There does need to be an error thrown when it gets called by identities trust since if the identity doesn't exist we don't want to add it to the gestalt graph, which means the permission can't be set.

[emmacasolin]

In terms of the CLI command for identitiesInfoGetConnected, I think that the current command for identitiesInfoGet should be repurposed for identitiesInfoGetConnected. identitiesInfoGet is currently called by CommandSearch, the description of which is 'Searches a Provider for any Connected Identities', however, this name and description is better suited for identitiesInfoGetConnected. identitiesInfoGet simply returns the first authenticated identity id on the provider, so what would be an appropriate CLI command for that? Is one even necessary?

Can you show this through possible PK CLI commands. It's a bit hard to visualise the workflow.

Yeah sure. So currently the identities search [providerId] command returns your own identity id (if you have any authenticated identities for the provider) or if you have no authenticated identities it returns nothing and logs 'No Connected Identities found for Provider'.

My suggestion is that instead of returning your own identity id, identities search is used for finding connected identities (as its description suggests). So example usage would be something like:

pk identities search [providerId] [...searchTerms]

Provider: [providerId] // this probably isn't needed considering you have to pass in a provider to search on so all identities will be on that provider
Identity: [identityId]
Name: [name]
Email: [email]
URL: [url]

... repeat for remaining connected identities

-------------------------------------------------------------------------

pk identities search [providerIdInvalid] [...searchTerms]

INFO: No Connected Identities found for Provider

I can't think of a use case for needing to retrieve your own authenticated identity id(s), unless you wanted to check if you were authenticated? But in that case it should probably be a functionality that's part of identities authenticate. The identities search command doesn't seem like the right place for that.

[emmacasolin]

There's actually a problem with setting identities into the GG in the same handler as trusting the identity. This is because you can't just trust an identity on its own - you're trusting a gestalt BY one of its identities. So if an identity has only just been set into the GG (and is not connected to any nodes, since we expect this to be done in the background by Discovery), then we won't be able to set permissions for it. This is only a problem for identities (not nodes), but I think the process of trusting should be the same for both identities and nodes so if it doesn't work for identities we should come up with a different solution for both.

[CMCDragonkai]

Not sure what you mean by checking if it is authenticated. Yes you need to check if the identity exists. And in some cases you need to check if cryptolink claim that led you there is valid. If if someone is specifically pointing to a given identity to add to the gestalt graph, there is no cryptolink to check.

Yeah just checking that an identity is valid. If we want to trust an identity we first need to add it to the gestalt graph, but we need to check it's a valid identity before doing so since it's just user-entered data.

As for exceptions, new exceptions could be added into the identities domain when checking for the existence of an identity. Perhaps a new method on our identity providers? I reckon getIdentityInfo is sufficient. Also in this case no exception is needed if you're returning the info only, simply return the sentinel value. Only have exceptions if it the returned value is expected to exist and carried to another procedure.

There's already a method called getIdentityData() which does this, however it just returns undefined if the identity can't be found. I could make it return an error instead though? It's only used inside discovery but I could just catch the error there instead of checking if identityData == null. There does need to be an error thrown when it gets called by identities trust since if the identity doesn't exist we don't want to add it to the gestalt graph, which means the permission can't be set.

Returning undefined is the correct response here. We don't use exceptions to indicate non-existence if the command is about fetching something. Value based errors is better in these scenarios.

An exception could be thrown inside the service handler as that's where the exception is, the expectation that identity exists is not fulfilled.

[CMCDragonkai]

There's actually a problem with setting identities into the GG in the same handler as trusting the identity. This is because you can't just trust an identity on its own - you're trusting a gestalt BY one of its identities. So if an identity has only just been set into the GG (and is not connected to any nodes, since we expect this to be done in the background by Discovery), then we won't be able to set permissions for it. This is only a problem for identities (not nodes), but I think the process of trusting should be the same for both identities and nodes so if it doesn't work for identities we should come up with a different solution for both.

A gestalt can be consist of just a single identity. In fact all vertexes start out single and then progressively build up relationships after that. So if the gestalt is just 1 identity then a permission can still be added. When that gestalt builds, the permissions are merged together.

[CMCDragonkai]

In terms of the CLI command for identitiesInfoGetConnected, I think that the current command for identitiesInfoGet should be repurposed for identitiesInfoGetConnected. identitiesInfoGet is currently called by CommandSearch, the description of which is 'Searches a Provider for any Connected Identities', however, this name and description is better suited for identitiesInfoGetConnected. identitiesInfoGet simply returns the first authenticated identity id on the provider, so what would be an appropriate CLI command for that? Is one even necessary?

Can you show this through possible PK CLI commands. It's a bit hard to visualise the workflow.

Yeah sure. So currently the identities search [providerId] command returns your own identity id (if you have any authenticated identities for the provider) or if you have no authenticated identities it returns nothing and logs 'No Connected Identities found for Provider'.

My suggestion is that instead of returning your own identity id, identities search is used for finding connected identities (as its description suggests). So example usage would be something like:

pk identities search [providerId] [...searchTerms]

Provider: [providerId] // this probably isn't needed considering you have to pass in a provider to search on so all identities will be on that provider
Identity: [identityId]
Name: [name]
Email: [email]
URL: [url]

... repeat for remaining connected identities

-------------------------------------------------------------------------

pk identities search [providerIdInvalid] [...searchTerms]

INFO: No Connected Identities found for Provider

I can't think of a use case for needing to retrieve your own authenticated identity id(s), unless you wanted to check if you were authenticated? But in that case it should probably be a functionality that's part of identities authenticate. The identities search command doesn't seem like the right place for that.

I think there are several possible uses here:

• Looking up a specific identity, this is where you know the <ProviderId> and the <IdentityId>
• Looking up all connected identities across all providers.
• Looking up all connected identities for a single provider.
• Searching across all providers for a given search term, this is where you know the <ProviderId>
• Searching across a single provider for a given search term.

Should a single command be capable of doing them all or separate these ideas?

# --provider could be optional and limit the search to to a specific set of providers (see commander options for this)
# search is limited to "connected identities" by default, but an additional flag can be set to be unbound, and search any identity that exists - certain providers may not allow this to occur
pk identities search --provider <ProviderId> --provider <ProviderId> term1 term2
pk identities search --provider <ProviderId> <ProviderId> term1 term2
pk identities search term1 term2
pk identities search --disconnected term1 term2

# there are other commands that are available too
pk identities list
pk identities discover

# other possible keywords that haven't been used
pk identities find
pk identities lookup

BTW, I remember now that we made a decision to group all gestalt-related functionality within the identities subcommand. So there is no pk gestalts subcommand. This was a UX decision so that users don't have to know what "gestalt" means. We can reverse this decision, but for now most of what we want to do will be identity focused.

[emmacasolin]

There's actually a problem with setting identities into the GG in the same handler as trusting the identity. This is because you can't just trust an identity on its own - you're trusting a gestalt BY one of its identities. So if an identity has only just been set into the GG (and is not connected to any nodes, since we expect this to be done in the background by Discovery), then we won't be able to set permissions for it. This is only a problem for identities (not nodes), but I think the process of trusting should be the same for both identities and nodes so if it doesn't work for identities we should come up with a different solution for both.

A gestalt can be consist of just a single identity. In fact all vertexes start out single and then progressively build up relationships after that. So if the gestalt is just 1 identity then a permission can still be added. When that gestalt builds, the permissions are merged together.

At the moment, the GG pretty much just acts as an interface for the ACL. So it won't allow you to set permissions for just an identity since the ACL only stores permissions for nodes and vaults. For example this is the method for setting permissions by identity (this is the closest method in the GG to setting permissions for an identity, which I think is slightly different):

@ready(new gestaltsErrors.ErrorGestaltsGraphNotRunning())
  public async setGestaltActionByIdentity(
    providerId: ProviderId,
    identityId: IdentityId,
    action: GestaltAction,
  ): Promise<void> {
    return await this._transaction(async () => {
      return await this.acl._transaction(async () => {
        const identityKey = gestaltsUtils.keyFromIdentity(
          providerId,
          identityId,
        );
        if (
          (await this.db.get<IdentityInfo>(
            this.graphIdentitiesDbDomain,
            identityKey,
          )) == null
        ) {
          throw new gestaltsErrors.ErrorGestaltsGraphIdentityIdMissing();
        }
        const gestaltKeySet = (await this.db.get(
          this.graphMatrixDbDomain,
          identityKey,
        )) as GestaltKeySet;
        let nodeId: NodeId | undefined;
        for (const nodeKey in gestaltKeySet) {
          nodeId = gestaltsUtils.ungestaltKey(nodeKey as GestaltNodeKey).nodeId;
          break;
        }
        // If there are no linked nodes, this cannot proceed
        if (nodeId == null) {
          throw new gestaltsErrors.ErrorGestaltsGraphNodeIdMissing();
        }
        await this.acl.setNodeAction(nodeId, action);
      });
    });
  }

It needs to find a linked node to set the permissions for. If we want to be able to set permissions for just an identity on its own then we need to rethink the current methods in the GG.

[emmacasolin]

BTW, I remember now that we made a decision to group all gestalt-related functionality within the identities subcommand. So there is no pk gestalts subcommand. This was a UX decision so that users don't have to know what "gestalt" means. We can reverse this decision, but for now most of what we want to do will be identity focused.

I was thinking about this and was wondering if maybe the gestalts-related behaviour could go under something like permissions? So example commands would be

// Allow permission on gestalt by node/identity
pk permissions allow <gestaltId> <permissions>

// Disallow permission on gestalt by node/identity
pk permissions disallow <gestaltId> <permissions>

// Discover a gestalt by node/identity
pk permissions discover <gestaltId>

// Get a gestalt by node/identity
pk permissions get <gestaltId>

// Get all gestalts (and associated permissions)
pk permissions list

// Get the permissions for a gestalt by node/identity
pk permissions permissions <gestaltId>

// Trust a gestalt by node/identity
pk permissions trust <gestaltId>

// Untrust a gestalt by node/identity
pk permissions untrust <gestaltId>

It works better for some than others, but I think it's still clearer than using identities, expecially considering a lot of this behaviour operates on nodes.

[emmacasolin]

I think there are several possible uses here:

• Looking up a specific identity, this is where you know the <ProviderId> and the <IdentityId>
• Looking up all connected identities across all providers.
• Looking up all connected identities for a single provider.
• Searching across all providers for a given search term, this is where you know the <ProviderId>
• Searching across a single provider for a given search term.

Should a single command be capable of doing them all or separate these ideas?

# --provider could be optional and limit the search to to a specific set of providers (see commander options for this)
# search is limited to "connected identities" by default, but an additional flag can be set to be unbound, and search any identity that exists - certain providers may not allow this to occur
pk identities search --provider <ProviderId> --provider <ProviderId> term1 term2
pk identities search --provider <ProviderId> <ProviderId> term1 term2
pk identities search term1 term2
pk identities search --disconnected term1 term2

# there are other commands that are available too
pk identities list
pk identities discover

# other possible keywords that haven't been used
pk identities find
pk identities lookup

How's something like this?

Usage: polykey identities search [options] <searchTerms...>

Searches a Provider for any Connected Identities

Arguments:
  searchTerms                  (optional) Search parameters to apply to connected identities

Options:
  -pi, --provider-id <id>      Digital identity provider(s) to search on
  -i, --identity-id <id>       Name of the digital identity to search for
  -d, --disconnected           Include disconnected identities in search
  -n, --number <number>        Number of search results to display (default: "all")

Example usage:

// Look for a specific identity given the provider id and identity id
pk identities search --provider-id <providerId> --identity-id <identityId>

// Look up all connected identities across all providers
pk identities search

// Look up all connected identities for a single provider
pk identities search --provider-id <providerId>

// Search across all providers for a given search term
pk identities search [term1] [term2]

// Search across a single provider for a given search term
pk identities search --provider-id <providerId> [term1] [term2]

// Unbound search for a particular identity across all providers
pk identities search --identity-id <identityId> --disconnected

// Unbounded search across two providers for a given search term, displaying 20 (or fewer) results
pk identities search --provider-id <providerId> <providerId> --number 20 --disconnected [searchTerm]

[CMCDragonkai]

Interesting. So this means using --identity-id means looking up a specific identity rather than searching.

But if you combine it with search terms it would have to apply the filter to the resulting list of identities even if that list only has 1 identity.

I suggest that --identity-id be -iias words are always single characters.

Also this command should stream out the results which means output formatter has to be used in a loop. That is rather than outputting the entire array in own go, you would output it line by line in a stream as you read off from the grpc stream.

[CMCDragonkai]

Also standardised names for limiting results is --limit.

[CMCDragonkai]

BTW, I remember now that we made a decision to group all gestalt-related functionality within the identities subcommand. So there is no pk gestalts subcommand. This was a UX decision so that users don't have to know what "gestalt" means. We can reverse this decision, but for now most of what we want to do will be identity focused.

I was thinking about this and was wondering if maybe the gestalts-related behaviour could go under something like permissions? So example commands would be

// Allow permission on gestalt by node/identity
pk permissions allow <gestaltId> <permissions>

// Disallow permission on gestalt by node/identity
pk permissions disallow <gestaltId> <permissions>

// Discover a gestalt by node/identity
pk permissions discover <gestaltId>

// Get a gestalt by node/identity
pk permissions get <gestaltId>

// Get all gestalts (and associated permissions)
pk permissions list

// Get the permissions for a gestalt by node/identity
pk permissions permissions <gestaltId>

// Trust a gestalt by node/identity
pk permissions trust <gestaltId>

// Untrust a gestalt by node/identity
pk permissions untrust <gestaltId>

It works better for some than others, but I think it's still clearer than using identities, expecially considering a lot of this behaviour operates on nodes.

Not all gestalt related work has to do with permissions. And I think we do have an ACL subcommand. To prevent confusion it should either stay in identities or become its own subcommand as gestalts.

[CMCDragonkai]

Ok I must have forgotten how ACL works. But yes for now we can only set permissions on nodes. Therefore the identities we set permissions for must be augmented. We are meant to detect if the identity is augmented yet. I want to separate the job of finding identities from the job of querying the gestalt graph. So one can use the identity provider to know if an identity is augmented. It's the discovery modules job to build the gestalt graph and maintain it.

So if the identity is augmented you can give it a permission. If it isn't yet, you can't do it yet. Btw does this also mean notification permissions cannot be set for an identity too?

[emmacasolin]

Ok I must have forgotten how ACL works. But yes for now we can only set permissions on nodes. Therefore the identities we set permissions for must be augmented. We are meant to detect if the identity is augmented yet. I want to separate the job of finding identities from the job of querying the gestalt graph. So one can use the identity provider to know if an identity is augmented. It's the discovery modules job to build the gestalt graph and maintain it.

So if the identity is augmented you can give it a permission. If it isn't yet, you can't do it yet. Btw does this also mean notification permissions cannot be set for an identity too?

Yeah that's correct - notification permissions cannot be set for an (unaugmented) identity. So how should setting permissions for identities work for the CLI? Because right now you can run identities trust <nodeId> to add a node to your GG and set the notify permission, however running identities trust <identityId> adds the identity to the GG but then throws an error when trying to set the notify permission.

[emmacasolin]

BTW, I remember now that we made a decision to group all gestalt-related functionality within the identities subcommand. So there is no pk gestalts subcommand. This was a UX decision so that users don't have to know what "gestalt" means. We can reverse this decision, but for now most of what we want to do will be identity focused.

I was thinking about this and was wondering if maybe the gestalts-related behaviour could go under something like permissions? So example commands would be

// Allow permission on gestalt by node/identity
pk permissions allow <gestaltId> <permissions>

// Disallow permission on gestalt by node/identity
pk permissions disallow <gestaltId> <permissions>

// Discover a gestalt by node/identity
pk permissions discover <gestaltId>

// Get a gestalt by node/identity
pk permissions get <gestaltId>

// Get all gestalts (and associated permissions)
pk permissions list

// Get the permissions for a gestalt by node/identity
pk permissions permissions <gestaltId>

// Trust a gestalt by node/identity
pk permissions trust <gestaltId>

// Untrust a gestalt by node/identity
pk permissions untrust <gestaltId>

It works better for some than others, but I think it's still clearer than using identities, expecially considering a lot of this behaviour operates on nodes.

Not all gestalt related work has to do with permissions. And I think we do have an ACL subcommand. To prevent confusion it should either stay in identities or become its own subcommand as gestalts.

There isn't an ACL subcommand yet as far as I can see, but I agree that permissions probably isn't the right word. But this PR probably isn't the place to do a naming review.

[emmacasolin]

Interesting. So this means using --identity-id means looking up a specific identity rather than searching.

But if you combine it with search terms it would have to apply the filter to the resulting list of identities even if that list only has 1 identity.

I suggest that --identity-id be -iias words are always single characters.

Also this command should stream out the results which means output formatter has to be used in a loop. That is rather than outputting the entire array in own go, you would output it line by line in a stream as you read off from the grpc stream.

Incorporating this feedback:

Usage: polykey identities search [options] <searchTerms...>

Searches a Provider for any Connected Identities

Arguments:
  searchTerms                  (optional) Search parameters to apply to connected identities

Options:
  -pi, --provider-id <id>      Digital identity provider(s) to search on
  -ii, --identity-id <id>      Name of the digital identity to search for
  -d, --disconnected           Include disconnected identities in search
  -l, --limit <number>         Limit the number of search results to display to a specific number

And yes, using --identity-id would just be looking up a specific identity, however if you didn't include --provider-id then you could search for the same handle across all providers.

[emmacasolin]

What about the last tickbox?

The "sanity check the final build" one? I'm not entirely sure what this refers to, but my assumption is it's the pipeline, in which case I've checked over that.

BTW updating the inline documentation simply means using docblocks in the right places.

Oh ok! I'll double check all of that now.

[CMCDragonkai]

It's about running npm run build. Which should usually work if everything is passing.

[joshuakarp]

I completed my review of this PR over the weekend - everything else looks fine to me.

Although, I've just ran the Discovery tests, and it appears they are failing at this stage:

josh@josh-XPS-13-9360:~/Documents/MatrixAI/js-polykey$ npm test tests/discovery/

> @matrixai/[email protected] test
> jest "tests/discovery/"

Determining test suites to run...
GLOBAL SETUP
Global Data Dir: /tmp/polykey-test-global-KR4d9i
WARN: 0.0.0.0:51093:Forward Error: ErrorConnectionEndTimeout
 FAIL  tests/discovery/Discovery.test.ts (24.885 s)
  Discovery
    ✓ discovery readiness (10 ms)
    ✕ discovery by node (21 ms)
    ✕ discovery by identity (4 ms)
    ✕ updates previously discovered gestalts (4 ms)
    ✕ discovery persistence across restarts (6 ms)

  ● Discovery › discovery by node

    ErrorNodeGraphEmptyDatabase:

      518 |     // then we should throw an error. We aren't going to find any others.
      519 |     if (shortlist.length === 0) {
    > 520 |       throw new nodesErrors.ErrorNodeGraphEmptyDatabase();
          |             ^
      521 |     }
      522 |     // Need to keep track of the nodes that have been contacted.
      523 |     // Not sufficient to simply check if there's already a pre-existing connection

      at Object.getClosestGlobalNodes (src/nodes/NodeGraph.ts:520:13)
      at Object.findNode (src/nodes/NodeManager.ts:610:17)
      at Object.establishNodeConnection (src/nodes/NodeManager.ts:485:27)
      at Object.getConnectionToNode (src/nodes/NodeManager.ts:466:22)
      at Object.requestChainData (src/nodes/NodeManager.ts:297:24)
      at Object.setupDiscoveryQueue (src/discovery/Discovery.ts:216:33)
      at Object.runDiscoveryQueue (src/discovery/Discovery.ts:354:22)

  ● Discovery › discovery by identity

    ErrorNodeGraphEmptyDatabase:

      518 |     // then we should throw an error. We aren't going to find any others.
      519 |     if (shortlist.length === 0) {
    > 520 |       throw new nodesErrors.ErrorNodeGraphEmptyDatabase();
          |             ^
      521 |     }
      522 |     // Need to keep track of the nodes that have been contacted.
      523 |     // Not sufficient to simply check if there's already a pre-existing connection

      at Object.getClosestGlobalNodes (src/nodes/NodeGraph.ts:520:13)
      at Object.findNode (src/nodes/NodeManager.ts:610:17)
      at Object.establishNodeConnection (src/nodes/NodeManager.ts:485:27)
      at Object.getConnectionToNode (src/nodes/NodeManager.ts:453:22)
      at Object.requestChainData (src/nodes/NodeManager.ts:297:24)
      at Object.setupDiscoveryQueue (src/discovery/Discovery.ts:216:33)
      at Object.runDiscoveryQueue (src/discovery/Discovery.ts:354:22)

  ● Discovery › updates previously discovered gestalts

    ErrorNodeGraphEmptyDatabase:

      518 |     // then we should throw an error. We aren't going to find any others.
      519 |     if (shortlist.length === 0) {
    > 520 |       throw new nodesErrors.ErrorNodeGraphEmptyDatabase();
          |             ^
      521 |     }
      522 |     // Need to keep track of the nodes that have been contacted.
      523 |     // Not sufficient to simply check if there's already a pre-existing connection

      at Object.getClosestGlobalNodes (src/nodes/NodeGraph.ts:520:13)
      at Object.findNode (src/nodes/NodeManager.ts:610:17)
      at Object.establishNodeConnection (src/nodes/NodeManager.ts:485:27)
      at Object.getConnectionToNode (src/nodes/NodeManager.ts:453:22)
      at Object.requestChainData (src/nodes/NodeManager.ts:297:24)
      at Object.setupDiscoveryQueue (src/discovery/Discovery.ts:216:33)
      at Object.runDiscoveryQueue (src/discovery/Discovery.ts:354:22)

  ● Discovery › discovery persistence across restarts

    ErrorNodeGraphEmptyDatabase:

      518 |     // then we should throw an error. We aren't going to find any others.
      519 |     if (shortlist.length === 0) {
    > 520 |       throw new nodesErrors.ErrorNodeGraphEmptyDatabase();
          |             ^
      521 |     }
      522 |     // Need to keep track of the nodes that have been contacted.
      523 |     // Not sufficient to simply check if there's already a pre-existing connection

      at Object.getClosestGlobalNodes (src/nodes/NodeGraph.ts:520:13)
      at Object.findNode (src/nodes/NodeManager.ts:610:17)
      at Object.establishNodeConnection (src/nodes/NodeManager.ts:485:27)
      at Object.getConnectionToNode (src/nodes/NodeManager.ts:453:22)
      at Object.requestChainData (src/nodes/NodeManager.ts:297:24)
      at Object.setupDiscoveryQueue (src/discovery/Discovery.ts:216:33)
      at Object.runDiscoveryQueue (src/discovery/Discovery.ts:354:22)
      at Object.stop (src/discovery/Discovery.ts:147:5)
      at Object.stop (node_modules/@matrixai/async-init/src/CreateDestroyStartStop.ts:130:22)

Test Suites: 1 failed, 1 total
Tests:       4 failed, 1 passed, 5 total
Snapshots:   0 total
Time:        24.916 s, estimated 29 s
Ran all test suites matching /tests\/discovery\//i.
GLOBAL TEARDOWN
Destroying Global Data Dir: /tmp/polykey-test-global-KR4d9i
Jest did not exit one second after the test run has completed.

This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.

I'll take a quick look at this.

Never mind - all fine, it was a problem on my end.

[emmacasolin]

Rebased on master. All tests are still passing and everything has been lintfixed again. All that's left for this PR is this remaining issue: #320 (comment)

At the moment I've implented the ability to lookup your own authenticated identity ids using the command pk identities authenticated. The naming could probably be improved, but this is something that could be left to a later PR, as suggested here #320 (comment)

[CMCDragonkai]

This should fix #310 (comment)

tests/bin/identities/identities.test.ts

However the splitting of that will be in #311.

Same with tests/client/rpcGestalts.

[emmacasolin]

This should fix #310 (comment)

tests/bin/identities/identities.test.ts

However the splitting of that will be in #311.

Same with tests/client/rpcGestalts.

tests/bin/identities/identities.test.ts is actually still failing here. I attempted to debug it, however there are many nested describes and instances of setting state in beforeEach/afterEach that make it incredibly difficult to work out where the errors are coming from. I think it will be easier to do this while splitting the tests in #311, expecially considering there's a good chance that most of the errors are from inteference that splitting the tests removes anyway.

[CMCDragonkai]

Merged. @emmacasolin remember to avoid using full stops in our comments, mainly as it's not used in other places. Docblocks can use it for very long commentary, but I usually do this:

/**
 *  Something something something
 *  Beginning something something
 *  continuing the comment
 */

So no full stops ever used.

So #311 will have more things to do, can you update the PR description there to include fixing tests:

1. tests/bin/identities/identities.test.ts
2. tests/client/rpcGestalts.test.ts

Also new issue(s) for pk identities token commands, remember to spec it out