-
Notifications
You must be signed in to change notification settings - Fork 403
Add functions to test persistence #2012
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
base: master
Are you sure you want to change the base?
Add functions to test persistence #2012
Conversation
8f06dc6
to
247d478
Compare
247d478
to
83b54a6
Compare
I tried re-running CI to no avail, maybe try force pushing. |
83b54a6
to
51d148e
Compare
Great idea. ConceptACK |
Added the following functions to test if persistence of `bdk_chain` is happening correctly. - `persist_txgraph_changeset` - `persist_indexer_changeset` - `persist_local_chain_changeset` - `persist_last_seen`, `persist_last_evicted`, `persist_first_seen` - `persist_txouts` - `persist_txs` - `persist_anchors` - `persist_last_revealed` - `persist_spk_cache` Even though the first three tests cover every part of the `ChangeSet`s , the other tests are retained so as to help in unit testing.
Since there is no way we can enable miniscript/no-std for bdk_chain::miniscript, cargo throws up an error when compiling with `--no-default-features`. Adding miniscript as another dependency does not resolve the issue because bdk_chain/miniscript is also required. Removed unnecessary links from docs as just learned that docs.rs can infers the links to dependencies.
f2623db
to
b77a114
Compare
In 10c42dc I added a |
We could have the testenv default feature enable |
Should the miniscript feature introduced be removed? I was thinking if some users might not want bdk_chain/miniscript feature to be enabled? |
b77a114
to
1eee95b
Compare
The miniscript feature is retained so as to not be coupled with "download".
1eee95b
to
fd9429c
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the correct thing to do is something like this by making the dependency explicit.
[dependencies]
miniscript = { version = "12.3.5", default-features = false, optional = true }
[features]
default = ["std" , "download", "bdk_chain/default"]
miniscript = ["dep:miniscript"]
std = ["bdk_chain/std", "miniscript?/std"]
serde = ["bdk_chain/serde", "miniscript?/serde"]
But a simpler hack is to just have default = ["bdk_chain/default"]
(which seems to be missing and should probably be done anyway). In that case the cfg attribute would be changed to #[cfg(feature = "default"]
. Sorry to cause more bikeshedding on the matter. I'll be in favor of whatever is clear and easy to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with the second approach since the first seemed a bit complicated 😅 (like the test utils are gated by miniscript feature which might not be enabled even though all we need is bdk_chain/default).
|
The feature now depends on `bdk_chain/default`. Now default feature gates the persistence test utils rather than std.
Description
Added basic tests to the testenv for testing custom persistence backends. Partially solves bdk_wallet#14 and might help with bdk_wallet#234.
Notes to the reviewers
An alternative approach could be to create a struct whose methods are these tests but the current approach seems better since these tests are meant to be used as unit tests. For the same reason redundant functions to check persistence of individual fields of the
ChangeSet
s are provided.Update
default
feature of testenv to specifybdk_chain/default
as the only optional dependency.default
feature.Changelog notice
Todo
Checklists
All Submissions:
New Features: