Multi-Keychain-Redo#1
Open
110CodingP wants to merge 4 commits into
Open
Conversation
The KeyRing<K> struct was introduced with the following methods: new, network, add_descriptor, add_multipath_descriptor, add_multipath_descriptor_with_range, into_params, list_keychains. The descriptors field within the KeyRing struct is specifically designed for O(1) membership checks. `KeyRing::add_descriptors` was intentionally not included because it is difficult to provide atomicity guarantees without introducing overly complicated logic. Also, while `KeyRing::add_multipath_with_range` is currently internal, it should be made `pub` in the future if we can incorporate a generic range parameter in an elegant manner.
Introduced `InitError` to report errors at the time of `Wallet` creation since `DescriptorError` does not report duplicate keychains case now. Also introduced `MissingKeychain` error to report the case when address APIs are called with an invalid keychain. `CreateParams::multi_path_descriptor_with_range` follows the logic of `KeyRing::add_multipath_descriptor_with_range`. Removed `ExternalAndInternalAreTheSame` variant from `Error` in `descriptor` in order to avoid propagating the generic to `IntoWalletDescriptor`. Also maintaining whether keychains are the same should be the concern of the `Wallet` level and not the descriptor level. The `from_v3` and `to_v3` APIs are for backwards and forward compatibility respectively such that the `load_from_params` does not need to deal with 2 separate types of `ChangeSets`. Users should call these APIs appropriately. `load_from_v3`, `load_from_v3_async`, `persist_to_v3`, and `persist_to_v3_async` have been added to `PersistedWallet` for users convenience. Refactor of export.rs is NOT COMPLETE. `Utxo` and `WeightedUtxo` were primarily used in `TxBuilder` contexts so they still use `KeychainKind`. Note we have decided that transaction building for multi-keychain `Wallet` will only happen through `create_psbt` API. `Merge` implementation for `ChangeSet` does not take into account the `descriptor` and `change_descriptor` fields because ideally `v4` completely ignores these fields. But `from_sqlite` and `persist_to_sqlite` still load and persist these two fields for backwards/forwards compatibility.
The tests related to signers are yet to be fixed completely.
The new example (electrum_v3) is just for testing purposes and should be removed from the final version of the commit.
110CodingP
force-pushed
the
multi_keychain_redo
branch
from
fa470d1 to
3b89212
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Acc to VM's suggestions.
Notes to the reviewers
Particularly need review on
KeyRingand its APIsfrom_v3andto_v3(for backwards and forwards compatibility respectively) [tested with a db from v3, checkelectrum_v3example.]Mergeimplementation forChangeSet.schema_v2,from_sqliteandpersist_to_sqlite(inchangeset.rs)load_from_params(inwallet/mod.rs)