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

Add decryptMessage method #1596

Merged
merged 2 commits into from
Aug 30, 2023
Merged

Add decryptMessage method #1596

merged 2 commits into from
Aug 30, 2023

Conversation

mikesposito
Copy link
Member

Explanation

This PR adds the decryptMessage method needed by extension's DecryptMessageController.

References

Changelog

@metamask/keyring-controller

  • ADDED: decryptMessage method

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@mikesposito mikesposito requested a review from a team as a code owner August 17, 2023 13:03
@mikesposito mikesposito marked this pull request as draft August 17, 2023 13:04
@mikesposito
Copy link
Member Author

Converted to a draft as I'm trying to add a test case as EthKeyringController doesn't have coverage for this method

@mikesposito mikesposito changed the title feat: add decryptMessage method Add decryptMessage method Aug 17, 2023
@cryptodev-2s cryptodev-2s force-pushed the feat/decrypt-message branch 3 times, most recently from 3d98084 to 16ca6a7 Compare August 29, 2023 12:59
@cryptodev-2s cryptodev-2s self-assigned this Aug 29, 2023
@cryptodev-2s cryptodev-2s marked this pull request as ready for review August 29, 2023 13:00
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

One comment!

packages/keyring-controller/src/KeyringController.test.ts Outdated Show resolved Hide resolved
@cryptodev-2s cryptodev-2s requested review from mcmire, Gudahtt and a team August 30, 2023 14:07
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks good!

@cryptodev-2s cryptodev-2s merged commit cb5be27 into main Aug 30, 2023
91 checks passed
@cryptodev-2s cryptodev-2s deleted the feat/decrypt-message branch August 30, 2023 16:15
Comment on lines +602 to +623
const { importedAccountAddress } =
await controller.importAccountWithStrategy(
AccountImportStrategy.privateKey,
[privateKey],
);
const message = 'Hello, encrypted world!';
const encryptedMessage = encrypt({
publicKey: await controller.getEncryptionPublicKey(
importedAccountAddress,
),
data: message,
version: 'x25519-xsalsa20-poly1305',
});

const messageParams = {
from: importedAccountAddress,
data: encryptedMessage,
};

const result = await controller.decryptMessage(messageParams);

expect(result).toBe(message);
Copy link
Member Author

Choose a reason for hiding this comment

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

We could also simplify this by taking inspiration from getEncryptionPublicKey tests, with something like this:

Suggested change
const { importedAccountAddress } =
await controller.importAccountWithStrategy(
AccountImportStrategy.privateKey,
[privateKey],
);
const message = 'Hello, encrypted world!';
const encryptedMessage = encrypt({
publicKey: await controller.getEncryptionPublicKey(
importedAccountAddress,
),
data: message,
version: 'x25519-xsalsa20-poly1305',
});
const messageParams = {
from: importedAccountAddress,
data: encryptedMessage,
};
const result = await controller.decryptMessage(messageParams);
expect(result).toBe(message);
const { importedAccountAddress } =
await controller.importAccountWithStrategy(
AccountImportStrategy.privateKey,
[privateKey],
);
const result = await controller.decryptMessage({
from: importedAccountAddress,
data: {
version: 'x25519-xsalsa20-poly1305',
nonce: 'wLwuCz6aY+IjdF34os94LbAer8H5TPck',
ephemPublicKey: 'B4cLIZfy4AU7xdM8DAO/0+VaIm8W2aY2xb7ciaPmpx8=',
ciphertext:
'4mNw0NlJfa+qWAsCza/iYk7uQtYcY/TBLLQnFsPsnS0E7ErjLU8J/ViK',
},
});
expect(result).toBe('My name is Satoshi Buterin');

This would also remove interdependency between unit tests

Copy link
Contributor

Choose a reason for hiding this comment

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

@mikesposito PR got already merged

Copy link
Member Author

Choose a reason for hiding this comment

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

Ops, it got merged while I was reviewing it 😄 nevermind!

MajorLift pushed a commit that referenced this pull request Oct 11, 2023
feat: add decryptMessage method

---------

Co-authored-by: Salah <[email protected]>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
feat: add decryptMessage method

---------

Co-authored-by: Salah <[email protected]>
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.

3 participants