Conversation
| return deriveFromRaw(hmacResult) | ||
| } | ||
|
|
||
| override fun deriveFromRaw(raw: ByteArray): Bip32ExtendedKeyPair { |
There was a problem hiding this comment.
We don't use it for now so I haven't check how it works.
We can throw KeypairFactory.SoftDerivationNotSupported() in this case like in OtherSubstrateKeypairFactory but I think current version is more correct
There was a problem hiding this comment.
When we alter something in encrypt/ folder, we should always write tests, let alone leave the code even manually untested
In order to reduce the work needed, I suggest we remove deriveFromRaw (the name is very confusing btw) and just add it do the Sr25519SubstrateKeypairFactory directly. Then, SubstrateKeypairFactory may have a single method decodeSr25519Keypair that delegates to Sr25519SubstrateKeypairFactory
And please write at least some tests for that
| return deriveFromRaw(hmacResult) | ||
| } | ||
|
|
||
| override fun deriveFromRaw(raw: ByteArray): Bip32ExtendedKeyPair { |
There was a problem hiding this comment.
The same here
| override fun deriveFromRaw(raw: ByteArray): Sr25519Keypair { | ||
| val publicKey = Sr25519.getPublicKeyFromSecret(raw) | ||
| val keypairBytes = raw + publicKey | ||
| return decodeSr25519Keypair(keypairBytes) | ||
| } |
There was a problem hiding this comment.
We can get privateKey+nonce+publicKey in raw in theory so maybe it would be nice to check raw size before adding publickKey here in future
sr25519-java/src/lib.rs
Outdated
| * @return public key [32 bytes] | ||
| */ | ||
| #[no_mangle] | ||
| pub unsafe extern "system" fn Java_io_novasama_substrate_1sdk_1android_encrypt_Sr25519_getPublicKeyFromSeed( |
There was a problem hiding this comment.
Do we need that? Seems like we can just use keypairFromSeed to get both the private key and the public key.
We would like to keep the api surface minimal to avoid confusion
| public static final int PUBLIC_SIZE = 32; | ||
|
|
||
| /// Size of SR25519 PRIVATE (SECRET) KEY, which consists of [32 bytes key | 32 bytes nonce] | ||
| /// Size of SR25519 PRIVATE (SECRET) KEY, which consists of [32bytes key|32bytes nonce] |
There was a problem hiding this comment.
Just autoformat issue - fixed
| return deriveFromRaw(hmacResult) | ||
| } | ||
|
|
||
| override fun deriveFromRaw(raw: ByteArray): Bip32ExtendedKeyPair { |
There was a problem hiding this comment.
When we alter something in encrypt/ folder, we should always write tests, let alone leave the code even manually untested
In order to reduce the work needed, I suggest we remove deriveFromRaw (the name is very confusing btw) and just add it do the Sr25519SubstrateKeypairFactory directly. Then, SubstrateKeypairFactory may have a single method decodeSr25519Keypair that delegates to Sr25519SubstrateKeypairFactory
And please write at least some tests for that
| private const val PARTS_MIN = 3 | ||
| private const val PARTS_MAX = 4 | ||
|
|
||
| class SecretQrFormat : QrFormat<SecretQrFormat.Payload> { |
There was a problem hiding this comment.
Tbh I think this should go to the app. Since this is highly specific to how Vault exposes the secrets
| return decodeSr25519Keypair(keypairBytes) | ||
| } | ||
|
|
||
| fun deriveEncryptedKeypair(raw: ByteArray): Sr25519Keypair { |
There was a problem hiding this comment.
I think this could be named createKeypairFromSecret(secretKey: ByteArray)
| @@ -0,0 +1,6 @@ | |||
| package io.novasama.substrate_sdk_android.encrypt | |||
|
|
|||
| fun sr25519PublicKeyFromSeed(seed: ByteArray): ByteArray { | |||
There was a problem hiding this comment.
I think it would be better if we add this to SubstrateKeyaprFactory instead to not to pollute global namespace
| } | ||
|
|
||
| fun decodeSr25519Keypair( | ||
| encryptedKey: ByteArray, |
There was a problem hiding this comment.
The key is not encrypted, it just just a secret key. We should be carefull with naming to avoid confusion that might cause wrong threatement of the secret
There was a problem hiding this comment.
I think we can delete this method since we make Sr25519SubstrateKeypairFactory public and can use it directly
| fun decodeSr25519Keypair( | ||
| encryptedKey: ByteArray, | ||
| junctions: List<Junction> = emptyList() | ||
| ): Keypair = Sr25519SubstrateKeypairFactory.deriveKeyPair( |
There was a problem hiding this comment.
I am a bit confused: so we want to construct a sr25519 keypair from the secret and then perform the derivation? I thought we get an already derived keypair from vault OR the seed
| assertEquals(shouldBeValid, isValid) | ||
| } | ||
|
|
||
| private fun signAndVerifySr25519WithEncryptedKey( |
There was a problem hiding this comment.
What does this test actually check? Since the code we change is related to derivation the test should be in the derivations test cases and should check that we do the derivation correctly:
- Get the private key from some Vault account. Also get the account id. Use it as the input data
- Derive the keypair
- Check that the publicKey matches the account id from Vault. Also check that the secret key (private key + nonce) is the same as the one that was passed
For the seed derivation, do the same, but check just the public key
2 participants
No description provided.