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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement BDK Database types for gloo::LocalStorage #5

Merged
merged 8 commits into from Oct 25, 2022
Merged

Implement BDK Database types for gloo::LocalStorage #5

merged 8 commits into from Oct 25, 2022

Conversation

benthecarman
Copy link
Collaborator

This has a lot of copy pasting from ldk lite and bdk. I copied ldk lite's Error enum, some of bdk's private helpers for key-value and memory database stuff, and bdk's helpers for testing the database.

One caveat to this is that the database does not actually batch (we just immediately write to LocalStorage, I'm not sure if that will break anything, but should be enough to get started on doing bdk stuff 馃殌

node-manager/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@futurepaul futurepaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is so epic it might be nice to do a call to review a bit / make sure I understand it. but overall it looks super clean.

if you run cargo clippy you get some suggestions about unnecessary clones and formats

also some unused imports and dead code

probably a good practice to handle most warnings before merging if possible?

node-manager/src/error.rs Show resolved Hide resolved
node-manager/src/localstorage.rs Outdated Show resolved Hide resolved
node-manager/src/localstorage.rs Outdated Show resolved Hide resolved
node-manager/src/lib.rs Outdated Show resolved Hide resolved
node-manager/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@futurepaul futurepaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay I'm happy with this. I'd like to kick the tires from the application code but pretty stoked to have so much bdk stuff in here already

node-manager/src/localstorage.rs Outdated Show resolved Hide resolved
@benthecarman
Copy link
Collaborator Author

Updated bdk version to bitcoindevkit/bdk#770 as it seems it does not work with wasm without it

@TonyGiorgio
Copy link
Contributor

Now that we're using nightly, should we add that to the project configurations? https://stackoverflow.com/a/71978441

Copy link
Contributor

@TonyGiorgio TonyGiorgio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done man, looking good.

@benthecarman benthecarman merged commit b519d2a into MutinyWallet:master Oct 25, 2022
@benthecarman benthecarman deleted the bdk-db branch October 25, 2022 17:07
@notmandatory
Copy link

Updated bdk version to bitcoindevkit/bdk#770 as it seems it does not work with wasm without it

Yes you will need the next release of bdk (0.24.0) to work easily with Wasm. There is a way to work around it (see bitcoindevkit/bdk#646 for the whole story), but if you can wait a week we'll have the official release out.

@notmandatory
Copy link

BTW it's very cool you got this working!! 馃ぉ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants