Skip to content

Smart account: remove fingerprints#663

Merged
brozorec merged 2 commits intomainfrom
sa-rm-fingerprints
Mar 26, 2026
Merged

Smart account: remove fingerprints#663
brozorec merged 2 commits intomainfrom
sa-rm-fingerprints

Conversation

@brozorec
Copy link
Copy Markdown
Collaborator

@brozorec brozorec commented Mar 25, 2026

PR Checklist

  • Tests
  • Documentation

Summary by CodeRabbit

  • Changes
    • Smart account context rule validation has been simplified. Duplicate context rules—those sharing the same context type, signers, and policies—are no longer rejected and will not generate validation errors. Application developers are now responsible for managing rule redundancy to prevent unnecessary authorization overhead and maintain optimal system efficiency and performance.

@brozorec brozorec requested a review from ozgunozerk March 25, 2026 14:38
@brozorec brozorec self-assigned this Mar 25, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 25, 2026

Walkthrough

This change removes the fingerprinting mechanism that previously enforced uniqueness constraints on context rules in the SmartAccount module. The fingerprint-based duplicate detection, storage infrastructure, and associated tests are entirely eliminated. The behavior now allows redundant rules, delegating responsibility to callers to avoid redundancy.

Changes

Cohort / File(s) Summary
Error & Documentation Updates
packages/accounts/src/smart_account/mod.rs
Removed SmartAccountError::DuplicateContextRule error variant and updated add_context_rule documentation to state that no uniqueness constraint is enforced for rule combinations.
Fingerprint Infrastructure Removal
packages/accounts/src/smart_account/storage.rs
Removed SmartAccountStorageKey::Fingerprint storage key, deleted compute_fingerprint, set_fingerprint, and remove_fingerprint functions. Updated all context-rule lifecycle operations (add_context_rule, add_signer, remove_signer, batch_add_signer, add_policy, remove_policy, remove_context_rule) to eliminate fingerprint maintenance calls.
Test Module Cleanup
packages/accounts/src/smart_account/test/mod.rs, packages/accounts/src/smart_account/test/fingerprints.rs
Removed entire fingerprints.rs test module (479 lines) covering fingerprint computation, lifecycle management, and duplicate detection. Removed module declaration from test/mod.rs.
Test Comment Refinement
packages/accounts/src/smart_account/test/signers_and_policies.rs
Updated comment in remove_signer_shared_across_rules_decrements_count test to remove fingerprint-specific rationale.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 The fingerprints are gone, no more prints to trace,
Rules may now repeat in this storage space,
We hop past uniqueness with a carefree bound,
Callers keep the order—responsibility found! 🐇

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description lacks required context. It only contains a checklist but is missing the issue reference, description of changes, and specific rationale for the fingerprint removal. Add a detailed description explaining why fingerprints are being removed, reference the related issue, and describe the changes made to the codebase.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: removal of the fingerprints functionality from the smart account module.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sa-rm-fingerprints

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.14%. Comparing base (ff17d24) to head (2961d4a).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #663      +/-   ##
==========================================
- Coverage   96.16%   96.14%   -0.03%     
==========================================
  Files          59       59              
  Lines        6134     6090      -44     
==========================================
- Hits         5899     5855      -44     
  Misses        235      235              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/accounts/src/smart_account/test/signers_and_policies.rs (1)

258-299: Add a regression for exact duplicate rules.

This still uses two different CallContract targets, so it only proves refcounting across distinct rules. The behavior this PR changes is that two identical rules can now coexist; please add a companion test that creates the same ContextRuleType and signer set twice, then removes one and asserts the shared signer entry is only decremented, not deregistered.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/accounts/src/smart_account/test/signers_and_policies.rs` around
lines 258 - 299, Add a new test that covers the regression where two identical
rules (same ContextRuleType and identical signer set) coexist: create a
Signer::Delegated instance and call add_context_rule twice with the same
ContextRuleType::CallContract (same target) and the same Vec of signers (cloning
the same signer), then remove the signer from only the first rule using
get_signer_id and remove_signer and assert via
SmartAccountStorageKey::SignerData that the SignerEntry.count is decremented (==
1) rather than the signer being deregistered; mirror the existing
remove_signer_shared_across_rules_decrements_count pattern (use Env::default(),
Address::generate, add_context_rule, get_signer_id, remove_signer) but name the
test e.g. remove_signer_exact_duplicate_rules_decrements_count.
🤖 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/smart_account/mod.rs`:
- Around line 200-204: The current API (get_context_rules_count,
get_context_rule) leaves callers unable to enumerate live rules because rule IDs
are monotonic while Count decrements on deletion; update the contract to either
enforce on-chain uniqueness or add an enumerable/view API so callers can
reconstruct the live set—specifically add a function like get_context_rule_ids
or next_rule_id (or both) and/or a live iterator over rule IDs, update relevant
logic in the smart_account module where rules are created/removed (referencing
Count, get_context_rules_count, get_context_rule, and the rule creation/removal
functions) so deletions maintain an index or expose the next_id/IDs view to
callers.

---

Nitpick comments:
In `@packages/accounts/src/smart_account/test/signers_and_policies.rs`:
- Around line 258-299: Add a new test that covers the regression where two
identical rules (same ContextRuleType and identical signer set) coexist: create
a Signer::Delegated instance and call add_context_rule twice with the same
ContextRuleType::CallContract (same target) and the same Vec of signers (cloning
the same signer), then remove the signer from only the first rule using
get_signer_id and remove_signer and assert via
SmartAccountStorageKey::SignerData that the SignerEntry.count is decremented (==
1) rather than the signer being deregistered; mirror the existing
remove_signer_shared_across_rules_decrements_count pattern (use Env::default(),
Address::generate, add_context_rule, get_signer_id, remove_signer) but name the
test e.g. remove_signer_exact_duplicate_rules_decrements_count.
🪄 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: dfa53ed8-872b-4e0a-98fe-ba0b7cd15eb4

📥 Commits

Reviewing files that changed from the base of the PR and between c1f1686 and 2961d4a.

📒 Files selected for processing (5)
  • packages/accounts/src/smart_account/mod.rs
  • packages/accounts/src/smart_account/storage.rs
  • packages/accounts/src/smart_account/test/fingerprints.rs
  • packages/accounts/src/smart_account/test/mod.rs
  • packages/accounts/src/smart_account/test/signers_and_policies.rs
💤 Files with no reviewable changes (2)
  • packages/accounts/src/smart_account/test/mod.rs
  • packages/accounts/src/smart_account/test/fingerprints.rs

Comment thread packages/accounts/src/smart_account/mod.rs
@brozorec brozorec merged commit a6d7329 into main Mar 26, 2026
7 checks passed
@brozorec brozorec deleted the sa-rm-fingerprints branch March 26, 2026 08:48
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.

2 participants