Skip to content

feat: add Kyoto #296

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

Closed

Conversation

rubensmachion
Copy link
Collaborator

@rubensmachion rubensmachion commented May 25, 2025

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:

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • UI changes tested on small, medium, and large devices to ensure layout consistency

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@rubensmachion rubensmachion self-assigned this May 25, 2025
@rubensmachion rubensmachion added the WIP working in progress label May 25, 2025
@rubensmachion
Copy link
Collaborator Author

Hi @reez,
I think this PR is ready for a new review. I’ve completed all the appointment-related work and fixed other bugs I found along the way.

I created two new extensions for BDKClient to organize the static types of Esplora and Kyoto.
For now, I’ve kept the BDKSyncService protocol so I can reuse some functions like buildWallet, buildBackupInfo, deleteWallet, etc.

Please let me know what you think. If needed, I can refactor the protocol into a concrete type.

@rubensmachion rubensmachion marked this pull request as ready for review June 17, 2025 14:59
@rubensmachion rubensmachion changed the title (draft) feat: add Kyoto feat: add Kyoto Jun 17, 2025
@rubensmachion rubensmachion requested a review from reez June 17, 2025 15:05
}
}

func loadWalleFromBackup() throws -> Wallet {
Copy link
Collaborator

@reez reez Jun 17, 2025

Choose a reason for hiding this comment

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

spelling, missing "t"


needsFullScan = true
func getSyncMode() -> SyncMode? {
try? keyClient.getSyncMode()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whats the reason we save syncmode to keychain? Its probably something obvious I'm missing but I can't think of it at the moment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to store the SyncMode—either Kyoto or Esplora—somewhere so we can restore it the next time the app is opened. I chose to save it in the Keychain to avoid having to manage another way of persisting data in the app. Another option would be to store it in UserDefaults

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah the keychain part is not a problem, but the reason for needing to know the sync mode when the app is opened im still trying to chase down

func stopService() async throws
}

extension BDKSyncService {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't remember, what was the reason for removing a lot of this logic from BDKService and moving it into a BDKSyncService that helps the Kyoto part?

Copy link
Collaborator Author

@rubensmachion rubensmachion Jun 18, 2025

Choose a reason for hiding this comment

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

We need to manage the Wallet instance in both Kyoto and Esplora, so I centralized the common logic—such as buildWallet, buildBackupInfo, deleteWallet, etc.—into BDKSyncService, allowing both KyotoService and EsploraService to use it without duplicating code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok let me think on that a bit

@reez
Copy link
Collaborator

reez commented Jun 20, 2025

A lot of good changes, not all strictly necessary for Kyoto feature, I propose we keep this branch, have this branch available on the main repo for folks to use already (since seeing this all work is really cool), and then migrate this PR into main via smaller/concise pieces/PRs (which can make it easier to review & test separate changes in isolation).



I’m happy to do this since you’ve already done a great job getting this to work! I’ve already started by pushing your branch (with experiement/kyoto name) to the main repo https://github.com/bitcoindevkit/BDKSwiftExampleWallet/tree/experiment/kyoto and will let people know they should check out this branch to see things working already.







Next steps I think will be taken are starting with PRs to main with the bits of code refactored that don’t have to do with Kyoto (StorageUtils, URL+Extension, etc) but are really nice additions to the current codebase, and then saving Kyoto for last so that hopefully Kyoto just becomes a simple addition focused purely on the new feature/client.

@rubensmachion
Copy link
Collaborator Author

Totally agree with you @reez. If that’s okay with you, I can start creating other PRs with those additions (StorageUtils, URL+Extension, etc).

@reez
Copy link
Collaborator

reez commented Jun 20, 2025

Totally agree with you @reez. If that’s okay with you, I can start creating other PRs with those additions (StorageUtils, URL+Extension, etc).

Sure, you rule!

@reez
Copy link
Collaborator

reez commented Jun 20, 2025

Im going to close this PR (we can always re-open if needed) because we've got the branch live for people to use https://github.com/bitcoindevkit/BDKSwiftExampleWallet/tree/experiment/kyoto and we will be spinning this PR out into other PRs

@reez reez closed this Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants