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

[bug-fix] Set the db sync height #726

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

rajarshimaitra
Copy link
Contributor

Description

Fixes #719

Previously we weren't setting the db sync height in populate_test_db
macro even when current height is provided.. This creates a bug that
get_funded_wallet will return 0 balance.

This PR fixes the populate_test_db macro and updates tests which were
previously dependent on the unsynced wallet behavior.

Notes to the reviewers

Checklists

All Submissions:

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

Bugfixes:

  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@rajarshimaitra rajarshimaitra changed the title Set the db sync height [bug-fix] Set the db sync height Aug 17, 2022
src/wallet/mod.rs Outdated Show resolved Hide resolved
src/wallet/mod.rs Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Contributor Author

Updated with review comments..

Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

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

utACK

src/wallet/mod.rs Outdated Show resolved Hide resolved
src/wallet/mod.rs Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Contributor Author

Thanks @vladimirfomene for the catches.. Updated..

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

utACK b8ce842

src/database/memory.rs Outdated Show resolved Hide resolved
src/database/memory.rs Show resolved Hide resolved
@rajarshimaitra rajarshimaitra force-pushed the bug-fix2 branch 2 times, most recently from fdf1f5c to a72a97b Compare August 25, 2022 12:20
@rajarshimaitra
Copy link
Contributor Author

Thanks @afilini for the look.. Updated as suggested.. In the last dev call we talked about releasing this as a patch version for some downstream libs to update.. See bitcoindevkit/bdk-cli#102 (comment)

@danielabrozzoni
Copy link
Member

Sorry I didn't notice the two issues that Alekos has found, I should have inspected the code better.

re-utACK a72a97b

@notmandatory
Copy link
Member

notmandatory commented Aug 25, 2022

@rajarshimaitra even though we talked about doing a fix release for this, since this is a test framework fix and code freeze for 0.22 is next Wed. (Aug 31), can we skip doing a fix release only for this PR and wait for 0.22 to put it out (Sept 7)?

@rajarshimaitra
Copy link
Contributor Author

@notmandatory sounds good to me.. Only needed for the bdk-cli PR.. That can wait a bit..

src/database/memory.rs Outdated Show resolved Hide resolved
Previously we weren't setting the db sync height in populate_test_db
macro even when current height is provided.. This creates a bug that
get_funded_wallet will return 0 balance.

This PR fixes the populate_test_db macro and updates tests which were
previously dependent on the unsynced wallet behavior.
@rajarshimaitra
Copy link
Contributor Author

Updated and rebased..

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.

ACK 08668ac

@afilini afilini merged commit 2bff4e5 into bitcoindevkit:master Aug 29, 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.

[bug] get_funded_wallet() does not return balance
5 participants