Skip to content

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

Merged
CMCDragonkai merged 1 commit intostagingfrom
feature-claim-fix
Oct 21, 2023
Merged

Fixing identities claim to be in-line with identities discover and identities trust, by introducing a new parser parseGestaltIdentityId#595
CMCDragonkai merged 1 commit intostagingfrom
feature-claim-fix

Conversation

@addievo
Copy link
Copy Markdown
Contributor

@addievo addievo commented Oct 19, 2023

RELATED TO MatrixAI/Polykey-CLI#29

Description

For the CLI fix, we need a new function to parse the gestalt identity, this introduces parseGestaltIdentityId in Polykey to achieve the same.

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 #28

Tasks

  • 1. Introduce parseGestaltIdentityId

Final checklist

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

@addievo addievo changed the title fixes: identities claim being different discover and trust Fixing claim syntax being out of sync from trust and discover Oct 19, 2023
@addievo addievo requested a review from CMCDragonkai October 19, 2023 06:13
@addievo addievo self-assigned this Oct 19, 2023
@ghost
Copy link
Copy Markdown

ghost commented Oct 19, 2023

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

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@addievo addievo changed the title Fixing claim syntax being out of sync from trust and discover Fixing identities claim to be in-line with identities discover and identities trust, by introducing a new parser parseGestaltIdentityId Oct 19, 2023
Comment thread src/ids/index.ts Outdated
@CMCDragonkai
Copy link
Copy Markdown
Member

@addievo in the future please name your branches a bit more specifically. We actually leave our feature branches around for a while before GCing them and your name is too generic, leaving plenty of opportunity for conflicts.

@CMCDragonkai
Copy link
Copy Markdown
Member

@addievo your commit messages are incorrect, see the new format. Also can you re-read the conventional commits specification.

@CMCDragonkai CMCDragonkai merged commit 19569a8 into staging Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants