Feature persistent store #22

w-i-n-s · 2021-03-12T09:43:05Z

This PR will resolve next issues:

Implement CoreData persistent store Implement CoreData persistent store #12
Multiple wallet support Multiple wallet support #19

* upstream/master: Fix address screen layout constraint, close Sjors#13

NthKey/SupportingFiles/NthKey.xcdatamodeld/NthKey.xcdatamodel/contents

NthKey/Model/PersistentStore.swift

Sjors · 2021-03-25T13:27:41Z

Several tests are failing at the moment on Travis: https://travis-ci.com/github/Sjors/nthkey-ios/builds/220832007#L997

Please let me know when you think it's ready to test.

Sjors

Would it make sense to move NthKey.xcdatamodeld from SupportingFiles to Model?

NthKey/Model/DataManager.swift

Sjors

The order here is a bit confusing:

The Mainnet / Testnet toggle only applies to the creation of a new wallet. Let's handle wallet creation in a model window (both Announce and adding by QR / JSON). However, this can be done in a followup PR.

I created a fresh seed and a multisig wallet with Specter, on testnet. For some reason the addresses don't match between Specter and the simulator.

NthKey/View/Settings/Wallet/WalletListView.swift

NthKey/View/Settings/Wallet/WalletEntityView.swift

w-i-n-s · 2021-04-06T15:16:21Z

@Sjors

I created a fresh seed and a multisig wallet with Specter, on testnet. For some reason the addresses don't match between Specter and the simulator.

Yes, the addresses creation process didn't take into account all the cosigners keys, just fixed it in f303327.

Note: Please remove the old wallet before your test. It can be performed by the long tap (UX isn't perfect for the moment). Because on addresses tab you can see addresses for the selected wallet, and if you just add new one (application didn't block you to add several copies of the same wallet for the moment) it will be hard to select right wallet.

Sjors · 2021-04-06T15:21:08Z

When scanning a QR code (or reading JSON) from Specter to import a wallet, you want to sanity check the network. Here's an example:

{ 
  "label": "NanoS with iPhone 12 simulator",
  "blockheight": 0,
  "descriptor": "wsh(sortedmulti(1,[048117aa\/48h\/1h\/0h\/2h]tpubDF5EvuWLmn6qak12tkKAi6wFHBBYFg22RxNycBHfMk43FeKgz5chhv1UANVuAmbF3PQopymtH54B1pKJomeMrH8cY9RUf6e51Dtt9vFxfsj\/0\/*,[ee081656\/48h\/1h\/0h\/2h]tpubDFFahYHoXpUZfqXEAP1KcuMCvJZ1s815LHFTtayUsg13GiZeJt9zvhx11bYRioSaxJ2FmjNaKN3Wjezbr5t3FxbD3yqJ5F22Gfds551iASQ\/0\/*))#99ajkvkr"
}

Ideally Specter should add a field with the network type, but the alternative way is to look at the xpub / tpub in descriptor. tpub is for testnet (and signet), xpub for mainnet.

If the user tries to import a wallet for the wrong network, just give an error. That should reduce complexity a bit in that last commit.

w-i-n-s · 2021-04-06T15:22:28Z

@Sjors

The order here is a bit confusing:
The Mainnet / Testnet toggle only applies to the creation of a new wallet. Let's handle wallet creation in a model window (both Announce and adding by QR / JSON). However, this can be done in a followup PR.

Yes, it is a good idea to guide user throw the "Add wallet" flow (Show QR to specter and scan QR from it to complete). I'll create issue for it.

Also, as I mention in #22 (comment) we have the same fingerprint for testnet/mainnet and we cannot separate which network contains the Specter wallet QR. So, better to ask user about it and remove toggle from AnnounceView

I'll follow you #22 (comment)

Sjors · 2021-04-06T15:29:33Z

I deleted and reimported the wallet, but the addresses are still not correct. In the above descriptor, the first address should be tb1q6um5p5gf5uhkwfw8t9g4rvlam7rxrah2n70sxx3j2h6s9sf8rpmqj2ktty) . I also notice it sometimes imports as testnet and other times as mainnet. Using a modal window, where the user first selects the network, should make all this less confusing (see #22 (review)).

The device fingerprint is the same for all networks; that's just the BIP32 standard.

w-i-n-s · 2021-04-06T15:32:30Z

I deleted and reimported the wallet, but the addresses are still not correct. In the above descriptor, the first address should be tb1q6um5p5gf5uhkwfw8t9g4rvlam7rxrah2n70sxx3j2h6s9sf8rpmqj2ktty) . I also notice it sometimes imports as testnet and other times as mainnet. Using a modal window, where the user first selects the network, should make all this less confusing (see #22 (review)).

Yes, the network recognition not working correctly at the moment, I'll fix it soon and let you know.

The device fingerprint is the same for all networks; that's just the BIP32 standard.

Okay, got it.

Sjors · 2021-04-07T18:31:00Z

NthKey/Model/SeedManager.swift

@@ -53,13 +53,9 @@ struct SeedManager {
            precondition(false)
        }
        let seedHex = BIP39Mnemonic(entropy)!.seedHex()
+        let masterKey = HDKey(seedHex, .testnet)! // FIXME: HDKey in this case shouldn't depend on network, because fingerprint is unique for the device


The HDKey object does depend on the network, but its fingerprint doesn't (please check). For the purpose of this function, using .mainnet looks better.
https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#master-key-generation

The differentiation between mainnet and testnet happens at depth 2: e.g. m/84'/0' (mainnet) vs m/84'/1' (testnet), but this doesn't really matter in practice.

@Sjors Yes, the fingerprint is the same, but HDKey is network-relewant, just check it. Okay, we'll use .mainnet here. fixed in bcda176

Sjors · 2021-04-07T18:33:54Z

NthKey/Model/DataManager.swift

+            var receivedNetwork: Network?
+            for key in desc.extendedKeys {
+                var keyNetwork: Network?
+                switch key.xpub.prefix(4) {


Instead of parsing the string like this, I think you can use the HDKey constructor in LibWally with the full string and see which network it returns?

This initiliazer:
https://github.com/blockchain/libwally-swift/blob/master/LibWally/BIP32.swift#L157

And then HDKey class has a method which uses libwally-core internals to figure out the network: https://github.com/blockchain/libwally-swift/blob/master/LibWally/BIP32.swift#L213

@Sjors Yes, you are right. fixed in 8d06e25

Sjors · 2021-04-08T16:15:59Z

Alright, this seems to work! Can you tidy up the commits a bit, and maybe test it yourself with Specter? I'll then give it a final review and merge. I made some issues for (minor) improvements here: https://github.com/Sjors/nthkey-ios/milestone/1

w-i-n-s · 2021-04-09T06:57:46Z

@Sjors

Alright, this seems to work! Can you tidy up the commits a bit, and maybe test it yourself with Specter?

Sure, I test all cases for the scope before each commit. I mean small scope as a small piece of the whole task.

I'll then give it a final review and merge. I made some issues for (minor) improvements here: https://github.com/Sjors/nthkey-ios/milestone/1

It isn't ready yet.

sign PSBT. Required in this PR
mark an address as used (at the moment I didn't find a way to do it). Not required in this PR - could you please advice me?
TBC: Arrange the whole PR by commits (even rewrite the whole history of PR). For now, it seems dirty to me.

Sjors · 2021-04-09T07:26:06Z

mark an address as used

This can wait.

w-i-n-s · 2021-04-19T07:46:26Z

@Sjors

Please let me know when you think it's ready to test.
Now it ready for the manual tests. User can export device creds -> import wallet -> check list of addresses -> load and sign a transactions.

Sjors · 2021-04-19T09:01:17Z

Recovering from a mnemonic works. Importing a wallet from Specter works. Receive addresses are correct. But the send tab has issues.

For some reason it's no longer hiding the change address. The 9899836 sats at the top should be hidden, because we can detect it's change.

The PSBT on clipboard is correct. The saved PSBT binary is also correct. But the QR is broken (obviously, it's too small):

Sjors · 2021-04-19T09:20:02Z

NthKey/Model/Destination.swift

-            signer.hdKey
-        }
+        let us = Signer.getOurselfSigner()
+
        self.isChange = output.isChange(signer: us.hdKey, inputs: inputs, cosigners: cosignerKeys, threshold: threshold)


Maybe cosignerKeys is not passed in correctly here, causing the change detection to break?

@Sjors Actually, HDKeys did initialization use only description, without fingerprint (fixed in f880dcc). QR code was just a placeholder, after last fixes everything should work as far as my test shows same flow between master and current branch

add addresses mockups, fix address views to operate with CD entities

fix preview settings

prepare for handle scans

fix default wallet selection

add sign view model, add code signers view model, add data manager empty mock, remove cosigners / hasWallet / fingerprint / mainnet saved values from user defaults, add fingerprints foa all networks to user default saved values, limit psbt manager / Signer / Document picker filename to testnet fo a while, switch off "should have cosigners" precondition on sign view controller, rename WalletManager to SeedManager, remove unused code

fix wallet selection

…llet in user defaults

move threshold to co-signers section

…ected wallet

refactoring SignView and it model to FSM, refactor SignViewController from UIHostingController to regular view controller, remove threshold from UserDefaults, remove Cosigners from Signer

Merge remote-tracking branch 'upstream/master'

21e6379

* upstream/master: Fix address screen layout constraint, close Sjors#13

Sjors reviewed Mar 12, 2021

View reviewed changes

w-i-n-s force-pushed the feature_persistent_store branch from 9e7264a to cc80b3f Compare March 21, 2021 23:16

Sjors reviewed Mar 28, 2021

View reviewed changes

NthKey/Model/DataManager.swift Outdated Show resolved Hide resolved

Sjors reviewed Apr 5, 2021

View reviewed changes

NthKey/View/Settings/Wallet/WalletListView.swift Outdated Show resolved Hide resolved

NthKey/View/Settings/Wallet/WalletListView.swift Outdated Show resolved Hide resolved

NthKey/View/Settings/Wallet/WalletEntityView.swift Outdated Show resolved Hide resolved

Sjors reviewed Apr 7, 2021

View reviewed changes

Sjors mentioned this pull request Apr 8, 2021

Auto select wallet after adding #28

Closed

w-i-n-s force-pushed the feature_persistent_store branch from 5cd9c8a to 23fc709 Compare April 19, 2021 07:56

w-i-n-s marked this pull request as ready for review April 19, 2021 07:56

w-i-n-s changed the title ~~WIP: Feature persistent store~~ Feature persistent store Apr 19, 2021

Sjors reviewed Apr 19, 2021

View reviewed changes

w-i-n-s force-pushed the feature_persistent_store branch from 23fc709 to c839a4d Compare April 22, 2021 19:15

w-i-n-s added 6 commits April 22, 2021 22:27

Add wallet and cosigner items into cd model

f5be6c2

add persistent store,

2d3984a

add addresses mockups, fix address views to operate with CD entities

remove unused code

5b8e152

refactoring to MVVM architecture

fa38064

add wallets view

ea14dab

add another wallet and cosigners mocks

7e926e7

w-i-n-s added 25 commits April 22, 2021 22:31

fix wallet list preview settings

9b1df98

remove misplaced Mainnet switcher,

0dacd1f

fix preview settings

add codesigner model,

c6a2592

prepare for handle scans

add cosigners support,

8d4a7a7

fix default wallet selection

fix fingerprints keys to be String type

6de519d

add save store feature

e7258be

fix add wallet flow,

c636e69

fix wallet selection

add remove wallet by long press feature

6d8bb11

fix to save generic id instead of received descriptor for selected wa…

f880dcc

…llet in user defaults

add error on loading presenting feature

504e7ab

fix UI text,

d250eed

move threshold to co-signers section

fix close QR scanner after scan

559f412

fix address generation with taking into account all cosigners for sel…

535422c

…ected wallet

fix network selection for wallet based on co-signers xpubs

25b6f4b

remove fingerprint separation by network

34ad5b2

fix network identification

b8e019e

add check for prevent adding a duplicated wallets

8bf78f4

remove unused files

f54c5d9

add moskup for PSBT

7eaa0b5

remove conformance to NSCoding protocol

af94739

remove unused code

baf7465

add wrapper around cosigner's HDKey

7bab9dd

remove PSBT manager and reassign to sign view model,

d374259

refactoring SignView and it model to FSM, refactor SignViewController from UIHostingController to regular view controller, remove threshold from UserDefaults, remove Cosigners from Signer

move CD model into Model folder

907c8d5

w-i-n-s force-pushed the feature_persistent_store branch from c839a4d to 907c8d5 Compare April 22, 2021 19:33

github-pages bot temporarily deployed to github-pages April 23, 2021 08:35 Inactive

Sjors merged commit 907c8d5 into Sjors:master Apr 23, 2021

Sjors mentioned this pull request Apr 23, 2021

Add signet feature #37

Merged

2 tasks

w-i-n-s deleted the feature_persistent_store branch April 23, 2021 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature persistent store #22

Feature persistent store #22

w-i-n-s commented Mar 12, 2021 •

edited

Sjors commented Mar 25, 2021 •

edited

Sjors left a comment

Sjors left a comment

w-i-n-s commented Apr 6, 2021

Sjors commented Apr 6, 2021

w-i-n-s commented Apr 6, 2021 •

edited

Sjors commented Apr 6, 2021 •

edited

w-i-n-s commented Apr 6, 2021 •

edited

Sjors Apr 7, 2021

w-i-n-s Apr 8, 2021 •

edited

Sjors Apr 7, 2021

Sjors Apr 7, 2021

w-i-n-s Apr 8, 2021

Sjors commented Apr 8, 2021

w-i-n-s commented Apr 9, 2021 •

edited

Sjors commented Apr 9, 2021

w-i-n-s commented Apr 19, 2021

Sjors commented Apr 19, 2021 •

edited

Sjors Apr 19, 2021 •

edited

w-i-n-s Apr 22, 2021

Feature persistent store #22

Feature persistent store #22

Conversation

w-i-n-s commented Mar 12, 2021 • edited

Sjors commented Mar 25, 2021 • edited

Sjors left a comment

Choose a reason for hiding this comment

Sjors left a comment

Choose a reason for hiding this comment

w-i-n-s commented Apr 6, 2021

Sjors commented Apr 6, 2021

w-i-n-s commented Apr 6, 2021 • edited

Sjors commented Apr 6, 2021 • edited

w-i-n-s commented Apr 6, 2021 • edited

Sjors Apr 7, 2021

Choose a reason for hiding this comment

w-i-n-s Apr 8, 2021 • edited

Choose a reason for hiding this comment

Sjors Apr 7, 2021

Choose a reason for hiding this comment

Sjors Apr 7, 2021

Choose a reason for hiding this comment

w-i-n-s Apr 8, 2021

Choose a reason for hiding this comment

Sjors commented Apr 8, 2021

w-i-n-s commented Apr 9, 2021 • edited

Sjors commented Apr 9, 2021

w-i-n-s commented Apr 19, 2021

Sjors commented Apr 19, 2021 • edited

Sjors Apr 19, 2021 • edited

Choose a reason for hiding this comment

w-i-n-s Apr 22, 2021

Choose a reason for hiding this comment

w-i-n-s commented Mar 12, 2021 •

edited

Sjors commented Mar 25, 2021 •

edited

w-i-n-s commented Apr 6, 2021 •

edited

Sjors commented Apr 6, 2021 •

edited

w-i-n-s commented Apr 6, 2021 •

edited

w-i-n-s Apr 8, 2021 •

edited

w-i-n-s commented Apr 9, 2021 •

edited

Sjors commented Apr 19, 2021 •

edited

Sjors Apr 19, 2021 •

edited