Add blocklist#626
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a new ChangesBlocklist Security Module
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/src/security/test/Blocklist.test.ts (1)
70-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the “no-op for non-member” test to validate ledger non-mutation
The current assertion on Line 70-73 only checks
isBlocked(BOB) === false, which still passes ifunblockwrites afalseentry. Add agetPublicState().Blocklist__blocked.member(BOB)assertion to codify whether “no-op” means “no new key inserted.”🤖 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/src/security/test/Blocklist.test.ts` around lines 70 - 73, The "is a no-op for a non-member" test for the unblock method in Blocklist.test.ts currently only validates that isBlocked(BOB) returns false, but this alone does not verify that the unblock operation truly did nothing to the ledger state. Add an additional assertion to check the underlying public state directly by verifying that getPublicState().Blocklist__blocked.member(BOB) confirms no new entry was inserted into the blocklist, ensuring the test properly validates that unblock is truly a no-op for non-members.
🤖 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.
Inline comments:
In `@contracts/src/security/Blocklist.compact`:
- Around line 97-99: The _unblock circuit function currently uses
_blocked.insert with a false value instead of actually removing the account from
the blocklist. This causes state growth even when unblocking non-members and
creates a semantic mismatch with the documented behavior. Replace the insert
call with _blocked.remove(disclose(account)) to properly remove the account
entry from the map, ensuring true removal rather than storing a tombstone value.
---
Nitpick comments:
In `@contracts/src/security/test/Blocklist.test.ts`:
- Around line 70-73: The "is a no-op for a non-member" test for the unblock
method in Blocklist.test.ts currently only validates that isBlocked(BOB) returns
false, but this alone does not verify that the unblock operation truly did
nothing to the ledger state. Add an additional assertion to check the underlying
public state directly by verifying that
getPublicState().Blocklist__blocked.member(BOB) confirms no new entry was
inserted into the blocklist, ensuring the test properly validates that unblock
is truly a no-op for non-members.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02772100-7456-4756-bc3b-606cee156b65
📒 Files selected for processing (6)
CHANGELOG.mdcontracts/src/security/Blocklist.compactcontracts/src/security/test/Blocklist.test.tscontracts/src/security/test/mocks/MockBlocklist.compactcontracts/src/security/test/simulators/BlocklistSimulator.tscontracts/src/security/test/witnesses/BlocklistWitnesses.ts
Resolves #608
See #625 for rationale
Summary by CodeRabbit
New Features
Tests