Open
Conversation
Contributor
|
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:
WalkthroughThe PR adds contract events for policy lifecycle management (install, uninstall, and configuration changes) across three policy implementations. It removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
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)
📝 Coding Plan for PR comments
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #616 +/- ##
==========================================
+ Coverage 96.37% 96.41% +0.03%
==========================================
Files 58 58
Lines 5960 6021 +61
==========================================
+ Hits 5744 5805 +61
Misses 216 216 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/accounts/src/policies/test/weighted_threshold.rs (1)
64-65: Assert the installed event payload, not just the total count.
len() == 1only proves that some event was emitted in this test path. It will not catch a wrong topic or wrong payload, which is the contract this PR is changing. Snapshot the count beforeinstall, then assert the appended event matchesWeightedPolicyInstalled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/accounts/src/policies/test/weighted_threshold.rs` around lines 64 - 65, Before calling install in the test, capture the current event count from e.events().all().events().len(), then call the install path and assert that exactly one new event was appended; retrieve the newly appended event from e.events().all().events()[old_len] and assert its topic/variant and payload equal the expected WeightedPolicyInstalled (check the event's discriminant and the contained payload fields); update the assertion in weighted_threshold.rs to validate the specific event type and payload rather than only checking len() == 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/accounts/src/policies/test/simple_threshold.rs`:
- Around line 60-61: The assertions currently assume a fresh event list but Env
event storage is cumulative (e.events().all().events()), so save the pre-call
count (e.g., let before = e.events().all().events().len()) before calling
install/uninstall and assert against before + 1 (or appropriate delta), or
alternatively inspect the tail event directly via
e.events().all().events().last().unwrap() and match its type; also update the
enforce_success check (the code referencing enforce_success) to use the same
saved pre-call count logic so it asserts the delta rather than assuming a
single-event-only Env.
In `@packages/accounts/src/policies/test/spending_limit.rs`:
- Around line 80-81: The test is asserting the global event count via
e.events().all().events().len(), which is cumulative across the Env and makes
the assert in uninstall_success brittle; instead record the event count before
the action (let before = e.events().all().events().len()), perform the
uninstall/install action, then assert the delta
(assert_eq!(e.events().all().events().len() - before, 1)) so you verify
per-action events; apply the same change to the other similar assertion(s)
around lines 446-447 that use e.events().all().events().len().
In `@packages/accounts/src/policies/test/weighted_threshold.rs`:
- Around line 446-447: The test asserts a fixed total event count
(assert_eq!(e.events().all().events().len(), 1)) which is brittle because the
policy was installed earlier; instead, record the event count before calling
uninstall (let before = e.events().all().events().len()), perform the uninstall,
then get the new count and assert new_count == before + 1, and finally assert
that the newly appended event (e.events().all().events().last().unwrap()) is the
WeightedPolicyUninstalled variant to verify the correct event was emitted.
In `@packages/accounts/src/policies/weighted_threshold.rs`:
- Around line 531-538: Check whether the policy state exists before removing and
only publish WeightedPolicyUninstalled when an actual stored entry was removed:
look up the storage key constructed with
WeightedThresholdStorageKey::AccountContext(smart_account.clone(),
context_rule.id), call e.storage().persistent().get/remove in a way that detects
existence (e.g., remove returns Option or check get first), and only invoke
WeightedPolicyUninstalled { smart_account: smart_account.clone(),
context_rule_id: context_rule.id }.publish(e) if the stored entry was present;
do not publish when nothing was removed.
---
Nitpick comments:
In `@packages/accounts/src/policies/test/weighted_threshold.rs`:
- Around line 64-65: Before calling install in the test, capture the current
event count from e.events().all().events().len(), then call the install path and
assert that exactly one new event was appended; retrieve the newly appended
event from e.events().all().events()[old_len] and assert its topic/variant and
payload equal the expected WeightedPolicyInstalled (check the event's
discriminant and the contained payload fields); update the assertion in
weighted_threshold.rs to validate the specific event type and payload rather
than only checking len() == 1.
🪄 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: d3ec2a12-5812-4454-8f8a-bc9ecdc3f4bf
📒 Files selected for processing (10)
examples/multisig-smart-account/README.mdpackages/accounts/src/policies/mod.rspackages/accounts/src/policies/simple_threshold.rspackages/accounts/src/policies/spending_limit.rspackages/accounts/src/policies/test/simple_threshold.rspackages/accounts/src/policies/test/spending_limit.rspackages/accounts/src/policies/test/weighted_threshold.rspackages/accounts/src/policies/weighted_threshold.rspackages/accounts/src/smart_account/mod.rspackages/accounts/src/smart_account/storage.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #613
PR Checklist
Summary by CodeRabbit
Documentation
New Features
Improvements
Tests