fix(profile,events): replace saturating_add with checked_add + typed errors - #101
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). |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Disabled knowledge base sources:
📝 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
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
…-cap comments - rustfmt (rust 1.93) wants the assert_eq! args in event_id_overflow_reverts split one-per-line; apply it (CI rustfmt was failing on this). - The errors enum is at 48/50 after boundlessfi#99 removed the three deadline variants, so the three 'enum is at the 50-case cap / consolidate before adding' comments were stale. Reword them: state the real reuse rationale, and the true current count.
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