Skip to content

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

Merged
afilini merged 1 commit intobitcoindevkit:masterfrom
rajarshimaitra:bug-fix2
Aug 29, 2022
Merged

[bug-fix] Set the db sync height#726
afilini merged 1 commit intobitcoindevkit:masterfrom
rajarshimaitra:bug-fix2

Conversation

@rajarshimaitra
Copy link
Copy Markdown
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
Comment thread src/wallet/mod.rs Outdated
Comment thread src/wallet/mod.rs Outdated
@rajarshimaitra
Copy link
Copy Markdown
Contributor Author

Updated with review comments..

Copy link
Copy Markdown
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

Comment thread src/wallet/mod.rs Outdated
Comment thread src/wallet/mod.rs Outdated
@rajarshimaitra
Copy link
Copy Markdown
Contributor Author

Thanks @vladimirfomene for the catches.. Updated..

Copy link
Copy Markdown
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

Comment thread src/database/memory.rs Outdated
Comment thread src/database/memory.rs
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

Comment thread src/database/memory.rs Outdated
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
Copy Markdown
Contributor Author

Updated and rebased..

Copy link
Copy Markdown
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

None yet

Development

Successfully merging this pull request may close these issues.

[bug] get_funded_wallet() does not return balance

5 participants