Skip to content

test for excluded bounds in outputs_in_range#1897

Merged
ValuedMammal merged 1 commit intobitcoindevkit:masterfrom
aagbotemi:test-excluded-bound
Jul 3, 2025
Merged

test for excluded bounds in outputs_in_range#1897
ValuedMammal merged 1 commit intobitcoindevkit:masterfrom
aagbotemi:test-excluded-bound

Conversation

@aagbotemi
Copy link
Copy Markdown
Contributor

@aagbotemi aagbotemi commented Mar 18, 2025

Description

This PR covers a wide range on tests for excluded bounds and the SpkTxOutIndex::outputs_in_range. This PR fixes bitcoindevkit/bdk_wallet#58

Notes to the reviewers

Changelog notice

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

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

@aagbotemi aagbotemi mentioned this pull request Mar 18, 2025
Closed
@ValuedMammal ValuedMammal added the audit Suggested as result of external code audit label Mar 18, 2025
@notmandatory notmandatory moved this to In Progress in BDK Chain Apr 23, 2025
@GideonBature
Copy link
Copy Markdown

GideonBature commented Apr 26, 2025

tACK #1897

Tested and working as it should.

ValuedMammal
ValuedMammal reviewed Jun 9, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK

@aagbotemi If you plan to rebase I left a comment that I think would make the test better.

Comment thread crates/chain/tests/test_spk_txout_index.rs Outdated
@aagbotemi aagbotemi force-pushed the test-excluded-bound branch 4 times, most recently from 9f16a42 to 8a86594 Compare June 10, 2025 00:02
@aagbotemi
Copy link
Copy Markdown
Contributor Author

aagbotemi commented Jun 10, 2025

@aagbotemi If you plan to rebase I left a comment that I think would make the test better.

@ValuedMammal I have rebased and attended to the comment. Thank you.

@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Chain Jun 10, 2025
@ValuedMammal ValuedMammal added this to the Wallet 2.1.0 milestone Jun 10, 2025
@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Jun 10, 2025
@ValuedMammal
Copy link
Copy Markdown
Collaborator

ValuedMammal commented Jun 11, 2025

ACK 8a86594 pending a passing CI run.

@aagbotemi aagbotemi force-pushed the test-excluded-bound branch 2 times, most recently from 83cc556 to d8819c3 Compare June 13, 2025 17:22
@aagbotemi
Copy link
Copy Markdown
Contributor Author

aagbotemi commented Jun 13, 2025

ACK 8a86594 pending a passing CI run.

@ValuedMammal I have rebased with PR #1972, that will fix the CI as discussed

ValuedMammal
ValuedMammal approved these changes Jun 16, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK d8819c3

Comment thread crates/chain/tests/test_spk_txout_index.rs
Comment on lines +294 to +296
let outputs_to_three_excl: Vec<_> = spk_index
.outputs_in_range((Bound::Included(4u32), Bound::Unbounded::<u32>))
.collect();
Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal Jun 16, 2025

Choose a reason for hiding this comment

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

You might have meant to do Bound::Excluded(3) here.

Copy link
Copy Markdown
Contributor Author

@aagbotemi aagbotemi Jun 16, 2025

Choose a reason for hiding this comment

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

Yes, thank you for pointing it out. Using Bound::Excluded(3) provides clarity.

@aagbotemi aagbotemi force-pushed the test-excluded-bound branch from d8819c3 to c39ba0a Compare June 16, 2025 21:46
@notmandatory notmandatory removed this from the Wallet 2.1.0 milestone Jun 17, 2025
notmandatory
notmandatory approved these changes Jun 17, 2025
View reviewed changes
Copy link
Copy Markdown
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 c39ba0a

Just needs a rebase to pick up a CI change and then it looks ready to merge.

@aagbotemi
test for excluded bounds in outputs_in_range
ad79267 
- replace range syntax with explicit Bound types in outputs_in_range tests
@aagbotemi aagbotemi force-pushed the test-excluded-bound branch from c39ba0a to ad79267 Compare June 17, 2025 22:53
@aagbotemi
Copy link
Copy Markdown
Contributor Author

aagbotemi commented Jun 17, 2025

Just needs a rebase to pick up a CI change and then it looks ready to merge.

Thank you for the feedback. I have rebased

@notmandatory notmandatory added this to the Wallet 2.1.0 milestone Jun 26, 2025
oleonardolima
oleonardolima approved these changes Jul 2, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK ad79267

@ValuedMammal
Copy link
Copy Markdown
Collaborator

ValuedMammal commented Jul 3, 2025

ACK ad79267

@ValuedMammal ValuedMammal merged commit 45573ea into bitcoindevkit:master Jul 3, 2025
19 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Chain Jul 3, 2025
@oleonardolima oleonardolima mentioned this pull request Jul 31, 2025
Closed
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@notmandatory notmandatory notmandatory approved these changes

@oleonardolima oleonardolima oleonardolima approved these changes

@ValuedMammal ValuedMammal ValuedMammal approved these changes

Assignees

@aagbotemi aagbotemi

Labels

audit Suggested as result of external code audit tests

Projects

BDK Chain
Archived in project

Milestone

Wallet 2.1.0

Development

Successfully merging this pull request may close these issues.

Test excluded bounds in SpkTxOutIndex::outputs_in_range

5 participants

@aagbotemi @GideonBature @ValuedMammal @notmandatory @oleonardolima