feat: default to uncompressed keys #2165
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 30ff14a The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
berendsliedrecht
force-pushed
the
key-compression
branch
2 times, most recently
from
3a5b77f to
7075bbc
Compare
TimoGlastra
approved these changes
Feb 4, 2025
tests/InMemoryWallet.ts
Outdated
| @@ -13,6 +13,7 @@ import BigNumber from 'bn.js' | |||
|
|
|||
| import { convertToAskarKeyBackend } from '../packages/askar/src/utils/askarKeyBackend' | |||
| import { didcommV1Pack, didcommV1Unpack } from '../packages/askar/src/wallet/didcommV1' | |||
| import { expandIfPossible } from '../packages/core/src/crypto/jose/jwk/ecCompression' | |||
There was a problem hiding this comment.
Should this method be available as export? Or on a key instance or something?
E.g. a getter with compressedPubliKey that optionally returns null?
There was a problem hiding this comment.
The key instance public get publicKey returns the uncompressed public key
| import { isEncryptionSupportedForKeyType, isSigningSupportedForKeyType } from './keyUtils' | ||
| import { getKeyTypeByMultiCodecPrefix, getMultiCodecPrefixByKeyType } from './multiCodecKey' | ||
|
|
||
| export class Key { | ||
| public readonly publicKey: Buffer | ||
| public readonly publicKey: Uint8Array |
There was a problem hiding this comment.
This is quite a big breaking change. Can you add the needed changeset entry?
| @@ -52,7 +52,7 @@ describeSkipNode18('BBS Signing Provider', () => { | |||
| key, | |||
| }) | |||
| ).rejects.toThrow( | |||
| 'Error signing data with verkey AeAihfn5UFf7y9oesemKE1oLmTwKMRv7fafTepespr3qceF4RUMggAbogkoC8n6rXgtJytq4oGy59DsVHxmNj9WGWwkiRnP3Sz2r924RLVbc2NdP4T7yEPsSFZPsWmLjgnP1vXHpj4bVXNcTmkUmF6mSXinF3HehnQVip14vRFuMzYVxMUh28ofTJzbtUqxMWZQRu. Unsupported keyType: bls12381g1g2' | |||
| 'Error signing data with key associated with AeAihfn5UFf7y9oesemKE1oLmTwKMRv7fafTepespr3qceF4RUMggAbogkoC8n6rXgtJytq4oGy59DsVHxmNj9WGWwkiRnP3Sz2r924RLVbc2NdP4T7yEPsSFZPsWmLjgnP1vXHpj4bVXNcTmkUmF6mSXinF3HehnQVip14vRFuMzYVxMUh28ofTJzbtUqxMWZQRu. Unsupported keyType: bls12381g1g2' | |||
There was a problem hiding this comment.
Suggested change
| 'Error signing data with key associated with AeAihfn5UFf7y9oesemKE1oLmTwKMRv7fafTepespr3qceF4RUMggAbogkoC8n6rXgtJytq4oGy59DsVHxmNj9WGWwkiRnP3Sz2r924RLVbc2NdP4T7yEPsSFZPsWmLjgnP1vXHpj4bVXNcTmkUmF6mSXinF3HehnQVip14vRFuMzYVxMUh28ofTJzbtUqxMWZQRu. Unsupported keyType: bls12381g1g2' | |
| 'Error signing data with key associated with publicKeyBase58 AeAihfn5UFf7y9oesemKE1oLmTwKMRv7fafTepespr3qceF4RUMggAbogkoC8n6rXgtJytq4oGy59DsVHxmNj9WGWwkiRnP3Sz2r924RLVbc2NdP4T7yEPsSFZPsWmLjgnP1vXHpj4bVXNcTmkUmF6mSXinF3HehnQVip14vRFuMzYVxMUh28ofTJzbtUqxMWZQRu. Unsupported keyType: bls12381g1g2' |
| @@ -349,7 +352,9 @@ export abstract class AskarBaseWallet implements Wallet { | |||
| if (!isError(error)) { | |||
| throw new CredoError('Attempted to throw error, but it was not of type Error', { cause: error }) | |||
| } | |||
| throw new WalletError(`Error signing data with verkey ${key.publicKeyBase58}. ${error.message}`, { cause: error }) | |||
| throw new WalletError(`Error signing data with key associated with ${key.publicKeyBase58}. ${error.message}`, { | |||
There was a problem hiding this comment.
Suggested change
| throw new WalletError(`Error signing data with key associated with ${key.publicKeyBase58}. ${error.message}`, { | |
| throw new WalletError(`Error signing data with key associated with publicKeyBase58 ${key.publicKeyBase58}. ${error.message}`, { |
| @@ -28,6 +28,7 @@ import { | |||
| KeyBackend, | |||
| KeyType, | |||
| utils, | |||
| expandIfPossible, | |||
There was a problem hiding this comment.
If we export i think we should have a more unique name. expandPublicKeyIfPossible maybe?
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
berendsliedrecht
force-pushed
the
key-compression
branch
from
267c22c to
4758e8b
Compare
…ative implementations - By default rely on uncompressed keys instead of compressed (for P256, P384, P521 and K256) - Utilze Uint8Array more instead of Buffer (i.e. for internally representing a key) Signed-off-by: Berend Sliedrecht <berend@animo.id>
TimoGlastra
reviewed
Feb 6, 2025
Comment on lines
+8
to
+9
| - By default rely on uncompressed keys instead of compressed (for P256, P384, P521 and K256) | ||
| - Utilze Uint8Array more instead of Buffer (i.e. for internally representing a key) |
There was a problem hiding this comment.
These two should be marked as minor (due to breaking changes)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Default to uncompressed keys where possible.
Later TODO's: