Abstract wallet persistence with external persistence providers

[dr-orlovsky]

Closes #11, #54, #56

Addresses RGB-WG/RFC#11

I have tried all possible approaches and this is the only one which was possible to actually implement in practice. Unfortunately it took some tall on the API: now all Wallet objects became no-Clone, due to adding Box<dyn PersistenceProvider>: if the PersistenceProvider: Clone than it can't be made into an object and can't be used in that way. UPD: I have mitigated the problem by adding a new trait CloneNoPersistence and implementing it for all wallet structures. Such clones will not "remember" how to persist the data, but the persistence can be restored on them by using make_persistent API.

Adding persistence provider as a generic field also doesn't work: the scale of changes becomes so dramatic that everything using BP wallet would have to be literally rewritten.

The recommended way of using the APIs for the non-blocking I/O: RGB-WG/RFC#12

Asking @will-bitlight and @Matrix-Zhang to provide a feedback on the changes.

@zoedberg and @nicbus please let me know if you need help in updating your repositories - I can work on the PRs there

In the meanwhile I will be updating rgb-std and rgb repos to match the changes here.
Two other related PRs:

• Add store providers to persistence module RGB-WG/rgb-std#251
• Use new StoreProvider APIs RGB-WG/rgb#238

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 209 lines in your changes missing coverage. Please review.

Project coverage is 0.0%. Comparing base (13d59a7) to head (95a3a37).
Report is 17 commits behind head on master.

Files with missing lines	Patch %	Lines
src/wallet.rs	0.0%	137 Missing ⚠️
src/fs.rs	0.0%	53 Missing ⚠️
src/cli/command.rs	0.0%	5 Missing ⚠️
src/layer2.rs	0.0%	5 Missing ⚠️
src/cli/args.rs	0.0%	4 Missing ⚠️
src/indexers/electrum.rs	0.0%	4 Missing ⚠️
src/indexers/esplora.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##           master    #66   +/-   ##
=====================================
  Coverage     0.0%   0.0%           
=====================================
  Files          23     24    +1     
  Lines        2070   2159   +89     
=====================================
- Misses       2070   2159   +89     

Flag	Coverage Δ
rust	0.0% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dr-orlovsky]

I have mitigated the problem by adding a new trait CloneNoPersistence and implementing it for all wallet structures. Such clones will not "remember" how to persist the data, but the persistence can be restored on them by using make_persistent API.

[zoedberg]

@zoedberg and @nicbus please let me know if you need help in updating your repositories - I can work on the PRs there

Thanks, I'll try to do it by myself and ask help if needed.

Right now it seems bp-wallet doesn't compile. You can run cargo test on this branch https://github.com/zoedberg/rgb-integration-tests/tree/try_store_prs to see the errors, that are:

error[E0599]: no method named `clone` found for type parameter `D` in the current scope
   --> bp-wallet/src/wallet.rs:155:39
    |
151 | impl<K, D: Descriptor<K>, L2: Layer2Descriptor> CloneNoPersistence for WalletDescr<K, D, L2> {
    |         - method `clone` not found for this type parameter
...
155 |             generator: self.generator.clone(),
    |                                       ^^^^^ method not found in `D`
    |
    = help: items from traits can only be used if the type parameter is bounded by the trait
help: the following trait defines an item `clone`, perhaps you need to restrict type parameter `D` with it:
    |
151 | impl<K, D: Descriptor<K> + Clone, L2: Layer2Descriptor> CloneNoPersistence for WalletDescr<K, D, L2> {
    |                          +++++++

error[E0599]: the method `to_string` exists for reference `&D`, but its trait bounds were not satisfied
   --> bp-wallet/src/wallet.rs:278:36
    |
278 |             descriptor: descriptor.to_string(),
    |                                    ^^^^^^^^^ method cannot be called on `&D` due to unsatisfied trait bounds
    |
    = note: the following trait bounds were not satisfied:
            `D: std::fmt::Display`
            which is required by `D: ToString`
            `&D: std::fmt::Display`
            which is required by `&D: ToString`
    = help: items from traits can only be used if the type parameter is bounded by the trait
help: the following trait defines an item `to_string`, perhaps you need to restrict type parameter `D` with it:
    |
275 |     pub(crate) fn new_nonsync<K, D: Descriptor<K> + ToString>(descriptor: &D) -> Self {
    |                                                   ++++++++++

All dependencies are the beta 7 ones, except for:

• amplify-nonasync (unreleased, checked out master commit b1bf354)
• bp-wallet (checked out store commit 5dc38aa)
• rgb (checked out store commit e4c744f)
• rgb-std (checked out store commit 10690ef)
• rgb-core (checked out nonce commit 1f6e9823)
• rust-amplify (I've tried both with v4.7.0 and v5.0.0-beta.1, but the errors don't change)

[dr-orlovsky]

Can you pls check all the patches I am using in ray-server - the repo I shared with you? BP wallet certainly compiles with all them applied

[zoedberg]

By checking out bp-std to the store branch (commit efb5b67) bp-wallet compiles. @dr-orlovsky Maybe you should open a PR for the store branch in bp-std?

[dr-orlovsky]

Yep, forgot about PR for it

[dr-orlovsky]

Done: BP-WG/bp-std#40

[zoedberg]

Here zoedberg/rgb-tests@45932e7 I've updated the integration tests to use the refactored persistence flow. It compiles and all tests pass, but I've noticed an unexpected behavior: if I create the directory for bp-wallet (std::fs::create_dir_all(&bp_dir).unwrap();) the tests correctly save the wallet-related data, but if I don't create it no error is raised and the wallet-related data doesn't get saved. I expected that make_persistent would fail saying there's no directory or that it would create the directory.

P.S. I'm seeing also some lint warnings:

warning: unused variable: `e`
   --> bp-wallet/src/wallet.rs:175:24
    |
175 |             if let Err(e) = self.store() {
    |                        ^ help: if this is intentional, prefix it with an underscore: `_e`
    |
    = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `e`
   --> bp-wallet/src/wallet.rs:239:24
    |
239 |             if let Err(e) = self.store() {
    |                        ^ help: if this is intentional, prefix it with an underscore: `_e`

warning: unused variable: `e`
   --> bp-wallet/src/wallet.rs:373:24
    |
373 |             if let Err(e) = self.store() {
    |                        ^ help: if this is intentional, prefix it with an underscore: `_e`

warning: `bp-wallet` (lib) generated 3 warnings

[dr-orlovsky]

About the warning: the system reports via log, and to have that reporting you have to use log feature.

I have pushed commit to store branches of both bp-wallet and rgb-std which forces creation of the directory structure when the storage providers are constructed (and their constructors are now failable). It shouldn't be done in make_provider since it relies on the fact that the storage provider is already constructed and valid.

[zoedberg]

About the warning: the system reports via log, and to have that reporting you have to use log feature.

Code should be linted regardless all features being enabled. Warnings will disappear by changing:

if let Err(e) = self.store() {
    #[cfg(feature = "log")]
    log::error!(
        "impossible to automatically-save wallet cache during the Drop operation: {e}"
    );
}

to:

if let Err(_e) = self.store() {
    #[cfg(feature = "log")]
    log::error!(
        "impossible to automatically-save wallet cache during the Drop operation: {_e}"
    );
}

I have pushed commit to store branches of both bp-wallet and rgb-std which forces creation of the directory structure when the storage providers are constructed (and their constructors are now failable). It shouldn't be done in make_provider since it relies on the fact that the storage provider is already constructed and valid.

I've tried the updated code and confirm this now works as expected.

[dr-orlovsky]

Warnings will disappear by changing:

we just need to add #[allow(unused_variables)] on top of the if statement. I will do it in another commit which I will push here.