Skip to content

fix(contracts): check virtual offset additions - #609

Merged
collinsezedike merged 3 commits into
drydocs:mainfrom
samlogy1:fix/checked-pricing-additions
Aug 28, 2026
Merged

fix(contracts): check virtual offset additions#609
collinsezedike merged 3 commits into
drydocs:mainfrom
samlogy1:fix/checked-pricing-additions

Conversation

@samlogy1

@samlogy1 samlogy1 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

closes #569

Return ContractError::Overflow when pricing offset additions exceed i128 instead of panicking.

Summary

  • Wraps the two + OFFSET additions in deposit's share-pricing calculation with checked_add, returning ContractError::Overflow on overflow instead of panicking, consistent with the surrounding checked_mul/checked_div chain in the same function.

Test plan

  • pnpm lint && pnpm typecheck && pnpm test pass locally
  • cargo test -p vault: 59 passed, 0 failed

Return ContractError::Overflow when pricing offset additions exceed i128 instead of panicking.

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
@vercel

vercel Bot commented Aug 27, 2026

Copy link
Copy Markdown

@samlogy1 is attempting to deploy a commit to the Collins' projects Team on Vercel.

A member of the Team first needs to authorize it.

@drips-wave

drips-wave Bot commented Aug 27, 2026

Copy link
Copy Markdown

@samlogy1 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@collinsezedike collinsezedike left a comment

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.

@samlogy1 the fix itself is correct, both additions now go through checked_add before the multiply/divide, matching the issue's proposed solution exactly. Two things needed before this can merge:

  • No test covers the overflow path this fix adds. Add one that drives total_shares/total_assets toward the limit where the addition would overflow and asserts ContractError::Overflow is returned. PR #605 in this repo is a good model for how to trigger an overflow in a test.
  • The PR body is still the unfilled template: ## Summary has no bullet, both ## Test plan boxes are unchecked, and there's a dangling Closes # with no number at the end on top of the closes #569 already at the top. Please fill it in per CONTRIBUTING.md's PR body format.

@collinsezedike collinsezedike left a comment

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.

Merging now.

@collinsezedike collinsezedike left a comment

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.

Walking back the approval — CONTRIBUTING.md line 345 requires the PR template be filled out fully before review, and the Test plan checkboxes were still empty. I filled in the Summary section myself since it was just describing the diff, but the Test plan is your confirmation that you ran the commands locally, not something I can fill in for you. Please check those boxes once you've actually run pnpm lint && pnpm typecheck && pnpm test, or update them to reflect what you verified.

The code change itself is still correct and unchanged from my last review.

@collinsezedike collinsezedike left a comment

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.

Merging now.

@collinsezedike
collinsezedike merged commit 30cfe39 into drydocs:main Aug 28, 2026
8 of 9 checks passed
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] Unchecked additions inside the checked pricing expression

2 participants