Implement persistence with the new structures#965
Implement persistence with the new structures#965evanlinjin merged 2 commits intobitcoindevkit:masterfrom
Conversation
da5054a to
6752b69
Compare
b23bcaa to
6ac7d25
Compare
d2e7b48 to
a427a6d
Compare
46259f5 to
f8dd5e2
Compare
This is a more generic version of `keychain::persist::*` structures. Additional changes: * The `Append` trait has a new method `is_empty`. * Introduce `Store` structure for `bdk_file_store`.
f8dd5e2 to
2aa08a5
Compare
LLFourn
left a comment
There was a problem hiding this comment.
I left a few nits. Otherwise LGTM.
| fn append(&mut self, other: Self); | ||
|
|
||
| /// Returns whether the structure is considered empty. | ||
| fn is_empty(&self) -> bool; |
There was a problem hiding this comment.
Let's say I wanted to implement Append for a u32 to represent a monotonically increasing value -- I guess u32::MIN would return true for is_empty.
There was a problem hiding this comment.
I think implementing Append for u32 is a rather confusing combination. Option<u32> or using a wrapped type makes more sense for me.
935dc16 to
4963240
Compare
| #[derive(Debug)] | ||
| pub struct Persist<B, C> { | ||
| backend: B, | ||
| stage: C, |
There was a problem hiding this comment.
nit: maybe it should be S for stage, instead of C?
There was a problem hiding this comment.
C for changeset though 😮
There was a problem hiding this comment.
Then do changeset: C :P
There was a problem hiding this comment.
I support changeset: C 🙂
| let mut temp = C::default(); | ||
| core::mem::swap(&mut temp, &mut self.stage); | ||
| self.backend.write_changes(&temp) |
There was a problem hiding this comment.
Mhhhh maybe put a small comment here, it took me a second to figure out what you were trying to do (commit changes and clean self.stage at the same time)
Also, a shorter way of writing this, but not necessarily a cleaner one, might be:
self.backend.write_changes(core::mem::take(&mut self.stage))
There was a problem hiding this comment.
actually shouldn't we only clear it in the case that it succeeds.
There was a problem hiding this comment.
/// Commit the staged changes to the underlying persistance backend.
///
/// Changes that are committed (if any) are returned.
///
/// # Error
///
/// Returns a backend-defined error if this fails.
pub fn commit(&mut self) -> Result<Option<C>, B::WriteError> {
if self.stage.is_empty() {
return Ok(None);
}
self.backend
.write_changes(&self.stage)
// if written successfully, take and return `self.stage`
.map(|_| Some(core::mem::take(&mut self.stage)))
}| db_file: Option<&'t mut File>, | ||
|
|
||
| /// The file position for the first read of `db_file`. | ||
| start_pos: Option<u64>, |
There was a problem hiding this comment.
nit: maybe expand in the docs why it's an Option and what happens when this is None
Description
This is part of #895 and #971
keychain::persist::*structures that only needs a single generic for the changeset type.Additional changes:
Appendtrait has a new methodis_empty.Storestructure forbdk_file_store(which implementsPersistBackend).Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: