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

Update bdk cli for wasm #117

Conversation

danielabrozzoni
Copy link
Member

@danielabrozzoni danielabrozzoni commented Sep 5, 2022

Description

This PR adds a module to bdk-cli that exposes some structures/functions to WASM. You can see it in use here: bitcoindevkit/bitcoindevkit.org#118

Notes to the reviewers

Based on #116

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@danielabrozzoni danielabrozzoni marked this pull request as draft September 5, 2022 13:32
src/wasm.rs Outdated Show resolved Hide resolved
Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

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

I would also add a CI job for wasm (just building the code, no tests) so that we don't forget to keep this updated going forward

src/wasm.rs Outdated Show resolved Hide resolved
src/wasm.rs Outdated Show resolved Hide resolved
@danielabrozzoni danielabrozzoni force-pushed the fix/update_bdk_cli_for_wasm branch 2 times, most recently from 44a04ec to eef619e Compare September 20, 2022 15:42
@notmandatory notmandatory added this to the Release 0.7.0 milestone Sep 21, 2022
@notmandatory
Copy link
Member

I added this to the 0.7.0 milestone since it's the next big thing worth making a release for.

@danielabrozzoni
Copy link
Member Author

The code is almost ready (I'm working on fixing the CI), do you think we could squeeze it in the 0.6.0 release? I'd like to have the playground updated as soon as possible, as if you use it nowadays it generates insecure descriptors :)

Add a module "wasm" with utilities to be used in the bdk playground:
- A WasmWallet structure, to create a wallet and run commands
- A compile function, to compile policies into descriptors
.github/workflows/cont_integration.yml Outdated Show resolved Hide resolved
src/wasm.rs Show resolved Hide resolved
@notmandatory
Copy link
Member

notmandatory commented Sep 23, 2022

The code is almost ready (I'm working on fixing the CI), do you think we could squeeze it in the 0.6.0 release? I'd like to have the playground updated as soon as possible, as if you use it nowadays it generates insecure descriptors :)

Hey sorry I just saw this comment. If this one is just about ready to go then probably better to put it in 0.6.0 then have to do a 0.7.0 right after 0.6.0. That sound OK @rajarshimaitra ?

@rajarshimaitra
Copy link
Contributor

Sounds good to me.. If this is almost ready we can push it in 0.6.. I will put in a review too soon..

@rajarshimaitra
Copy link
Contributor

On the initial look the code LGTM.. I just have one conceptual question..

Is there any wasm related technical problem of just using this function here and expose it via bindgen?

pub(crate) fn handle_command(cli_opts: CliOpts) -> Result<String, Error> {

All the wallet structures and everything else is created internally within this function, so in theory we won't need to have dedicated wasm related structure definition. That was my motivation behind simplifying the handle_command function to only take a CliOpts which is a structured list of strings, same as command line arguments. So a wasm app can just play the function and depending on what it needs to do pass in corresponding string arguments?

I haven't tried this out myself, so wondering if there's something specific to wasm that doesn't allow us to do that?? The purpose of handle_command is one single function to operate everything bdk-cli..

@afilini
Copy link
Member

afilini commented Sep 24, 2022

For sure the type is not compatible with wasm, you can't get a CliOpts directly from wasm.

The second issue is that in wasm only the MemoryDatabase is supported, which forces you to use "repl style" commands. handle_command recreates the wallet and the database every time, which means you would have an empty database every time and it would be impossible to create transactions, see the balance, etc.

@rajarshimaitra
Copy link
Contributor

rajarshimaitra commented Sep 24, 2022

Thanks @afilini ya that makes sense.. So there is no easy way to do this without the extra structures.. This one is good to go modulo your comments.. Or can they be added in separate PR and we merge this for the next release?

@danielabrozzoni
Copy link
Member Author

danielabrozzoni commented Sep 25, 2022

The second issue is that in wasm only the MemoryDatabase is supported, which forces you to use "repl style" commands. handle_command recreates the wallet and the database every time, which means you would have an empty database every time and it would be impossible to create transactions, see the balance, etc.

+1, and also creating the db/blockchain requires creating directories (see prepare_home_dir), and you can't do that on WASM

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

ACK 731dc74

@rajarshimaitra rajarshimaitra merged commit b5f9177 into bitcoindevkit:master Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants