Skip to content

test(utils): add boundary tests for Older::check_older and After::check_after#443

Open
KrishnaShuk wants to merge 1 commit into
bitcoindevkit:masterfrom
KrishnaShuk:test/utils-check-older-boundary
Open

test(utils): add boundary tests for Older::check_older and After::check_after#443
KrishnaShuk wants to merge 1 commit into
bitcoindevkit:masterfrom
KrishnaShuk:test/utils-check-older-boundary

Conversation

@KrishnaShuk
Copy link
Copy Markdown

Description

Addresses the // TODO: test >= / > on line 111 of utils.rs. Adds 11 boundary-condition tests for the Satisfier impls of Older and After.

Notes to the reviewers

Uses Satisfier::<PublicKey>::check_older(...) fully-qualified syntax because the trait is generic over Pk.

Changelog notice

Checklists

All Submissions:

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

Comment thread src/wallet/utils.rs
let after = After::new(Some(100_000), false);
assert!(Satisfier::<PublicKey>::check_after(
&after,
absolute::LockTime::from_consensus(100_000)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

absolute::LockTime can also represent Unix timestamps (>= 500_000_000), but After only has current_height and no current_time. So a timestamp-based locktime would end up being compared against a block height. I also noticed the existing tests don't cover this. Is this intentionally limited to height-based locktimes for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants