Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: remove message encryption types and tools #793

Closed

Conversation

arty-name
Copy link
Collaborator

We’ve removed messages but some message-related types and tools remained, this PR cleans it up. I’d argue that they should rather be added in the kilt-extension-api.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)
    • Either PR or Ticket to update the Docs
    • Link the PR/Ticket here

@arty-name arty-name force-pushed the ta-remove-message-encryption-types-and-tools branch from 8c69bbf to 3d7ed05 Compare August 17, 2023 12:10
package.json Show resolved Hide resolved
packages/utils/package.json Show resolved Hide resolved
@arty-name arty-name force-pushed the ta-remove-message-encryption-types-and-tools branch from 3d7ed05 to d7c53aa Compare August 17, 2023 12:42
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

Is it a good idea to remove x25519 key creation? How would you imagine people would create and add x25519 public keys to their DID instead?

@@ -5,7 +5,12 @@
* found in the LICENSE file in the root directory of this source tree.
*/

import { DidDocument, DidServiceEndpoint, DidUri } from '@kiltprotocol/types'
import {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import {
import type {

Comment on lines -368 to -382

/**
* Generate from a seed a x25519 keypair to be used as DID encryption key.
*
* @param seed The keypair seed, only optional in the tests.
* @returns The keypair.
*/
export function makeEncryptionKeypairFromSeed(
seed = randomAsU8a(32)
): KiltEncryptionKeypair {
return {
...naclBoxPairFromSecret(seed),
type: 'x25519',
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I get removing the encrypt/decrypt stuff, but don't you think we should keep key creation around as we allow a certain set of public key types on DIDs and removing this would mean x25519 keys are the only ones that cannot be created using the sdk?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is more of a discussion PR. If these keys are only usable for messaging, and the messaging is deprecated, probably we don’t need to support them by the SDK directly. If you think they’d still be useful, let’s close this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I would rather keep it around for now. x25519-based key agreement is a use case for our DIDs, and even if there are other encryption schemes based on that besides the one implemented here, I think it's fair to have at least one supported by the sdk.

@rflechtner rflechtner closed this Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants