FLW-1187 BIP 32 support #43
Conversation
| import BigInt | ||
| import IrohaCrypto | ||
|
|
||
| public struct BIP32JunctionFactory: JunctionFactoryProtocol { |
There was a problem hiding this comment.
We have duplicated logic regarding junction parsing compared with JunctionFactory. Consider to turn to the class and make base class with common logic.
| private func createChaincodeFromJunction(_ junction: String, type: ChaincodeType) throws -> Chaincode { | ||
| guard | ||
| var numericJunction = UInt32(junction), | ||
| numericJunction < 0x80000000 |
There was a problem hiding this comment.
Let's move 0x8... to constants and give appropriate name.
| @@ -22,6 +22,7 @@ public enum JunctionFactoryError: Error { | |||
| case multiplePassphrase | |||
| case emptyJunction | |||
| case invalidStart | |||
| case invalidBIP32Junction | |||
There was a problem hiding this comment.
Let's create a separate error enum for BIP32
| @@ -18,3 +18,9 @@ public protocol DerivableSeedFactoryProtocol: KeypairFactoryProtocol { | |||
| func deriveChildSeedFromParent(_ seed: Data, | |||
| chaincodeList: [Chaincode]) throws -> Data | |||
| } | |||
|
|
|||
| public protocol DerivableChaincodeFactoryProtocol: KeypairFactoryProtocol { | |||
There was a problem hiding this comment.
It should have been removed, ty
✅
| @@ -6,6 +6,7 @@ public typealias SeedFactoryResult = (seed: Data, mnemonic: IRMnemonicProtocol) | |||
| public protocol SeedFactoryProtocol { | |||
| func createSeed(from password: String, strength: IRMnemonicStrength) throws -> SeedFactoryResult | |||
| func deriveSeed(from mnemonicWords: String, password: String) throws -> SeedFactoryResult | |||
| func deriveNativeSeed(from mnemonicWords: String, password: String) throws -> SeedFactoryResult | |||
There was a problem hiding this comment.
Le't move this implementation to a separate factory BIP39SeedFactory and also please implement createSeed function.
| } | ||
|
|
||
| public struct BIP32KeyFactory { | ||
| private let initialSeed = "Bitcoin seed" |
There was a problem hiding this comment.
I think we could make it static to prevent from duplication in different instances
There was a problem hiding this comment.
Could you please elaborate more? What is duplicated?
| let sourceData: Data = try { | ||
| switch chaincode.type { | ||
| case .hard: | ||
| let padding = try Data(hexString: "0x00") |
There was a problem hiding this comment.
looks like overhead, consider to use Data(repeating: 0 count: 1) to remove an option where we can receive exception but it never happens.
|
|
||
| var privateKeyInt = BigUInt(privateKeySourceData.rawData()) | ||
|
|
||
| guard privateKeyInt < .secp256k1CurveOrder else { |
There was a problem hiding this comment.
Should we also require from privateKey not to be zero?
There was a problem hiding this comment.
Thanks, I lost it somehow during refactoring
✅
|
|
||
| var privateKeyData = privateKeyInt.serialize() | ||
|
|
||
| if privateKeyData.count < 32 { |
There was a problem hiding this comment.
Consider to use SECPrivateKey.length instead of 32
|
|
||
| if privateKeyData.count < 32 { | ||
| var paddedPrivateKeyData = Data(repeating: 0, count: 32) | ||
| paddedPrivateKeyData[(32 - privateKeyData.count)...] = privateKeyData |
There was a problem hiding this comment.
Consider to use concatenation instead of indexing. It should be just like Data(repeating: 0, count: length-pk.length) + pk.
|
|
||
| private func createChaincodeFromJunction(_ junction: String, type: ChaincodeType) throws -> Chaincode { | ||
| guard | ||
| var numericJunction = UInt32(junction), |
There was a problem hiding this comment.
Let's separate this cases into different errors. So for a library user it will be clear what is going wrong.
| try performTest(filename: "BIP32HDKD", keypairFactory: BIP32KeypairFactory(), useMiniSeed: false) | ||
| } | ||
|
|
||
| func testBIP32DerivationPathStandardData() throws { |
There was a problem hiding this comment.
Consider to rename to testBIP32DerivationPathForEtalonTestVectors
There was a problem hiding this comment.
Good suggestion, I definitely struggled with naming
✅
| } | ||
| } | ||
|
|
||
| private func performSeedReadyTest(filename: String, keypairFactory: KeypairFactoryProtocol) throws { |
There was a problem hiding this comment.
performSeedTestForVectorsFrom(..)
| @@ -16,9 +16,9 @@ class KeypairDeriviationTests: XCTestCase { | |||
| try performTest(filename: "ecdsaHDKD", keypairFactory: EcdsaKeypairFactory()) | |||
| } | |||
|
|
|||
| private func performTest(filename: String, keypairFactory: KeypairFactoryProtocol) throws { | |||
| private func performTest(filename: String, keypairFactory: KeypairFactoryProtocol, useMiniSeed: Bool = true) throws { | |||
There was a problem hiding this comment.
Why do we need useMiniSeed if it is not used?
There was a problem hiding this comment.
Well, it's there if someone (us?) would want to use it (:
I needed it first for BIP32 tests then I decided to separate them, so I thought it might be a good idea to leave it there for future uses
There was a problem hiding this comment.
Let's remove it for now if it is not used
| try performSeedReadyTest(filename: "BIP32HDKDStandard", keypairFactory: BIP32KeypairFactory()) | ||
| } | ||
|
|
||
| private func performTest(filename: String, keypairFactory: KeypairFactoryProtocol, useMiniSeed: Bool = true) throws { |
There was a problem hiding this comment.
Why do we need useMiniSeed option if it is not used
There was a problem hiding this comment.
Here we don't need it, yes
Add support for BIP32 address derivation