Skip to content

fix(profile,events): replace saturating_add with checked_add + typed errors#101

Open
Shadow-MMN wants to merge 2 commits into
boundlessfi:testnetfrom
Shadow-MMN:fix/issue-72-arithmetic-hardening
Open

fix(profile,events): replace saturating_add with checked_add + typed errors#101
Shadow-MMN wants to merge 2 commits into
boundlessfi:testnetfrom
Shadow-MMN:fix/issue-72-arithmetic-hardening

Conversation

@Shadow-MMN

@Shadow-MMN Shadow-MMN commented Jul 23, 2026

Copy link
Copy Markdown

Closes #72

Problem

Two unbounded accumulators use saturating_add, which silently clamps on overflow rather than reverting:

  1. boundless-profile earnings (earnings.rs:26): per-user per-token earnings silently pin at i128::MAX on overflow, causing irrecoverable accounting drift. Later increments become no-ops.

  2. boundless-events event ID counter (idempotency.rs:30-31): the counter freezes at u64::MAX, returning the same ID forever — causing ID collisions and event-state aliasing.

Fix

Use checked arithmetic that reverts with a typed error:

  • current.checked_add(amount).ok_or(Error::EarningsOverflow)?
  • id.checked_add(1).ok_or(Error::EventIdOverflow)?

This aligns with the repo's hard rule: no unwrap on host-returned
Option/Result — return a typed Error instead.

Changes

File Change
contracts/profile/src/errors.rs Added EarningsOverflow = 21
contracts/profile/src/earnings.rs saturating_addchecked_add + error
contracts/profile/src/tests/earnings.rs Updated test to expect EarningsOverflow instead of clamped value
contracts/events/src/errors.rs Added EventIdOverflow = 71
contracts/events/src/idempotency.rs next_event_id returns Result<u64, Error>, uses checked_add
contracts/events/src/event_ops.rs Propagated ? from new next_event_id return type
contracts/events/src/tests/op_id_security.rs Added event_id_overflow_reverts test

Testing

  • cargo test --release: 287/287 passed (221 events + 66 profile)
  • cargo fmt -- --check: clean
  • WASM builds: events 55,676 bytes / profile 15,907 bytes (both under 64 KB ceiling)

Summary by CodeRabbit

  • Bug Fixes

    • Event creation now safely rejects event ID overflow instead of risking duplicate or invalid IDs.
    • Earnings updates now reject arithmetic overflow while preserving the existing balance.
  • Tests

    • Added regression coverage confirming event ID and earnings overflows revert without changing stored values.

@almanax-ai

almanax-ai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Quota reached

Your plan allows 300 CI/CD file units per month. You've used 297 and this scan would add 7 more (total: 304).

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Shadow-MMN, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5df72b58-7c9b-4896-8430-77ee83077169

📥 Commits

Reviewing files that changed from the base of the PR and between 608515d and 32bab1c.

📒 Files selected for processing (2)
  • contracts/events/src/tests/op_id_security.rs
  • contracts/profile/src/tests/earnings.rs
📝 Walkthrough

Walkthrough

Event ID and profile earnings arithmetic now use checked addition with typed overflow errors. Event creation propagates event ID failures, and tests verify both overflow cases revert without invalid state updates.

Changes

Event ID overflow handling

Layer / File(s) Summary
Checked event ID generation
contracts/events/src/errors.rs, contracts/events/src/idempotency.rs, contracts/events/src/event_ops.rs, contracts/events/src/tests/op_id_security.rs
Event ID increments use checked arithmetic, return Error::EventIdOverflow, propagate through event creation, and are covered by a revert test.

Earnings overflow handling

Layer / File(s) Summary
Checked earnings accumulation
contracts/profile/src/errors.rs, contracts/profile/src/earnings.rs, contracts/profile/src/tests/earnings.rs
Earnings accumulation returns Error::EarningsOverflow on overflow, with tests confirming the stored value remains unchanged.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: 0xdevcollins

Poem

I’m a rabbit who checks every sum,
No wrapped-up numbers shall come.
IDs hop with care,
Earnings stay fair,
Overflow? A typed error will come!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title clearly summarizes the main change: replacing saturating_add with checked arithmetic and typed errors in profile/events.
Linked Issues check ✅ Passed Changes match issue #72 by switching both counters to checked_add, adding typed overflow errors, and updating tests.
Out of Scope Changes check ✅ Passed No unrelated code changes are evident; edits stay focused on overflow handling and tests for profile and events.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
contracts/events/src/tests/op_id_security.rs (1)

213-247: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Also assert rollback of pre-ID state changes.

create_event deposits escrow before next_event_id can return EventIdOverflow. This test checks only the error code; also assert that the stored counter remains u64::MAX, no event is persisted, and owner/fee-account balances are unchanged after the failed call.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contracts/events/src/tests/op_id_security.rs` around lines 213 - 247, Extend
event_id_overflow_reverts to verify transaction rollback after the failed
create_event call: capture the owner and fee-account balances and event
count/state before invocation, then assert they are unchanged afterward, the
stored next_event_id remains u64::MAX, and no event was persisted. Reuse the
existing storage and balance/event inspection helpers visible in the test
context.
contracts/profile/src/tests/earnings.rs (1)

193-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove narration-only comments from the test.

The test name and assertions already convey that the second registration must revert and that state remains unchanged. Keep comments only for non-obvious rationale.

As per coding guidelines, comments should explain information the code cannot convey; these comments restate self-evident behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contracts/profile/src/tests/earnings.rs` around lines 193 - 202, Remove the
narration-only comments surrounding the second registration assertion in the
earnings test, including the comments describing overflow/revert and unchanged
state. Keep the test name, setup, and assertions unchanged.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@contracts/events/src/tests/op_id_security.rs`:
- Around line 213-247: Extend event_id_overflow_reverts to verify transaction
rollback after the failed create_event call: capture the owner and fee-account
balances and event count/state before invocation, then assert they are unchanged
afterward, the stored next_event_id remains u64::MAX, and no event was
persisted. Reuse the existing storage and balance/event inspection helpers
visible in the test context.

In `@contracts/profile/src/tests/earnings.rs`:
- Around line 193-202: Remove the narration-only comments surrounding the second
registration assertion in the earnings test, including the comments describing
overflow/revert and unchanged state. Keep the test name, setup, and assertions
unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ba20f7b-366b-4a9c-90d8-fc3ea3ca081d

📥 Commits

Reviewing files that changed from the base of the PR and between 8e15770 and 608515d.

📒 Files selected for processing (7)
  • contracts/events/src/errors.rs
  • contracts/events/src/event_ops.rs
  • contracts/events/src/idempotency.rs
  • contracts/events/src/tests/op_id_security.rs
  • contracts/profile/src/earnings.rs
  • contracts/profile/src/errors.rs
  • contracts/profile/src/tests/earnings.rs

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 23, 2026
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.

Arithmetic hardening: saturating_add on earnings and next_event_id should be checked

1 participant