Skip to content

Fix unshieldedtreasury deposits, add tests, improve doc - #762

Merged
andrew-fleming merged 4 commits into
OpenZeppelin:mainfrom
andrew-fleming:fix-unshielded-lte
Aug 1, 2026
Merged

Fix unshieldedtreasury deposits, add tests, improve doc#762
andrew-fleming merged 4 commits into
OpenZeppelin:mainfrom
andrew-fleming:fix-unshielded-lte

Conversation

@andrew-fleming

@andrew-fleming andrew-fleming commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Fix: UnshieldedTreasury._deposit reverted on every call. Its overflow guard passed UINT128_MAX - amount (≈2¹²⁸) to unshieldedBalanceLte, which fails at runtime with failed to decode for built-in type u64 despite the built-in's declared Uint<128> parameter. Both guards now check the module's own _balances with in-circuit arithmetic instead of the protocol balance

That also fixes two latent bugs: the protocol balance is a start-of-execution snapshot, so it couldn't see a credit or debit made earlier in the same transaction. It rejected a legitimate receive-then-spend, and let an intra-transaction overspend through to fail as an opaque range fault instead of the named assert

Add: test and sim (dry and live). Covers protocol claims in the module's docs, including a regression guard that pins the unshieldedBalanceLte operand limit so the fix can't be quietly reverted

Docs: warnings for unspendable recipients (accepted, funds provably removed, not a sanctioned burn) and self-sends (zeroes the tracked balance while funds stay spendable), plus a note that zero-amount ops are value-preserving no-ops

Summary by CodeRabbit

  • Documentation

    • Clarified treasury balance accounting, transaction behavior, and deposit/send rules.
    • Documented zero-value operations, overflow and insufficiency checks, recipient considerations, and self-send behavior.
  • Tests

    • Added comprehensive coverage for deposits, sends, multi-call transactions, zero amounts, overflow handling, raw transfers, balance synchronization, and edge cases.
    • Added simulation support for validating treasury operations and balance behavior.

@andrew-fleming
andrew-fleming requested review from a team as code owners August 1, 2026 03:19
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0a9df07e-bc6b-4eb9-ba8a-09d17e90fa00

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Walkthrough

The treasury now uses _balances for overflow and sufficiency checks. New mock circuits and simulator methods expose wrapper and raw ledger behavior. Tests cover deposits, sends, multi-call transactions, zero amounts, backend divergence, and edge cases.

Changes

Unshielded Treasury behavior

Layer / File(s) Summary
Tracked balance accounting
contracts/src/multisig/UnshieldedTreasury.compact
Documentation defines _balances as the accounting source. _deposit and _send now validate against tracked balances.
Mock circuits and simulator wiring
contracts/src/multisig/test/mocks/MockUnshieldedTreasury.compact, contracts/src/multisig/test/simulators/UnshieldedTreasurySimulator.ts
Added combined operations, raw ledger operations, balance probes, self-send support, and simulator wrappers.
Deposit, send, and backend validation
contracts/src/multisig/test/UnshieldedTreasury.test.ts
Added coverage for normal operations, zero amounts, multi-call transactions, backend divergence, overflow comparisons, zero recipients, and self-sends.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested labels: enhancement

Suggested reviewers: 0xisk, pepebndc

Poem

A rabbit checks the ledger bright,
Deposits hop, then sends take flight.
Zero coins leave entries clear,
Raw paths and self-sends persevere.
Tests inspect each careful trace.
Treasury balances stay in place.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the deposit fix and the added tests and documentation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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/multisig/test/mocks/MockUnshieldedTreasury.compact`:
- Around line 99-110: The self-send mock is correct; update the related warning
and live test instead. In contracts/src/multisig/UnshieldedTreasury.compact
lines 94-98, remove the contradictory self-send warning without changing
MockUnshieldedTreasury.compact lines 99-110. In
contracts/src/multisig/test/UnshieldedTreasury.test.ts lines 274-290, make the
live-only test assert sent.kind === 'rejected' and remove post-send funds
probing.
🪄 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: c36a4350-3340-4224-868b-8bc27d876694

📥 Commits

Reviewing files that changed from the base of the PR and between c7f9ffc and a43bf11.

📒 Files selected for processing (4)
  • contracts/src/multisig/UnshieldedTreasury.compact
  • contracts/src/multisig/test/UnshieldedTreasury.test.ts
  • contracts/src/multisig/test/mocks/MockUnshieldedTreasury.compact
  • contracts/src/multisig/test/simulators/UnshieldedTreasurySimulator.ts

Comment thread contracts/src/multisig/test/mocks/MockUnshieldedTreasury.compact
@andrew-fleming
andrew-fleming merged commit 0e9d659 into OpenZeppelin:main Aug 1, 2026
7 of 8 checks passed
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