Wallet: move database to sqlite, remove kv and memmory db#804
Wallet: move database to sqlite, remove kv and memmory db#804jaoleal wants to merge 4 commits intogetfloresta:masterfrom
Conversation
90f9da2 to
a5e86c3
Compare
|
a5e86c3: Im seeing some build related problems, it appears that |
ba07354 to
14231f8
Compare
|
37e383c: Okay, this commit fixed the build problem i was having. Basically what happened is that they were bundling pre-generated bindings and that was triggering a version compatibility problem basicaly because rust doesnt want to have a stable ABI, it can change trough versions affecting bindings and yada yada. They took that decision to avoid bigger compile times but that doesnt appear to be a big problem, well... To solve that i activated the feature that should generate the bindings on the go and it worked, otherwise i would need to find a rusqlite version that was bundling the correct bindings for our MSRV and pin it, and im not sure if that would support our case. If anyone want to further investigate to help me being sure that was the correct decision: |
6f0f109 to
05134fe
Compare
|
Notable changes on 57695dc
|
4252dbd to
090fe9d
Compare
c8199b4 to
47bd5c7
Compare
Davidson-Souza
left a comment
There was a problem hiding this comment.
Your commits are in reverse chronological order. What happened?
| self.database.save(&new_address); | ||
| self.database | ||
| .save(&new_address) | ||
| .expect("Database not working"); |
There was a problem hiding this comment.
Errors that can happens should either be handled or propagated
There was a problem hiding this comment.
again, that refers to another method that does not return a result and IMHO changing this method would be out-of-scope for this pr.
There was a problem hiding this comment.
I don't think it's out of scope, but I would recommend opening another pr before this one gets merged, improving this trait to return a result. Since you are touching this, I think it's better to do it right. The old method was infallible because it had an expect inside the DB wrapper, completely terrible!
Yeah, i reversed the commits because its easier to apply suggestions on the latest commit instead of the first... The old order was erroring while editing the first commit because of feature flags and deprecation changes |
47bd5c7 to
cfaa2ab
Compare
|
Applied @Davidson-Souza suggestions |
cfaa2ab to
e358b7f
Compare
|
Blank push -f, for some reason the CI just DIED |
|
It's rebase season |
a76a391 to
a874519
Compare
ba18458 to
db07edd
Compare
d31ca78 to
612b3a2
Compare
|
Forgot to update Cargo.lock |
612b3a2 to
6f604f5
Compare
| [dependencies] | ||
| bitcoin = { workspace = true, features = ["serde"] } | ||
| kv = { workspace = true } | ||
| rusqlite = { version = "0.38", optional = true, features = [ "bundled", "buildtime_bindgen" ], default-features = false } |
There was a problem hiding this comment.
I also think buildtime_bindgen is not needed nor relevant
There was a problem hiding this comment.
I had some problems with the bindgen at the beginning of this PR that made me include that feature, did you saw #804 (comment) ?
|
|
||
| [dev-dependencies] | ||
| rand = { workspace = true } | ||
| rusqlite = { version = "0.38", features = [ "bundled", "buildtime_bindgen" ], default-features = false } |
6f604f5 to
c5c9058
Compare
b1c7e3b to
bdd317a
Compare
|
@Davidson-Souza I made the af015f0 commit thats focused to address #804 (comment) and similar cases. @JoseSK999 I took a different approach to make the tables more readable, please take a look. |
af015f0 to
d8bef82
Compare
|
b274920 addresses @moisesPompilio suggestions about gating |
Replace panicking .expect() and .unwrap() calls in AddressCacheInner and AddressCache methods with proper error propagation.
d8bef82 to
ccbb10d
Compare
|
Rebased and Applied @JoseSK999 suggestions. |
Description and Notes
Addressing #665, I implemented the existing
AddressCacheDatabasetrait on top ofrusqlite, the only feature that the changes present is "bundled" which they declared to be better when regarding windows support.Regarding the implementation, it is pretty straightforward, I had the other two databases,
memmory_database.rsandkv_database.rsto be inspired. I think the tests are extensive and ensure the implementation runs fine, for every commit I made sure forjust lint-featuresandjust test-featureto also run fine.How to verify the changes you have done?
Review tests under
sqlite_database.rsand run them.Since this present a lot of features changes, running
just lint-featuresandjust test-featureis good.Contributor Checklist
just pcc(recommended but slower)just lint-features '-- -D warnings' && cargo test --releaseFinally, you are encouraged to sign all your commits (it proves authorship and guards against tampering—see How (and why) to sign Git commits and GitHub's guide to signing commits).