Fixing identities claim to be in-line with identities discover and identities trust, by introducing a new parser parseGestaltIdentityId

[addievo]

Description

The pk identities claim currently takes two parameters in order to claim an identity.

pk identities claim github.com cmcdragonkai

This is not consistent with other two commands trust and discover.

pk identities trust github.com:cmcdragonkai
pk identities discover github.com:cmcdragonkai

These two commands are not using the ProviderIdentityId type.

type ProviderIdentityId = [ProviderId, IdentityId];

This is because the encoded form of this type is ["github.com", "cmcdragonkai"].

Instead we need to use a parser similar to binParsers.parseGestaltId (which is used by the trust and discover commands), however this parser, parses both node IDs and identities.

What we need to do is to extract the identity parsing portion of binParsers.parseGestaltId.

function parseGestaltId(data: any): GestaltId {
  if (typeof data !== 'string') {
    throw new validationErrors.ErrorParse('Gestalt ID must be string');
  }
  const nodeId = decodeNodeId(data);
  if (nodeId != null) {
    return ['node', nodeId];
  }
  const match = (data as string).match(/^(.+):(.+)$/);
  if (match == null) {
    throw new validationErrors.ErrorParse(
      'Gestalt ID must be either a Node ID or `Provider ID:Identity ID`',
    );
  }
  const providerId = parseProviderId(match[1]);
  const identityId = parseIdentityId(match[2]);
  return ['identity', [providerId, identityId]];
}

It will be:

  const match = (data as string).match(/^(.+):(.+)$/);
  if (match == null) {
    throw new validationErrors.ErrorParse(
      'Gestalt ID must be either a Node ID or `Provider ID:Identity ID`',
    );
  }
  const providerId = parseProviderId(match[1]);
  const identityId = parseIdentityId(match[2]);
  return ['identity', [providerId, identityId]];

This will be called parseGestaltIdentityId.

Issues Fixed

• Fixes Discrepancy in claim, discover and trust commands in CLI #28

Tasks

• 1. Alter handler call.
• 2. Alter args.

Final checklist

• Domain specific tests
• Lint fixed
• Squash and rebased
• Sanity check the final build

[ghost]

👇 Click on the image for a new way to code review

Legend

[tegefaulkes]

The main issue is that you're using the GestaltId parser when you should only ever be parsing a IdentityId. If there is no existing parser for IdentityId for the format of provider:user then you need to make one.

Using the switch statement like that to enforce the type is a pretty weird decision. It's almost as bad as using the as keyword to narrow the type.

Remember, a quick solution that doesn't address the issue is not a good one. You need take some extra time to think through your implementation and weather it's a good one.

If you're unsure about something of can think of multiple ways to address something the you can @ someone in a the issue or PR with a question. it also helps to think out loud in the issues or PRs when working.

[tegefaulkes]

Remember, don't resolve review comments until you have committed and pushed the change. It's really confusing on my end if its resolved with no changes.

[addievo]

Important

This PR needs MatrixAI/Polykey#595 to be merged in Polykey to work.

[addievo]

@tegefaulkes this PR was finished reviewing by Roger and you. Think you can merge it

[tegefaulkes]

This is not technically finished if it's waiting for a new version of Polykey to be released. All tests need to be passing as well.

[CMCDragonkai]

The parseGestaltIdentityId is a "fragment" of parseGestaltId so it should be returning ['identity', ProviderId, IdentityId, that's correct.

[tegefaulkes]

I've reverted that change now. I'll finish off this PR at @CMCDragonkai 's request

[tegefaulkes]

This is done now, I'm merging.