Skip to content

Commit bd3b6a7

Browse files
committed
refactor(address-set): extract the batch add/remove loops into AddressSetBatchLib, keeping each rule's own zero-address error
1 parent aa7a3a6 commit bd3b6a7

4 files changed

Lines changed: 129 additions & 55 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@ Custom changelog tag: `Dependencies`, `Documentation`, `Testing`
4949

5050
### Changed
5151

52+
- **The batch add/remove loops are shared instead of written three times** (`FEEDBACK_12.md` D-1). New `AddressSetBatchLib` holds the two loops that `RuleAddressSetInternal` and `RuleERC2980Internal` (once for its whitelist, once for its frozenlist) each carried their own copy of. Behaviour is unchanged and **storage layout is identical**, verified per-slot from the compiled artifacts across `RuleWhitelist`, `RuleERC2980`, `RuleBlacklist`, `RuleReceiverWhitelist`, `RuleSpenderWhitelist` and `IdentityRegistryWhitelist`.
53+
- Only the loops moved. Single-address `add` / `remove` / `contains` / `length` stay as one-line delegations to `EnumerableSet`, where a library would add indirection without removing duplication.
54+
- **Each rule keeps its own zero-address error.** `RuleAddressSet_ZeroAddressNotAllowed` and `RuleERC2980_ZeroAddressNotAllowed` are distinct per the one-error-namespace-per-rule convention, which a shared loop cannot name. The guard is therefore passed to `addBatch` as an `internal pure` function pointer: a required parameter, so it cannot be forgotten, while the revert data each rule produces is unchanged.
55+
- Costs ~34 gas per entry on a batch add and ~4 on a batch remove, from the indirect jump — 0.07% of a 20-address batch, which is dominated by cold `SSTORE`s, and an operator path rather than a per-transfer holder path. `RuleERC2980`'s runtime bytecode shrinks 200 bytes; `RuleWhitelist`'s grows 62.
5256
- **`RuleWhitelistWrapper`: the child-rule scan's early exit is now O(1) instead of a full rescan** (`FEEDBACK_12.md` A-2). `_detectTransferRestrictionForTargets` used to re-derive "have all targets been resolved?" by walking the whole `result` array after every child rule; it now maintains a counter of unresolved targets and breaks when it reaches zero. Behaviour is identical — including the documented consequence that a pair already resolved by an earlier child never reaches a later, broken child (`RESULT.md` WW-2). Saves ~85 gas per child scanned (~1% of the ~8.8k per-child cost, which is dominated by the external `STATICCALL`); ~850 gas on a rejected transfer through a 10-child wrapper, paid by the transferring user.
5357
- The resolved-counter form needs a `!result[j]` guard so an address listed in several children is counted once. Without it the counter would reach zero early and break out of the scan before a later child could resolve a *different* target, rejecting a valid transfer. Pinned by `testDetectTransferRestrictionOkWhenAddressListedInSeveralChildRules`, which covers both the 2-target and the `checkSpender` 3-target paths.
5458

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
// SPDX-License-Identifier: MPL-2.0
2+
pragma solidity ^0.8.20;
3+
4+
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
5+
6+
/**
7+
* @title AddressSetBatchLib
8+
* @notice The batch add/remove mechanics shared by every address-list rule in this library.
9+
* @dev Extracted because the same two loops were written three times: once in
10+
* {RuleAddressSetInternal} and twice in `RuleERC2980Internal`, for its whitelist and its frozenlist.
11+
* The copies had already drifted -- only the `RuleAddressSetInternal` one was covered by a test for
12+
* the zero-address rejection (`FEEDBACK_12.md` D-1, F-5).
13+
*
14+
* Only the two *loops* live here. `add` / `remove` / `contains` / `length` on a single address stay
15+
* in the inheriting contracts as one-line delegations to {EnumerableSet}: routing those through a
16+
* library would add a layer of indirection without removing any real duplication.
17+
*
18+
* ## Why the zero-address guard is a function parameter
19+
* Each rule reverts with its OWN custom error (`RuleAddressSet_ZeroAddressNotAllowed`,
20+
* `RuleERC2980_ZeroAddressNotAllowed`), matching the codebase-wide "one error namespace per rule"
21+
* convention. A shared library cannot name those errors, and the alternatives are worse:
22+
*
23+
* - Reverting with a single shared error would change the revert data callers and tests already
24+
* depend on, and break the per-rule error convention.
25+
* - Returning a "a zero was found" flag for the caller to check would make the guard optional in
26+
* practice: a caller that forgot the check would silently list `address(0)`, which is the exact
27+
* outcome the guard exists to prevent.
28+
*
29+
* Passing the guard as an `internal pure` function pointer keeps it MANDATORY -- it is a required
30+
* parameter, so the call does not compile without one -- while each rule keeps its own error. The
31+
* pointer is resolved at compile time and the library is `internal`, so this is a jump inside the
32+
* calling contract, not a `DELEGATECALL`.
33+
*/
34+
library AddressSetBatchLib {
35+
using EnumerableSet for EnumerableSet.AddressSet;
36+
37+
/**
38+
* @notice Adds every address in `addressesToAdd` to `set`, skipping entries already present.
39+
* @dev Duplicates are skipped and counted rather than rejected: an idempotent no-op that the
40+
* caller's batch event still describes truthfully. `address(0)` is NOT skipped -- `guard` is
41+
* invoked for every entry and is expected to revert on it, rejecting the whole batch. Silently
42+
* dropping the sentinel would make the caller's `Add*` event, which echoes the input array,
43+
* report a member that is not in the set.
44+
* @param set The address set to modify.
45+
* @param addressesToAdd The addresses to add.
46+
* @param guard Per-entry validation supplied by the calling rule; reverts with that rule's own
47+
* error. Invoked before the entry is inserted.
48+
* @return added The number of addresses newly inserted.
49+
* @return skipped The number of addresses already present.
50+
*/
51+
function addBatch(
52+
EnumerableSet.AddressSet storage set,
53+
address[] calldata addressesToAdd,
54+
function(address) internal pure guard
55+
) internal returns (uint256 added, uint256 skipped) {
56+
for (uint256 i = 0; i < addressesToAdd.length; ++i) {
57+
guard(addressesToAdd[i]);
58+
if (set.add(addressesToAdd[i])) {
59+
added += 1;
60+
} else {
61+
skipped += 1;
62+
}
63+
}
64+
}
65+
66+
/**
67+
* @notice Removes every address in `addressesToRemove` from `set`, skipping absent entries.
68+
* @dev No guard: removal has no invalid input. Removing an address that is not present is an
69+
* idempotent no-op, counted in `skipped`.
70+
* @param set The address set to modify.
71+
* @param addressesToRemove The addresses to remove.
72+
* @return removed The number of addresses actually removed.
73+
* @return skipped The number of addresses that were not present.
74+
*/
75+
function removeBatch(EnumerableSet.AddressSet storage set, address[] calldata addressesToRemove)
76+
internal
77+
returns (uint256 removed, uint256 skipped)
78+
{
79+
for (uint256 i = 0; i < addressesToRemove.length; ++i) {
80+
if (set.remove(addressesToRemove[i])) {
81+
removed += 1;
82+
} else {
83+
skipped += 1;
84+
}
85+
}
86+
}
87+
}

src/rules/validation/abstract/RuleAddressSet/RuleAddressSetInternal.sol

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ pragma solidity ^0.8.20;
33

44
/* ==== OpenZeppelin === */
55
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
6+
import {AddressSetBatchLib} from "./AddressSetBatchLib.sol";
67
import {RuleAddressSetInvariantStorage} from "./invariantStorage/RuleAddressSetInvariantStorage.sol";
78

89
/**
@@ -15,6 +16,7 @@ import {RuleAddressSetInvariantStorage} from "./invariantStorage/RuleAddressSetI
1516
*/
1617
abstract contract RuleAddressSetInternal is RuleAddressSetInvariantStorage {
1718
using EnumerableSet for EnumerableSet.AddressSet;
19+
using AddressSetBatchLib for EnumerableSet.AddressSet;
1820

1921
/*//////////////////////////////////////////////////////////////
2022
STATE VARIABLES
@@ -44,20 +46,23 @@ abstract contract RuleAddressSetInternal is RuleAddressSetInvariantStorage {
4446
virtual
4547
returns (uint256 added, uint256 skipped)
4648
{
47-
for (uint256 i = 0; i < addressesToAdd.length; ++i) {
48-
// The zero address is the mint/burn sentinel, never a participant. It is REJECTED
49-
// rather than skipped: the batch convention skips *duplicates* (an idempotent no-op that
50-
// the emitted event still describes truthfully), but silently dropping address(0) would
51-
// make `AddAddresses` report a member that is not in the set — re-polluting the very
52-
// off-chain view this guard exists to keep clean. Mint/burn is governed by
53-
// allowMint/allowBurn, never by list membership.
54-
require(addressesToAdd[i] != address(0), RuleAddressSet_ZeroAddressNotAllowed());
55-
if (_listedAddresses.add(addressesToAdd[i])) {
56-
added += 1;
57-
} else {
58-
skipped += 1;
59-
}
60-
}
49+
return _listedAddresses.addBatch(addressesToAdd, _requireNotZeroAddress);
50+
}
51+
52+
/**
53+
* @notice Per-entry guard for {_addAddresses}; reverts on the zero address.
54+
* @dev The zero address is the mint/burn sentinel, never a participant. It is REJECTED rather
55+
* than skipped: the batch convention skips *duplicates* (an idempotent no-op that the emitted
56+
* event still describes truthfully), but silently dropping address(0) would make `AddAddresses`
57+
* report a member that is not in the set — re-polluting the very off-chain view this guard
58+
* exists to keep clean. Mint/burn is governed by allowMint/allowBurn, never by list membership.
59+
*
60+
* Passed to {AddressSetBatchLib.addBatch} as a function pointer so the shared loop can reject
61+
* the sentinel with THIS rule's error rather than a generic one.
62+
* @param targetAddress The candidate address.
63+
*/
64+
function _requireNotZeroAddress(address targetAddress) internal pure {
65+
require(targetAddress != address(0), RuleAddressSet_ZeroAddressNotAllowed());
6166
}
6267

6368
/**
@@ -74,13 +79,7 @@ abstract contract RuleAddressSetInternal is RuleAddressSetInvariantStorage {
7479
virtual
7580
returns (uint256 removed, uint256 skipped)
7681
{
77-
for (uint256 i = 0; i < addressesToRemove.length; ++i) {
78-
if (_listedAddresses.remove(addressesToRemove[i])) {
79-
removed += 1;
80-
} else {
81-
skipped += 1;
82-
}
83-
}
82+
return _listedAddresses.removeBatch(addressesToRemove);
8483
}
8584

8685
/**

src/rules/validation/abstract/RuleERC2980/RuleERC2980Internal.sol

Lines changed: 18 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ pragma solidity ^0.8.20;
33

44
/* ==== OpenZeppelin === */
55
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
6+
import {AddressSetBatchLib} from "../RuleAddressSet/AddressSetBatchLib.sol";
67
import {RuleERC2980InvariantStorage} from "./invariantStorage/RuleERC2980InvariantStorage.sol";
78

89
/**
@@ -16,6 +17,7 @@ import {RuleERC2980InvariantStorage} from "./invariantStorage/RuleERC2980Invaria
1617
*/
1718
abstract contract RuleERC2980Internal is RuleERC2980InvariantStorage {
1819
using EnumerableSet for EnumerableSet.AddressSet;
20+
using AddressSetBatchLib for EnumerableSet.AddressSet;
1921

2022
/*//////////////////////////////////////////////////////////////
2123
STATE VARIABLES
@@ -47,16 +49,7 @@ abstract contract RuleERC2980Internal is RuleERC2980InvariantStorage {
4749
virtual
4850
returns (uint256 added, uint256 skipped)
4951
{
50-
for (uint256 i = 0; i < addressesToAdd.length; ++i) {
51-
// The zero address is the mint/burn sentinel, never a participant. REJECTED rather than
52-
// skipped, so the emitted batch event can never report it as a list member.
53-
require(addressesToAdd[i] != address(0), RuleERC2980_ZeroAddressNotAllowed());
54-
if (_whitelist.add(addressesToAdd[i])) {
55-
added += 1;
56-
} else {
57-
skipped += 1;
58-
}
59-
}
52+
return _whitelist.addBatch(addressesToAdd, _requireNotZeroAddress);
6053
}
6154

6255
/**
@@ -70,13 +63,7 @@ abstract contract RuleERC2980Internal is RuleERC2980InvariantStorage {
7063
virtual
7164
returns (uint256 removed, uint256 skipped)
7265
{
73-
for (uint256 i = 0; i < addressesToRemove.length; ++i) {
74-
if (_whitelist.remove(addressesToRemove[i])) {
75-
removed += 1;
76-
} else {
77-
skipped += 1;
78-
}
79-
}
66+
return _whitelist.removeBatch(addressesToRemove);
8067
}
8168

8269
/**
@@ -111,16 +98,7 @@ abstract contract RuleERC2980Internal is RuleERC2980InvariantStorage {
11198
virtual
11299
returns (uint256 added, uint256 skipped)
113100
{
114-
for (uint256 i = 0; i < addressesToAdd.length; ++i) {
115-
// The zero address is the mint/burn sentinel, never a participant. REJECTED rather than
116-
// skipped, so the emitted batch event can never report it as a list member.
117-
require(addressesToAdd[i] != address(0), RuleERC2980_ZeroAddressNotAllowed());
118-
if (_frozenlist.add(addressesToAdd[i])) {
119-
added += 1;
120-
} else {
121-
skipped += 1;
122-
}
123-
}
101+
return _frozenlist.addBatch(addressesToAdd, _requireNotZeroAddress);
124102
}
125103

126104
/**
@@ -134,13 +112,7 @@ abstract contract RuleERC2980Internal is RuleERC2980InvariantStorage {
134112
virtual
135113
returns (uint256 removed, uint256 skipped)
136114
{
137-
for (uint256 i = 0; i < addressesToRemove.length; ++i) {
138-
if (_frozenlist.remove(addressesToRemove[i])) {
139-
removed += 1;
140-
} else {
141-
skipped += 1;
142-
}
143-
}
115+
return _frozenlist.removeBatch(addressesToRemove);
144116
}
145117

146118
/**
@@ -159,6 +131,18 @@ abstract contract RuleERC2980Internal is RuleERC2980InvariantStorage {
159131
_frozenlist.remove(targetAddress);
160132
}
161133

134+
/**
135+
* @notice Per-entry guard for both batch adders; reverts on the zero address.
136+
* @dev The zero address is the mint/burn sentinel, never a participant. REJECTED rather than
137+
* skipped, so the emitted batch event can never report it as a list member. Passed to
138+
* {AddressSetBatchLib.addBatch} as a function pointer so the shared loop rejects the sentinel
139+
* with THIS rule's error rather than a generic one.
140+
* @param targetAddress The candidate address.
141+
*/
142+
function _requireNotZeroAddress(address targetAddress) internal pure {
143+
require(targetAddress != address(0), RuleERC2980_ZeroAddressNotAllowed());
144+
}
145+
162146
/*//////////////////////////////////////////////////////////////
163147
VIEW — INTERNAL
164148
//////////////////////////////////////////////////////////////*/

0 commit comments

Comments
 (0)