feat(token): native shielded token supply extension#638
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 Compact supply-accounting module family: ChangesNative Shielded Token Supply Accounting
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Optional on-chain supply accounting for the native shielded token, stacked on the core PR. Adds NativeShieldedTokenSupply (scalar) and NativeShieldedTokenFamilySupply (per-domain) over a shared NativeShieldedTokenSupplyCore, plus per-module mocks, simulators, and unit suites. * _addMinted / _addBurned building blocks, paired by the consumer with the matching mint and burn ops; totalMinted / totalBurned / totalSupply getters * _addBurned enforces burned <= minted: a readable error that also makes the add overflow and the totalSupply subtraction underflow unreachable * UINT128_MAX comes from Utils
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Looking good, @0xisk! You mind fixing conflicts? Also, I didn't review the integration files. I assume these are meant to be removed for now since we can't run the integration tests yet
0xisk
force-pushed
the
feat/native-shielded-token-supply-accounting
branch
from
22b84ac to
989538d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contracts/src/token/test/extensions/NativeShieldedTokenFamilySupply.test.ts (1)
63-78: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a mint-overflow test.
This suite covers the
burned <= mintedinvariant well, but doesn't exercise_addMinted's overflow guard (the"NativeShieldedTokenSupply: arithmetic overflow"path in the core). Given supply accounting is a security-sensitive invariant, a small case is worth adding here (or confirming it's covered by the property-based tests for the single-domain flavor).🤖 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/token/test/extensions/NativeShieldedTokenFamilySupply.test.ts` around lines 63 - 78, Add a mint-overflow test to the NativeShieldedTokenFamilySupply test suite to cover the `_addMinted` overflow guard. In `NativeShieldedTokenFamilySupply.test.ts`, extend the `describe('per-domain burned <= minted invariant'...)` area or nearby with a case that calls `_addMinted` on a domain using a value that would exceed the core arithmetic limit and asserts it rejects with `NativeShieldedTokenSupply: arithmetic overflow`. If this path is already covered elsewhere, point to that coverage explicitly and keep this suite focused on the family-specific invariant.contracts/src/token/extensions/NativeShieldedTokenSupply.compact (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider marking
key()aspure.
key()only returns a constant and touches neither ledger nor witness state, so it qualifies for thepuremodifier (consistent with theDEFAULT_ADMIN_ROLE-style pattern used elsewhere in the codebase).♻️ Proposed fix
- circuit key(): Bytes<32> { + pure circuit key(): Bytes<32> { return default<Bytes<32>>; }🤖 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/token/extensions/NativeShieldedTokenSupply.compact` around lines 62 - 64, The key() circuit in NativeShieldedTokenSupply only returns a constant value and does not read ledger or witness state, so update its declaration to use the pure modifier. Make this change on the key() method itself, keeping the existing return behavior intact and matching the pure pattern used for similar constant-returning methods elsewhere in the codebase.Source: Learnings
🤖 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/src/token/extensions/NativeShieldedTokenSupply.compact`:
- Around line 62-64: The key() circuit in NativeShieldedTokenSupply only returns
a constant value and does not read ledger or witness state, so update its
declaration to use the pure modifier. Make this change on the key() method
itself, keeping the existing return behavior intact and matching the pure
pattern used for similar constant-returning methods elsewhere in the codebase.
In `@contracts/src/token/test/extensions/NativeShieldedTokenFamilySupply.test.ts`:
- Around line 63-78: Add a mint-overflow test to the
NativeShieldedTokenFamilySupply test suite to cover the `_addMinted` overflow
guard. In `NativeShieldedTokenFamilySupply.test.ts`, extend the
`describe('per-domain burned <= minted invariant'...)` area or nearby with a
case that calls `_addMinted` on a domain using a value that would exceed the
core arithmetic limit and asserts it rejects with `NativeShieldedTokenSupply:
arithmetic overflow`. If this path is already covered elsewhere, point to that
coverage explicitly and keep this suite focused on the family-specific
invariant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 640100bc-d789-4657-9891-eb33c3fd80b1
📒 Files selected for processing (10)
contracts/src/token/extensions/NativeShieldedTokenFamilySupply.compactcontracts/src/token/extensions/NativeShieldedTokenSupply.compactcontracts/src/token/extensions/NativeShieldedTokenSupplyCore.compactcontracts/src/token/test/extensions/NativeShieldedTokenFamilySupply.test.tscontracts/src/token/test/extensions/NativeShieldedTokenSupply.property.test.tscontracts/src/token/test/extensions/NativeShieldedTokenSupply.test.tscontracts/src/token/test/mocks/MockNativeShieldedTokenFamilySupply.compactcontracts/src/token/test/mocks/MockNativeShieldedTokenSupply.compactcontracts/src/token/test/simulators/NativeShieldedTokenFamilySupplySimulator.tscontracts/src/token/test/simulators/NativeShieldedTokenSupplySimulator.ts
Apply the named import/re-export pattern to the supply extensions so an implementing contract's ledger keys read as `_totalMinted` / `_totalBurned` rather than `NativeShieldedTokenSupplyCore__totalMinted`. This mirrors how `NativeShieldedToken` / `NativeShieldedTokenFamily` re-export the core metadata keys, keeping the on-chain state readable from either flavor. * extensions: re-export `_totalMinted` / `_totalBurned` from `NativeShieldedTokenSupplyCore` in both `NativeShieldedTokenSupply` and `NativeShieldedTokenFamilySupply`; circuits keep the `Core_` prefix. * mocks: surface the same keys unprefixed so tests can read them via `getPublicState()`. * tests: add a simulator-wiring case to each unit suite asserting the getters return the ledger slots verbatim. Addresses review feedback on the supply extension.
`totalSupply` computed `minted - burned`, which reverts on underflow if `burned` ever exceeds `minted`. The `_addBurned` invariant makes that state unreachable through the accounting API, but a getter should read cleanly rather than revert on an out-of-bound state. Clamp to 0 so `totalSupply` is a total function. * SupplyCore: return `minted >= burned ? minted - burned : 0`; the guarded subtraction also nets one row cheaper (rows 440 -> 439, and the scalar wrapper 161 -> 160). * mocks: add a test-only `unsafeSetBurned` that writes `_totalBurned` directly, bypassing the invariant, so tests can reach `burned > minted`. * simulators/tests: expose `unsafeSetBurned` and assert `totalSupply` reads 0 on the corrupted state (scalar and per-domain). Addresses review feedback on the supply extension.
Consolidate the supply property test to cover both flavors in a single file: the scalar `NativeShieldedTokenSupply` and the per-domain `NativeShieldedTokenFamilySupply`. Both delegate to the shared `NativeShieldedTokenSupplyCore`, so the core accounting is fuzzed transitively through each; no standalone core mock is needed. * keep the scalar sequence property (totalMinted exact, totalSupply = minted - burned, burned <= minted) * add a per-domain family property that interleaves ops across three domains and checks the same invariants per domain Stays a unit-level property test (in-memory simulator, no deploy); an on-chain mint/burn-vs-counter scenario is left to the integration suite. Addresses review feedback on the supply extension.
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Improvements look good, @0xisk!
| assert(minted >= current, "NativeShieldedTokenSupply: burned exceeds minted"); | ||
| assert(minted - current >= amount, "NativeShieldedTokenSupply: burned exceeds minted"); |
There was a problem hiding this comment.
Do we need both checks? If so, should the error msgs reflect it? Right now, it's difficult to confirm which is triggered because the msgs are the same
There was a problem hiding this comment.
Yes, both are needed and now carry distinct messages (e00ef37).
minted >= current→"burned total exceeds minted": guards the subtraction below from underflowing if storedburnedis somehow already pastminted. Unreachable through the accounting API itself, so it's the corrupt-state guard.minted - current >= amount→"burn exceeds available supply": the reachable per-call check that this burn doesn't push the domain past its minted total.
Added a comment on the split and a corrupt-state test in each suite (unsafeSetBurned above minted, then _addBurned) so the first message is actually asserted — the existing five reverting cases all hit the second.
| const minted = totalMinted(domain); | ||
| const current = totalBurned(domain); | ||
| assert(minted >= current, "NativeShieldedTokenSupply: burned exceeds minted"); | ||
| assert(minted - current >= amount, "NativeShieldedTokenSupply: burned exceeds minted"); |
There was a problem hiding this comment.
This line in particular is interesting. This works assuming the minted side is correct. A potential concern is if the mint side undercounts, users may not be able to legitimately burn
There's an argument for creating a token + supply module so that there's a guarantee that accounting won't drift and we can safely assume the relationship is sound. This will be a reoccurring question with compositional design and I think both sides have a point. Flexibility + required user correctness || Rigid with more lib maintenance + harder for users to misuse
There was a problem hiding this comment.
If we keep this design as-is, let's document that failure to pair these circuits correctly with mint and burn can have serious consequences
There was a problem hiding this comment.
Kept the compositional design and documented the risk as a module-level @warning in all three supply files (2dcd26e). Core carries the authoritative version; the two flavors point back to it.
It calls the mint/burn ↔ accounting pairing a security-critical invariant the extension can't enforce or detect, and spells out the case you flagged: a mint whose _addMinted is missing/wrong-domain leaves totalMinted too low, so _addBurned then rejects burns that are actually legitimate — holders can't burn coins that genuinely exist. Also covers the overcount (over-burn allowed) and unpaired-burn (totalSupply overstated) directions.
2 participants
Types of changes
Stacked on #621 (core native shielded token). Base this PR's review on the diff against
feat/native-shielded-token-supply, notmain.Optional on-chain supply accounting for the native shielded token:
NativeShieldedTokenSupply(scalar) andNativeShieldedTokenFamily Supply(per-domain) are thin wrappers over a sharedNativeShieldedTokenSupplyCore._addMinted/_addBurnedbuilding blocks the consumer pairs with the matching mint/burn op;totalMinted/totalBurned/totalSupplygetters._addBurnedenforces the per-domainburned <= mintedinvariant: a readable error that also makes the add-overflow and thetotalSupplyunderflow unreachable.UINT128_MAXcomes fromUtils.PR Checklist
Summary by CodeRabbit
New Features
Bug Fixes