Repository navigation
refactor: order B20 checks pause → role → input - #101
Merged
stevieraykatz merged 5 commits intoMay 30, 2026
Merged
stevieraykatz merged 5 commits into
stevieraykatz merged 5 commits into
Conversation
Hoist cross-cutting gates from shared internal helpers to external
entrypoints as modifiers, so every mutating op evaluates its
preconditions in the canonical order:
pause → role → input → allowance → policy → invariants → effects
Modifier list order equals evaluation order (Solidity runs modifiers
left-to-right, body at `_`), so each function's modifier set reads
top-to-bottom as its revert precedence.
MockB20
- New modifiers: `whenNotPaused` (bypass-aware), `validReceiver`,
`validSender`, `validApprover`, `validSpender`, `nonEmptyFeatures`.
- Internal helpers reduced to pure mechanics: `_transfer`
(policy + balance + effects), `_mint` (policy + supply-cap + effects),
`_burnRaw` (balance + effects). `_burnSelf` folded away.
MockB20Security
- New `whenNotPausedStrict` modifier mirrors `onlyRoleStrict`: no
factory-bootstrap bypass, used by the holder-initiated `redeem` /
`redeemWithMemo` path.
- `batchMint` declares `whenNotPaused(MINT) + onlyRole(MINT_ROLE)`
itself (was inherited per-element via `_mint`); per-element
`validReceiver` inlined in the loop since modifier params evaluate
once at entry.
- `batchBurn` swaps inline pause check for the `whenNotPaused(BURN)`
modifier; role-strict semantics preserved.
- `_redeemBurn` body strips its inline pause check.
Interface natspec
- `IB20` / `IB20Security` revert-order lists rewritten as explicit
numbered lists in the new canonical order for `transfer`,
`transferFrom`, `approve`, `mint`, `burn`, `burnBlocked`, `pause`,
`unpause`, `redeem`, `batchMint`, `batchBurn`.
Tests
- Revert-order tests under
`test/unit/B20/{erc20,supply}/*_revertOrder.t.sol` and
`test/unit/B20Security/batch/*_revertOrder.t.sol` updated to assert
the new precedence (canonical-order docstrings rewritten; pair
tests renamed and flipped where precedence reversed).
- Two happy-path tests in
`test/unit/B20Security/batch/batchMint.t.sol` now grant MINT_ROLE
before triggering EmptyBatch / LengthMismatch since the role gate
now fires before body checks.
- forge build clean. forge test: 592 / 592 passing.
Out of band: the production Rust precompile (separate repo) must be
reordered identically and mock ↔ Rust parity verified on
multi-failure inputs, especially `transferFrom`. Coordinate with the
Rust owner.
Refs: BOP-215
Per review feedback: `validReceiver` / `validSender` / `validApprover` / `validSpender` / `nonEmptyFeatures` were misleading (the same address also has policy checks fired against it elsewhere, so a name like `validReceiver` implies completeness it doesn't have). Removed. Kept: `whenNotPaused` and `whenNotPausedStrict` (the pause gate is the only new modifier this PR introduces). Inlined zero-address / empty-feature checks at each entrypoint; revert order is unchanged. Tightened docstrings in mocks + tests to drop references to the removed modifiers.
Replaces the inline `InvalidReceiver` + `InvalidSender` checks in `transfer` / `transferFrom` / `transferWithMemo` / `transferFromWithMemo` with a single shared helper. Order preserved (receiver first, then sender). Mint paths keep the inline `InvalidReceiver` check — only one line per site.
Pause now always fires, even from the factory during the bootstrap window. Unlike role + policy bypass (which have real bootstrap motivations — factory needs to act before having roles, factory needs to seed initial supply before policies are configured), pause has no equivalent need: pause defaults to 'nothing paused' at creation, and any pause state during bootstrap is explicitly opted into by the operator's initCalls. Start-paused configurations must sequence the `pause(...)` call last. - `whenNotPaused` modifier drops its `_isPrivileged()` wrapper. - `whenNotPausedStrict` deleted (now redundant — `whenNotPaused` is always strict). - `redeem` / `redeemWithMemo` switched from `whenNotPausedStrict(REDEEM)` to `whenNotPaused(REDEEM)`. - Contract-level natspec updated to call out the pause-no-bypass rule. All 592 tests still pass — no test relied on the factory bypass for pause, which is strong evidence the bypass was unintentional / unused. Rust precompile gets the matching change via BOP-217.
amiecorso
force-pushed
the
amiecorso/bop-215-b20-mocks-reorder-cross-cutting-checks
branch
from
May 29, 2026 21:40
24b16c8 to
0741474
Compare
amiecorso
changed the title
|
stevieraykatz
deleted the
amiecorso/bop-215-b20-mocks-reorder-cross-cutting-checks
branch
May 30, 2026 00:43
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
2 participants
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.
Refactor
MockB20/MockB20Securityso every mutating entrypointevaluates cross-cutting gates in the canonical order
pause → role → input → allowance → policy → invariants → effects,expressed as modifiers on the external surface. Helpers (
_transfer,_mint,_burnRaw) reduce to pure mechanics;_burnSelfis foldedaway.
Aligns mock ↔ Rust precompile parity for which error surfaces when
multiple preconditions fail at once (notably
transferFrom, whereallowance + executor-policy used to fire before pause + input).
Linear
BOP-215
What changed
MockB20.sol— one new modifier:whenNotPaused(feature)— always-strict pause check (no factorybypass, see "Bonus" below)
Entrypoints declare
whenNotPaused+onlyRolemodifiers incanonical order; zero-address / empty-feature checks are inlined at
each entrypoint (or via the
_requireNonZeroActors(from, to)helperfor the four transfer-family entrypoints, which share both checks).
Internal helpers are pure mechanics:
_transfer→ policy + balance + effects_mint→ policy + supply cap + effects_burnRaw→ balance + effects_burnSelfremoved (folded intoburn/burnWithMemodirectcalls to
_burnRaw)MockB20Security.sol—batchMintadds entrypointwhenNotPaused(MINT) onlyRole(MINT_ROLE)modifiers (previouslyinherited from per-element
_mint) and inlines per-element receivercheck in the loop.
batchBurnaddswhenNotPaused(BURN)to itsmodifier set.
redeem/redeemWithMemocarrywhenNotPaused(REDEEM)on the entrypoint;
_redeemBurnbody strips its inline pause check.Interface natspec —
IB20.solandIB20Security.solrewrite therevert-order lists for
transfer,transferFrom,approve,mint,burn,burnBlocked,pause,unpause,redeem,batchMint,batchBurnas explicit numbered lists matching the new order.Bonus: pause bypass removal
The pre-refactor
_transfer/_mint/_burnSelfwrapped theirpause checks in
if (!_isPrivileged()) { ... }— i.e. pause honoredthe factory bootstrap bypass alongside role and policy. After
discussion, this PR removes that bypass: pause now always fires,
even from the factory during the bootstrap window.
Rationale: role and policy bypass have real bootstrap motivations
(the factory needs to act before having granted itself any roles; the
factory needs to seed initial supply before compliance policies are
configured). Pause has no equivalent motivation — pause defaults to
"nothing paused" at creation, and any pause state during bootstrap is
explicitly opted into by the operator's
initCalls. There's nocoherent flow where the factory pauses a feature in initCall N and
then needs to use it in initCall N+1; start-paused configurations
should sequence the
pause(...)call last. The same "no init-timeuse case → bypass widens attack surface without buying anything"
rationale already documented for
redeem/batchBurnapplies topause generally.
All 592 tests still pass after the change — no test exercised the
factory-bootstrap-with-pause-bypass path, which is strong evidence
the bypass was unintentional / unused in practice.
Implications:
whenNotPausedis now always strict;whenNotPausedStrict(whichthis PR briefly introduced for the
redeemfamily) is deleted asredundant.
BOP-217.
Tests
All 592 unit tests pass. The 6
*_revertOrder.t.solfiles areupdated for the new precedence (renamed pairs, flipped assertions,
canonical-order docstrings). Two happy-path tests in
batchMint.t.solnow grant
MINT_ROLEto the caller before triggeringLengthMismatch/EmptyBatch(the role check now fires first viathe entrypoint modifier).
forge buildclean.forge fmt --checkclean on all files this PRtouches. (One pre-existing fmt drift in
test/lib/B20FactoryLibTest.solexists on
main— unrelated and not touched here.)Out of band — Rust precompile parity
The production Rust precompile (separate repo) must be reordered
identically and parity verified on multi-failure inputs, especially
transferFrom. The pause-bypass removal also needs to land Rust-side.Both tracked under BOP-217.