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

Addresses generator (#1) #825

Merged
merged 20 commits into from
Jul 19, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 49 additions & 1 deletion Sources/Web3Core/KeystoreManager/BIP32Keystore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ public class BIP32Keystore: AbstractKeystore {
if path.hasPrefix(prefixPath) {
let upperIndex = (path.range(of: prefixPath)?.upperBound)!
if upperIndex < path.endIndex {
pathAppendix = String(path[path.index(after: upperIndex)])
pathAppendix = String(path[path.index(after: upperIndex)..<path.endIndex])
} else {
throw AbstractKeystoreError.encryptionError("out of bounds")
}
Expand Down Expand Up @@ -216,6 +216,54 @@ public class BIP32Keystore: AbstractKeystore {
try encryptDataToStorage(password, data: serializedRootNode, aesMode: self.keystoreParams!.crypto.cipher)
}

/// Fast generation addresses for current account
JeneaVranceanu marked this conversation as resolved.
Show resolved Hide resolved
/// used for shows which address user will get when changed number of his wallet
JeneaVranceanu marked this conversation as resolved.
Show resolved Hide resolved
/// - Parameters:
/// - password: password of seed storage
/// - number: number of wallets addresses needed to generate from 0 to number-1
JeneaVranceanu marked this conversation as resolved.
Show resolved Hide resolved
/// - Returns: Array of addresses generated from 0 to number bound, or empty array in case of error
public func getAddressForAccount(password: String, number: Int) -> [EthereumAddress] {
guard let decryptedRootNode = try? getPrefixNodeData(password) else {
return []
}
guard let rootNode = HDNode(decryptedRootNode) else {
return []
}
JeneaVranceanu marked this conversation as resolved.
Show resolved Hide resolved
let prefixPath = self.rootPrefix
var pathAppendix: String?

return [Int](0..<number).compactMap({ number in
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use trailing closure syntax

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

pathAppendix = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

your variable should be declared here and not line 233

let path = prefixPath + "/\(number)"
if path.hasPrefix(prefixPath) {
JeneaVranceanu marked this conversation as resolved.
Show resolved Hide resolved
let upperIndex = (path.range(of: prefixPath)?.upperBound)!
JeneaVranceanu marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

please no force unwrap

Copy link
Contributor Author

@6od9i 6od9i Jun 29, 2023

Choose a reason for hiding this comment

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

copyed from createNewCustomChildAccount

if upperIndex < path.endIndex {
pathAppendix = String(path[path.index(after: upperIndex)..<path.endIndex])
} else {
return nil
}

guard pathAppendix != nil else {
return nil
}
if pathAppendix!.hasPrefix("/") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

no force unwrap

Copy link
Contributor Author

@6od9i 6od9i Jun 29, 2023

Choose a reason for hiding this comment

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

copyed from createNewCustomChildAccount

pathAppendix = pathAppendix?.trimmingCharacters(in: CharacterSet.init(charactersIn: "/"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use proper swift init syntax

Copy link
Contributor Author

@6od9i 6od9i Jun 29, 2023

Choose a reason for hiding this comment

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

copyed from createNewCustomChildAccount

Copy link
Collaborator

Choose a reason for hiding this comment

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

All CharacterSet.init were replaced with .init to reduce the length of lines where it occurs.

}
} else {
if path.hasPrefix("/") {
pathAppendix = path.trimmingCharacters(in: CharacterSet.init(charactersIn: "/"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use proper swift init syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copyed from createNewCustomChildAccount

}
}
JeneaVranceanu marked this conversation as resolved.
Show resolved Hide resolved
guard pathAppendix != nil,
rootNode.depth == prefixPath.components(separatedBy: "/").count - 1,
let newNode = rootNode.derive(path: pathAppendix!, derivePrivateKey: true),
Copy link
Collaborator

Choose a reason for hiding this comment

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

please no force unwrap

Copy link
Contributor Author

@6od9i 6od9i Jun 29, 2023

Choose a reason for hiding this comment

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

Hi, thank you for your comments, but this code in style of all this file
I copied createNewCustomChildAccount method and changed it here, force unwraps had been written here everywhere, that why i used it. And i do not want to change all this file, i used this style

Copy link
Collaborator

Choose a reason for hiding this comment

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

In addition to all of the folks said, I'd like to add that there's a fair point in your arguing about relatively bad code style within all the least file. The thing is that this is how we decided to maintain this lib some time ago: we're leaving all the code that works yet unsound for as long as we're not required to change it. But if we are we're making it as sounded as we can.

Certainly it's not necessary to rewrite the whole file at the time, actually it's just fine to write in a good way just the required part, but it would be so much better if meanwhile you're applying your effort to the main problem to improve as much relative code as your effort big enough, this would really help to all successor folks who will use this lib afterwards, and you'll get a significant part of that appreciation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is very dangerous to change all this code, because i do not have enough time to understand all logic, i will change this method

let newAddress = Utilities.publicToAddress(newNode.publicKey) else {
return nil
}
return newAddress
})
}

fileprivate func encryptDataToStorage(_ password: String, data: Data, dkLen: Int = 32, N: Int = 4096, R: Int = 6, P: Int = 1, aesMode: String = "aes-128-cbc") throws {
guard data.count == 82 else {
throw AbstractKeystoreError.encryptionError("Invalid expected data length")
Expand Down
53 changes: 53 additions & 0 deletions Tests/web3swiftTests/localTests/BIP32KeystoreTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
//
// BIP32KeystoreTests.swift
// localTests
//
// Created by 6od9i on 29.06.2023.
//

import Foundation
import XCTest
import Web3Core

@testable import web3swift

class BIP32KeystoreTests: XCTestCase {
func testAddressGeneration() throws {
/// Seed randomly generated for this test
let mnemonic = "resource beyond merit enemy foot piece reveal eagle nothing luggage goose spot"
let password = "test_password"

let addressesCount = 101

guard let keystore = try BIP32Keystore(
mnemonics: mnemonic,
password: password,
mnemonicsPassword: "",
language: .english,
prefixPath: HDNode.defaultPathMetamaskPrefix) else {
XCTFail("Keystore has not generated")
throw NSError(domain: "0", code: 0)
}

let addresses = keystore.getAddressForAccount(password: password,
number: addressesCount)
XCTAssertEqual(addresses.count, addressesCount)
XCTAssertNotEqual(addresses[11], addresses[1])

guard let sameKeystore = try BIP32Keystore(
mnemonics: mnemonic,
password: password,
mnemonicsPassword: "",
language: .english,
prefixPath: HDNode.defaultPathMetamaskPrefix) else {
XCTFail("Keystore has not generated")
throw NSError(domain: "0", code: 0)
}

let walletNumber = addressesCount - 1
try sameKeystore.createNewCustomChildAccount(password: password,
path: HDNode.defaultPathMetamaskPrefix + "/\(walletNumber)")
let address = sameKeystore.addresses?.last?.address
XCTAssertEqual(addresses.last?.address, address)
}
}
Loading