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

Refactor wallet #228

Merged
merged 14 commits into from
Sep 19, 2019
Merged

Refactor wallet #228

merged 14 commits into from
Sep 19, 2019

Conversation

usatie
Copy link
Member

@usatie usatie commented Sep 18, 2019

Description of the Change

The Wallet related classes in this library focus on non-networking things.

Wallet usage

// Creating wallet
let wallet = Wallet.create(passphrase: "BitcoinKit-sample-wallet", network: .mainnet)

// Get receive address
wallet.address

// Get change address
wallet.changeAddress

// Get all the addresses
wallet.addresses

// Increment address index for receive address
wallet.incrementExternalIndex(by: 1)

// Increment address index for change address
wallet.incrementInternalIndex(by: 1)

// Get private keys for the addresses
wallet.privKeys

Plan, Build, and Sign a transaction

Assume you already have unspent transactions with theire keys.

let unspentTransactions: [UnspentTransaction] = ...
let privKeys: [PrivateKey] = ...

And then, Plan, Build and Sign a transaction.

// 1. Plan a transaction
let planner = TransactionPlanner(feePerByte: 1)
let plan = planner.plan(unspentTransactions: unspentTransactions, amount: targetAmount)


// 2. Build an unsigned transaction
let unsignedTx = TransactionBuilder.build(from: plan, toAddress: toAddress, changeAddress: changeAddress)


// 3. Sign the transaction
let sighashHelper = BCHSignatureHashHelper(hashType: .ALL)
let signer = TransactionSigner(unspentTransactions: plan.unspentTransactions, transaction: unsignedTx, sighashHelper: sighashHelper)
let signedTx = signer.sign(with: privKeys)

Benefits

More reliability, testability, and stability.

Possible Drawbacks

Users have to implement networking part on their own.
APIs/SPV etc...

Planning to implement such networking features as a separated framework on top of this library.

Applicable issues

closes #143

import Foundation

/// Registered coin types for usage in level 2 of BIP44 described in chapter "Coin type".
public enum CoinType {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to not use an enum for CoinType. But rather let it be a protocol, and then use a struct for coin types declared by this library. But internally also be using the protocol of course. Exactly like you did with CoinTypeEntity , and the static variables. Then other DLTs can use BitcoinKit. Actually we at Radix DLT use it in our Swift Library.

So something like this:

public protocol CoinTypeConvertible {
    /// BIP44 cointype value
    var index: UInt32 { get }
    var symbol: String { get }
    var name: String { get }
}

public struct CoinType: CoinTypeConvertible {
    public let index: UInt32
    public let symbol: String
    public let name: String
}

public extension CoinType {
    static let testnet = CoinType(1, "", "Testnet (all coins)")

    static let btc = CoinType(0, "BTC", "Bitcoin")
    static let bch = CoinType(145, "BCH", "Bitcoin Cash")
}

// Ethereum and Zilliqa etc can now use BitcoinKit (both uses curve `Secp256k1`)
public extension CoinTypeEntity {
    static let ethereum = CoinType(60, "ETH", "Ether")
    static let zilliqa = CoinType(313, "ZIL", "Zilliqa")
}

What do you think? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense.
Thank you for your feedback!

Copy link
Member Author

Choose a reason for hiding this comment

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

But why do we still need CoinTypeConvertible protocol?
I think we can just have CoinType struct.

@@ -25,28 +25,42 @@

import Foundation

public class Network {
open class Network {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe Network can be merged together with new CoinType, being a protocol and "open enum" (struct, not an enum) to ease of use for other DLTs :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I look into Network, and now I realize that it definitely needs more refactorization.
But I want to leave it for now. It's going to be another PR.

- Refactor HDWallet class
- Add CoinType
- Modify Network class modifier to be open
- Add PublicKey.toAddress(for network: Network)
- Refactor SighashType
- Add BCHSignatureHashHelper
- Add BCHSignatureHashHelperTests
- Add TransactionSignerTests
- Add BTCSignatureHashHelper
- Add Transaction+SignatureHash deprecated messages
♻️ Move deprecated UnsignedTransaction

♻️ Rename utxo to unspentTransaction
@usatie usatie merged commit 1235fe5 into master Sep 19, 2019
@usatie usatie deleted the refactor-wallet branch September 19, 2019 07:47
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.

HDWallet.coinType is hard-code for BTC
2 participants