Skip to content

fix(core): correct bound_into_included_u64 handling for excluded start bounds - #3589

Open
yappermoar-boop wants to merge 1 commit into
0xMiden:nextfrom
yappermoar-boop:fix-bound-into-included-u64-excluded-start
Open

fix(core): correct bound_into_included_u64 handling for excluded start bounds#3589
yappermoar-boop wants to merge 1 commit into
0xMiden:nextfrom
yappermoar-boop:fix-bound-into-included-u64-excluded-start

Conversation

@yappermoar-boop

Copy link
Copy Markdown

Updated bound_into_included_u64 in core/src/utils/mod.rs to correctly perform saturating_add(1) when handling an Excluded start bound (is_start == true). Previously, saturating_sub(1) was executed regardless of is_start, producing incorrect lower bounds. Added unit tests to verify behavior for all Bound variants.

@github-actions

Copy link
Copy Markdown

Automated check (CONTRIBUTING.md)

Findings:

Recommendations:

  • Consider adding a Test plan or clear review steps.

Next steps:

  • Link a relevant issue (e.g., "Fixes Implement SHA256 in Miden Assembly #123") and ensure it is assigned to you.
  • See CONTRIBUTING.md for expectations.
  • If this is a false positive, comment: /quality-review.

Comment thread core/src/utils/mod.rs
}

#[test]
fn test_bound_into_included_u64() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

At u64::MAX, the new addition saturates back to u64::MAX, so an excluded start becomes a range containing that value instead of an empty range. Excluding 0 as an end has the same problem. Could this return Option or Result to represent these empty bounds and cover both endpoints here?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants