feat: add Kyoto #296
feat: add Kyoto #296
Conversation
rubensmachion
added
enhancement
There was a problem hiding this comment.
Really good! I know we probably talked a lot about this in our call earlier but I added some comments to the PR.
Some more fine tuning to be done but very cool that we tested and saw this working already!
UI:
- using https://developer.apple.com/design/human-interface-guidelines/progress-indicators Progress bar and percentage on WalletView if using Kyoto.
- adding a network icon on WalletView if using Kyoto.
- can remove Full Scan in SettingsView if using Kyoto.
- we might want to remove any option other than
.signetif using Kyoto on OnboardingView
Misc:
- Remove printLogs and updateWarn potentially before merging this
| Picker("Sync type", selection: $viewModel.syncMode) { | ||
| Text("Esplora Server").tag(SyncMode.esplora) | ||
| Text("Kyoto").tag(SyncMode.kyoto) |
There was a problem hiding this comment.
I'm seeing this (bug?) when I load OnboardingView for the first time, where I think maybe its supposed to say "Esplora Server" as the default here? Maybe that's not the issue, but attached is a gif of what I'm seeing
| Group { | ||
| if viewModel.syncMode == .kyoto { | ||
| Section(header: Text("Network")) { | ||
| Text("Using Kyoto") |
There was a problem hiding this comment.
| Text("Using Kyoto") | |
| Text("Kyoto") |
| case "bitcoin": self = .bitcoin | ||
| case "bitcoin": self = .signet |
There was a problem hiding this comment.
Not sure why this was changed from .bitcoin to .signet?
| case esplora, kyoto | ||
| } | ||
|
|
||
| protocol BDKSyncService { |
There was a problem hiding this comment.
would like to use a concrete type instead of protocol
happy to collaborate with you to do this, because I do think it would be nice to stay consistent with existing way the codebase tries to do this
reason for preference for concrete types over protocols comes a lot from pointfree folks (for example, see comment from Brandon pointfreeco/swift-composable-architecture#522)
| @@ -93,4 +95,13 @@ extension Network { | |||
| Constants.Config.EsploraServerURLNetwork.Testnet4.allValues.first ?? "" | |||
| } | |||
| } | |||
|
|
|||
| var taprootHeight: UInt32 { | |||
There was a problem hiding this comment.
If we remove "Full Scan" do we still need taprootHeight? Maybe we do, but from what I'm seeing taprootHeight is only called from the code where we use full scan (correct me if I'm wrong about any of this though!)
| @@ -62,6 +62,23 @@ private struct KeyService { | |||
| func saveNetwork(network: String) throws { | |||
| keychain[string: "SelectedNetwork"] = network | |||
| } | |||
|
|
|||
| func deletaAllData() throws { | |||
There was a problem hiding this comment.
nit: spelling
| func deletaAllData() throws { | |
| func deleteAllData() throws { |
Description
This PR proposes to demonstrate an implementation of Kyoto while keeping the Esplora server.
[issue] #293
Notes to the reviewers
This implementation proposes adding Kyoto synchronization while keeping the Esplora server, to demonstrate to the user how we can support both synchronization methods.
To encapsulate Kyoto and Esplora, it introduces a protocol called BDKSyncService, which is used by the BDKService client.
Checklists
All Submissions:
.swift-formatfileNew Features:
Bugfixes: