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

Improve docs in terms of persistence with Wallet #1542

Open
evanlinjin opened this issue Aug 8, 2024 · 3 comments
Open

Improve docs in terms of persistence with Wallet #1542

evanlinjin opened this issue Aug 8, 2024 · 3 comments
Labels
documentation Improvements or additions to documentation
Milestone

Comments

@evanlinjin
Copy link
Member

Describe the enhancement

  • Explain and give examples of implementing PersistWith<CustomDb> for Wallet.
  • Explain when to call .persist on Wallet.

cc. @tnull

@evanlinjin evanlinjin added the new feature New feature or request label Aug 8, 2024
@tnull
Copy link
Contributor

tnull commented Aug 8, 2024

Thank you! Preferably, this wouldn't only be reflected in docs, but also in the API, e.g, either allowing us to get notified when persistence needs to happen or by returning a corresponding enum PersistenceNeeded or similar.

Also, please add guidance how to deal with persistence failures. Do we need to abort the current operation? Can we retry? Until when? Are there circumstances under which we need to panic as continuing without persistence would be considered highly unsafe?

@evanlinjin
Copy link
Member Author

When persist needs to happen

Isn't this up to the caller to decide? We just need to inform the caller which methods would reveal a new address/spk (since if we don't persist immediately, it will result in address reuse). Everything else, can be delayed (with no catastrophic consequences) if changes are not persisted.

Also, please add guidance how to deal with persistence failures.

The persistence backend is responsible for returning errors which are informative and hopefully it can be dealt with.

Are there circumstances under which we need to panic as continuing without persistence would be considered highly unsafe?

Unsafe in what regard? The database is left corrupted? The database data is left in an inconsistent state? -> There are all persistence-backend-specific right?

Determining whether data not being persisted would lead to something disastrous, isn't this dependent on the application?

@tnull
Copy link
Contributor

tnull commented Aug 12, 2024

Isn't this up to the caller to decide? We just need to inform the caller which methods would reveal a new address/spk (since if we don't persist immediately, it will result in address reuse). Everything else, can be delayed (with no catastrophic consequences) if changes are not persisted.

The persistence backend is responsible for returning errors which are informative and hopefully it can be dealt with.

Determining whether data not being persisted would lead to something disastrous, isn't this dependent on the application?

I mean yes, I mostly agree with all three of these points, but the API docs should still highlight what the consequences of address reuse (or, FWIW, any other edge cases arising from BDK's state not being immediately/correctly persisted) might be. Yes, it's application-specific, but it still would be important to make the API as safe as possible and the docs as accommodating to users with different knowledge levels as possible (without becoming overly verbose, of course).

@notmandatory notmandatory added this to the 1.0.0-beta milestone Aug 13, 2024
@notmandatory notmandatory added documentation Improvements or additions to documentation and removed new feature New feature or request labels Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Todo
Development

No branches or pull requests

3 participants