Enhance wallet class #159

usatie · 2018-09-18T10:45:08Z

Description of the Change

Implement high level class Wallet.
Now everyone can send/receive Bitcoin with only 10 lines.

// 1. Create Private Key, Publick Key, Address
let privkey: PrivateKey = PrivateKey(network: .mainnet)
let pubkey: PublicKey = privkey.publicKey()
let address: Cashaddr = pubkey.toCashaddr()
let qrImage: UIImage? = address.qrImage(size: CGSize(width: 240, height: 240))

// 2. Create Wallet, Get balance, Refresh balance
let wallet: Wallet = Wallet(privateKey: privkey,
                            walletDataStore: UserDefaults.standard)
let balance = wallet.balance
wallet.reloadBalance()

// 3. Send Bitcoin
let toAddress = try! AddressFactory.create("bitcoincash:qz936smm809wq0eqthqyh2sgdzhs7v08nvnuq0dmps")
wallet.send(to: toAddress, amount: 10000)

AddressProvider, UtxoProvider, TransactionHistoryProvider

Basically, both reload() and list() are methods for retrieving addresses, utxos and transactions. The difference between them is that reload() is async method, while list() is sync method.

Alternate Designs

None.

Benefits

More beginner welcome!

Possible Drawbacks

None.

Applicable Issues

closes #156

…tionBroadcaster

…llet class

…lector

akifuji · 2018-09-18T10:52:40Z

Sources/BitcoinKit/Wallet/BitcoinCom/BitcoinComService+WalletTransactionProvider.swift

+    let vout: UInt32
+    let sequence: UInt32
+    let scriptSig: ScriptSig
+    // let addr: String


Why are they comment out? Please comment the reason somewhere.

These are part of API response, but not used.

So I commented them out.

akifuji · 2018-09-18T11:08:48Z

Sources/BitcoinKit/Wallet/Protocol/AddressProvider.swift

+import Foundation
+
+public protocol AddressProvider {
+    // GET API: reload utxos


This comment is inappropriate, isn't it?

Oh it isn't. I'll fix it.

akifuji · 2018-09-18T11:14:36Z

Sources/BitcoinKit/Wallet/Protocol/TransactionBuilder.swift

+import Foundation
+
+public protocol TransactionBuilder {
+    func build(destinations: [(address: Address, amount: UInt64)], utxos: [UnspentTransaction]) throws -> UnsignedTransaction


The name might be confusing.
TransactionBuilder should build Transaction not UnsignedTransaction

I think the name is okay with this. But I think the name of Transaction and UnsignedTransaction are the problem.

UnsignedTransaction is almost just a wrap of Transaction and UnspentOutput

akifuji · 2018-09-18T11:15:43Z

Sources/BitcoinKit/Wallet/Protocol/TransactionProvider.swift

+
+import Foundation
+
+public protocol TransactionProvider {


Again, this one is also confusing.
How about TransactionHistoryProvider .

This is true. I'll change it.

akifuji · 2018-09-18T11:18:02Z

Sources/BitcoinKit/Wallet/Protocol/TransactionProvider.swift

+import Foundation
+
+public protocol TransactionProvider {
+    // GET API: reload transactions


I think this function would not be only with API, but also SPV or something.

akifuji · 2018-09-18T11:18:50Z

Sources/BitcoinKit/Wallet/Protocol/UtxoProvider.swift

+import Foundation
+
+public protocol UtxoProvider {
+    // GET API: reload utxos


Same as TransactionProvider .
Do not need to limit only API.

akifuji · 2018-09-18T11:22:55Z

Sources/BitcoinKit/Wallet/Protocol/WalletDataStoreProtocol.swift

+
+// MARK: - WalletDataStoreProtocol Extension
+internal enum WalletDataStoreKey: String {
+    case wif, internalIndex, extenralIndex


This protocol is not for HDWallet, isn't it?
Then, I think internalIndex and extenralIndex are not necessary.

This protocol will be for HDWallet as well.

akifuji · 2018-09-18T11:30:11Z

Sources/BitcoinKit/Wallet/Protocol/AddressProvider.swift

+
+public protocol AddressProvider {
+    // GET API: reload utxos
+    func reload(keys: [PrivateKey], completion: (([Address]) -> Void)?)


I cannot understand the reason why this function is necessary.
Is it enough to convert PrivateKey into Address?

No, converting PrivateKey to Address is not enough.
Mainly this protocol is for P2SH.

Someone may want to compute new P2SH addresses using the keys outside as well as the keys inside by using API or some other way.

akifuji · 2018-09-19T04:11:05Z

Sources/BitcoinKit/Wallet/Standard/StandardAddressProvider.swift

+
+    public func reload(keys: [PrivateKey], completion: (([Address]) -> Void)?) {
+        let data = try? JSONEncoder().encode(keys.map { $0.publicKey().toCashaddr() })
+        userDefaults.set(data, forKey: UserDefaultsKey.cashaddrs.rawValue)


You don't need to add completion?()?

akifuji · 2018-09-19T04:30:25Z

Sources/BitcoinKit/Wallet/Wallet.swift

+                 utxoSelector: UtxoSelector = StandardUtxoSelector.default,
+                 transactionBuilder: TransactionBuilder = StandardTransactionBuilder.default,
+                 transactionSigner: TransactionSigner = StandardTransactionSigner.default) {
+        guard let privkey = try? PrivateKey(wif: wif) else {


akifuji · 2018-09-19T04:46:39Z

Sources/BitcoinKit/Wallet/Wallet.swift

+        guard let wif = walletDataStore.getString(forKey: .wif) else {
+            return nil
+        }
+        guard let privkey = try? PrivateKey(wif: wif) else {


akifuji · 2018-09-19T04:49:21Z

WalletExample/WalletExample/ViewController.swift

+
+    func createWalletIfNeeded() {
+        if wallet == nil {
+            let privkey = PrivateKey(network: .testnet)


akifuji · 2018-09-19T04:49:38Z

WalletExample/WalletExample/ViewController.swift

+        qrCodeImageView.image = wallet?.address.qrImage()
+        addressLabel.text = wallet?.address.cashaddr
+        if let balance = wallet?.balance() {
+            balanceLabel.text = "残高：\(balance) satoshi"


Use English

akifuji · 2018-09-19T04:50:22Z

WalletExample/WalletExample/ViewController.swift

+//  ViewController.swift
+//  WalletExample
+//
+//  Created by Shun Usami on 2018/09/18.


Change copyright

…(dataStore)

akifuji · 2018-09-20T01:28:14Z

Sources/BitcoinKit/Wallet/BitcoinCom/BitcoinComEndpoint.swift

-public struct BitcoinComEndpoint {
-    public static let testnet: BitcoinComEndpoint = BitcoinComEndpoint(baseUrl: "https://trest.bitcoin.com/v1/")
-    public static let mainnet: BitcoinComEndpoint = BitcoinComEndpoint(baseUrl: "https://rest.bitcoin.com/v1/")
+public struct ApiEndPoint {


Should you modify file name too?

akifuji

LGTM

usatie added 21 commits September 18, 2018 19:39

✨ Add WalletDataStoreProtocol

913eacb

✏️ Modify copyright in WalletDataStoreProtocol.swift

54c8eef

✨ Add WalletUtxoProvider, WalletTransactionProvider and WalletTransac…

1bd4507

…tionBroadcaster

✨ Add UserDefaults+WalletDataStoreProtocol.swift

4bcb81c

♻️ Fix Transaction.version as UInt32

d757e5d

♻️ Fix TransactionOutput.value as UInt64

1dc9280

♻️ Fix tests to adopt bitcoin value as UInt64

eff8ce3

♻️ Refactor Wallet class

49420c8

✨ Add BitcoinComService

d71fe42

✨ Add utxoProvider, transactionProvider, transactionBroadcaster in Wa…

1f97c7c

…llet class

♻️ Refactor Wallet class

bc8097f

♻️ Move UserDefaults+WalletDataStoreProtocol.swift

f9274d2

✨ Add WalletTransactionBuilder, WalletTransactionSigner, WalletUtxoSe…

decadac

…lector

♻️ Rename Wallet related protocol names

c60ebf9

🔥 Remove unnecessary TODO

f562bef

♻️ Refactor BitcoinComService

c2b4a20

♻️ Refactor Wallet class

7178592

♻️ Add AddressProvider and StandardAddressProvider

ce8e147

♻️ Add AddressProvider, StandardAddressProvider in pbxproj

31009d3

✏️ Modify copyrights

264f8d0

✨ Add WalletExample

bfee8b5

usatie requested a review from akifuji September 18, 2018 10:45

usatie added the enhancement New feature or request label Sep 18, 2018

akifuji reviewed Sep 18, 2018

View reviewed changes

♻️ Rename TransactionHistoryProvider and modify some comments

9944111

akifuji reviewed Sep 19, 2018

View reviewed changes

usatie added 7 commits September 19, 2018 15:26

♻️ Fix StandardAddressProvider.reload()

b9ffa4a

✏️ Fix typo and copyrights

f908578

♻️ BitcoinKitDataStoreProtocol, TransactionHistoryProvider, UtxoProvider

bf81ced

♻️ Refactor BitcoinComEndpoint

c027b78

♻️ Refactor BitcoinCom APIs

06e66d6

♻️ Refactor BitcoinCom API initializers

ff21e7b

♻️ Refactor Wallet, Add Wallet.init(), Wallet.init(wif:), Wallet.init…

ffd45a4

…(dataStore)

akifuji reviewed Sep 20, 2018

View reviewed changes

akifuji approved these changes Sep 20, 2018

View reviewed changes

akifuji merged commit 62b5aec into master Sep 20, 2018

usatie deleted the enhance-wallet-class branch September 22, 2018 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance wallet class #159

Enhance wallet class #159

usatie commented Sep 18, 2018 •

edited

Loading

akifuji Sep 18, 2018

usatie Sep 18, 2018

akifuji Sep 18, 2018

usatie Sep 18, 2018

akifuji Sep 18, 2018

usatie Sep 18, 2018

usatie Sep 18, 2018

akifuji Sep 19, 2018

akifuji Sep 18, 2018 •

edited

Loading

usatie Sep 18, 2018

akifuji Sep 18, 2018

usatie Sep 18, 2018

akifuji Sep 18, 2018

usatie Sep 18, 2018

akifuji Sep 18, 2018

usatie Sep 18, 2018

akifuji Sep 18, 2018 •

edited

Loading

usatie Sep 18, 2018

akifuji Sep 19, 2018

akifuji Sep 19, 2018

akifuji Sep 19, 2018

akifuji Sep 19, 2018

akifuji Sep 19, 2018

akifuji Sep 19, 2018

akifuji Sep 20, 2018

akifuji left a comment

Enhance wallet class #159

Enhance wallet class #159

Conversation

usatie commented Sep 18, 2018 • edited Loading

Description of the Change

AddressProvider, UtxoProvider, TransactionHistoryProvider

Alternate Designs

Benefits

Possible Drawbacks

Applicable Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akifuji Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akifuji Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akifuji left a comment

Choose a reason for hiding this comment

usatie commented Sep 18, 2018 •

edited

Loading

akifuji Sep 18, 2018 •

edited

Loading

akifuji Sep 18, 2018 •

edited

Loading