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

exposing get_funded_wallet #411

Merged

Conversation

ulrichard
Copy link
Contributor

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

@ulrichard ulrichard force-pushed the feature/expose_testutils branch from 2c03c43 to 29f32d8 Compare July 29, 2021 09:19
@notmandatory
Copy link
Member

notmandatory commented Jul 29, 2021

This looks good, my only concern is exposing test functions in the release builds for bdk. I think it should be possible to put any test utility functions behind a feature flag. I'm working on testing that in another fork and if I get it to works I'll make some suggestions on how to add something like a test-wallet feature as part of this PR or as a follow-up PR.

@notmandatory
Copy link
Member

notmandatory commented Jul 29, 2021

BTW, my PR to add the feature gates as a follow up to this PR is #412 Looks like enabling features only for tests isn't currently possible.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 29f32d8

@ulrichard
Copy link
Contributor Author

ulrichard commented Jul 30, 2021

Thanks for testing that stuff.
Yes rust-lang/cargo#2911 would be really cool.

@tcharding
Copy link
Contributor

Perhaps we should put this behind #[cfg(debug_assertions)] so its only available in debug builds?

@ulrichard
Copy link
Contributor Author

ulrichard commented Aug 2, 2021

Perhaps we should put this behind #[cfg(debug_assertions)] so its only available in debug builds?

That looks like a good idea. I quickly tried it and executed a few commands that came to my mind. The only one that failed was:
cargo test --release
But I don't know if that is actually used by anybody.

@afilini
Copy link
Member

afilini commented Aug 3, 2021

I think fuzzing is generally done by compiling to release mode with tests enabled. I don't think it's really a problem if it's available in release mode to be honest, I would just leave it in there

@ulrichard ulrichard force-pushed the feature/expose_testutils branch from 75b1b75 to 29f32d8 Compare August 3, 2021 14:33
@@ -1548,19 +1551,60 @@ where
}
}

/// Return a fake wallet that appears to be funded for testing.
pub fn get_funded_wallet(
Copy link
Member

Choose a reason for hiding this comment

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

Did you try with #[cfg(test)] to see if you could hide this to non-testing environments?

I didn't realize this yesterday, I thought this was still part of the test module, which is only present when testing is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I add this, then I get the following error in bdk-reserves: "unresolved import bdk::wallet::get_funded_wallet"

And if I try the following in bdk-reserves/Cargo.toml: "bdk = { ... features = ["electrum", "test"] }" then I get the following error: "the package bdk-reserves depends on bdk, with features: test but bdk does not have these features."

It would be cool to make it work like this, but I don't know how.

@notmandatory notmandatory force-pushed the feature/expose_testutils branch from 29f32d8 to fa013ae Compare August 25, 2021 09:21
@notmandatory notmandatory merged commit 976e641 into bitcoindevkit:master Aug 25, 2021
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.

4 participants