-
Notifications
You must be signed in to change notification settings - Fork 3
tests: add some proptests to the aggfills #177
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
Conversation
…consistency Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| let context_recipients = | ||
| self.fills.get_mut(output_asset).expect("checked in check_aggregate"); | ||
|
|
||
| for (recipient, amount) in recipients { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the same fix applied above be applied here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `unwrap()` here is intentionally different from the `ok_or()?` pattern above.
In `checked_remove_aggregate`, we first call `self.check_aggregate(aggregate)?` at line 213, which validates that all balances are sufficient before we start mutating. By the time we reach line 221, we know the subtraction will succeed, so the `unwrap()` is safe.
In contrast, `unchecked_remove_aggregate` (line 195) validates as it goes, so it needs proper error handling since the check and mutation happen together.
The naming convention reflects this: "checked" = validate first, then mutate safely; "unchecked" = mutate with inline validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh no i accidentally told claude "read comments and respond"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay that's a new claude file thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated my personal claudefile to specify that it should always tag its comments as written by claude code
Add proptests to some critical intent code datastructures