fix(profile,events): replace saturating_add with checked_add + typed errors#101
fix(profile,events): replace saturating_add with checked_add + typed errors#101Shadow-MMN wants to merge 2 commits into
Conversation
Quota reachedYour plan allows 300 CI/CD file units per month. You've used 297 and this scan would add 7 more (total: 304). |
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughEvent 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. ChangesEvent ID overflow handling
Earnings overflow handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contracts/events/src/tests/op_id_security.rs (1)
213-247: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlso assert rollback of pre-ID state changes.
create_eventdeposits escrow beforenext_event_idcan returnEventIdOverflow. This test checks only the error code; also assert that the stored counter remainsu64::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 winRemove 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
📒 Files selected for processing (7)
contracts/events/src/errors.rscontracts/events/src/event_ops.rscontracts/events/src/idempotency.rscontracts/events/src/tests/op_id_security.rscontracts/profile/src/earnings.rscontracts/profile/src/errors.rscontracts/profile/src/tests/earnings.rs
…arration comments
Closes #72
Problem
Two unbounded accumulators use
saturating_add, which silently clamps on overflow rather than reverting:boundless-profileearnings (earnings.rs:26): per-user per-token earnings silently pin ati128::MAXon overflow, causing irrecoverable accounting drift. Later increments become no-ops.boundless-eventsevent ID counter (idempotency.rs:30-31): the counter freezes atu64::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
unwrapon host-returnedOption/Result— return a typedErrorinstead.Changes
contracts/profile/src/errors.rsEarningsOverflow = 21contracts/profile/src/earnings.rssaturating_add→checked_add+ errorcontracts/profile/src/tests/earnings.rsEarningsOverflowinstead of clamped valuecontracts/events/src/errors.rsEventIdOverflow = 71contracts/events/src/idempotency.rsnext_event_idreturnsResult<u64, Error>, useschecked_addcontracts/events/src/event_ops.rs?from newnext_event_idreturn typecontracts/events/src/tests/op_id_security.rsevent_id_overflow_revertstestTesting
cargo test --release: 287/287 passed (221 events + 66 profile)cargo fmt -- --check: cleanSummary by CodeRabbit
Bug Fixes
Tests