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

[DRAFT] Implement Ledger Support #1513

Draft
wants to merge 19 commits into
base: dev
Choose a base branch
from

Conversation

pacu
Copy link
Collaborator

@pacu pacu commented Nov 14, 2024

@Oscar-Pepper and I debated a bit on how to integrate ledger under the new UnifiedKeyStore architecture.

This PR will be implementing those changes gradually. There are a lot of build problems that are blocking the full integration and need to be solved but all the structural code here will work around that

Kickstarting ideas from Pepper

so i had a look through the the zondax PR and my opinion is that we shouldnt be duplicating the WalletCapability, it has many methods that just re-implements code thats already in the codebase which is harder to maintain. a lot of the code around derivation paths and address generation is standardised so I think its best we try to use current zingolib code as much as possible and integrate the ledger instead of tacking it on. My approach would be to have a feature gated struct in WalletCapability that holds any novel ledger information such as the ZcashApp and LedgerId. the current wallet capability can be used to hold the addresses and keys etc.

I think we can have the WalletBase::Ledger be similar to WalletBase::Ufvk except it connects to the ledger to retrieve the keys instead of a user inputted encoded UFVK string on startup. we can also feature gate the write impl for keys to stop the UFVK being written to file, so its loaded to memory on startup only

@pacu
Copy link
Collaborator Author

pacu commented Nov 14, 2024

@Oscar-Pepper here's the point where the single WalletCapability falls short.

I think I can apply a refactor where we extract the inner workings of WalletCapability into an internal trait that implements a visitor pattern that calls the corresponding variant without exposing that there is a variant WalletCapability

@zancas
Copy link
Member

zancas commented Nov 15, 2024

How is this related to #1352 ? Does this supersede that PR? Should that PR still be open?

@pacu
Copy link
Collaborator Author

pacu commented Nov 15, 2024

How is this related to #1352 ? Does this supersede that PR? Should that PR still be open?

That one unfortunately has been made obsolete by the new UnifiedKeystore work. I'm using it as reference only.

@pacu pacu force-pushed the feature/ledger-support branch 2 times, most recently from 310761a to 3d2caf5 Compare November 18, 2024 18:38
@pacu
Copy link
Collaborator Author

pacu commented Nov 18, 2024

@zancas @Oscar-Pepper I rebased and updated this PR

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.

2 participants