Skip to content

Adding privateKeyPath back to PolykeyAgent #600

Merged
addievo merged 1 commit intostagingfrom
feature-websocket_client-encapsulation
Oct 24, 2023
Merged

Adding privateKeyPath back to PolykeyAgent #600
addievo merged 1 commit intostagingfrom
feature-websocket_client-encapsulation

Conversation

@addievo
Copy link
Contributor

@addievo addievo commented Oct 24, 2023

Description

In

#582

Changes were made to PolykeyAgentOptions and it was flattened to remove dependency on types, this resulted in privateKeyPath being removed from PolykeyAgentOptions which is causing issues in Polykey-CLI.

This PR aims to add it back to PolykeyAgentOptions.

    privateKey: string;
    privateKeyPath: string;

Tasks

  • 1. Add privateKey and privateKeyPath back to PolykeyAgentOptions.

Final checklist

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

@ghost
Copy link

ghost commented Oct 24, 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 force-pushed the feature-websocket_client-encapsulation branch from b02eac1 to 6109fa6 Compare October 24, 2023 02:44
@addievo addievo merged commit 9f52ca0 into staging Oct 24, 2023
@CMCDragonkai
Copy link
Member

@addievo I don't think you lintfixed this?

@CMCDragonkai
Copy link
Member

  • Your PR isn't lintfixed - this is why the lintfixing is meant to be done at the end.
  • If the PR doesn't fix anything, delete that section
  • Don't tick things off until the end
  • Don't we need a default value for the configuration on agent options?

Comment on lines +64 to +65
privateKeyPath: string;
privateKey: PrivateKey;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The privateKey: PrivateKey should be using a more open type, instead the opaque type because of PolykeyAgent taking parameters from outside world.

So if you want to keep this then you should be using privateKey: Buffer.

Also privateKey should be moved up top.

privateKey: Buffer;
privateKeyPath: string;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 options aren't even being used, if these 2 options are to be used, they should be passed to the KeyRing.createKeyRing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addievo link the new PR to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PolykeyAgent.start should be taking DeepPartial of options, and the options need to keys: { recoveryCode; privateKey; privateKeyPath; }.

And also in the KeyRing.createKeyRing and KeyRing.start methods, these need to receive those options if you want to make them useful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the PolykeyAgentOptions needs to match:

  } & ( // eslint-disable-next-line @typescript-eslint/ban-types
    | {}
    | {
        recoveryCode: RecoveryCode;
      }
    | {
        privateKey: PrivateKey;
      }
    | {
        privateKeyPath: string;
      }

Which ensures mutually independent options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So:

keys: ({} | { recoveryCode: string; } | { privateKey: Buffer } | { privateKeyPath: string })

And PolykeyAgent.start should take this.

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