feat(BOP-440): add transferFromBlockedWithMemo and burnBlockedWithMemo seize surface - #177
Open
stephancill wants to merge 8 commits into
Open
feat(BOP-440): add transferFromBlockedWithMemo and burnBlockedWithMemo seize surface#177stephancill wants to merge 8 commits into
stephancill wants to merge 8 commits into
Conversation
…o seize surface Adds the additive seize operation class to the base-std reference and ABI (source of truth for fork tests), per the BOP-439 PPS: - transferFromBlockedWithMemo(from, to, amount, memo): admin seize that reassigns a blocked account's balance in one call. Skips allowance and the sender/receiver/executor transfer policies; the only membership check is that `from` is blocked under the new SEIZABLE_POLICY. `to` is not policy-checked (admin op). Emits Transfer + TransferredFromBlocked + Memo. - burnBlockedWithMemo(from, amount, memo): memo variant of burn-based seizure, gated on SEIZABLE_POLICY and the new SEIZE pause vector. - New SEIZABLE_POLICY scope (packed into the transfer slot's reserved lane 3, no storage-offset disruption), TRANSFER_FROM_BLOCKED_ROLE, and SEIZE PausableFeature. Reference impl uses an explicit guard sequence and a shared _moveBalance primitive; it does NOT reuse the factory-bootstrap privileged path. Out of scope: repointing the legacy burnBlocked to SEIZABLE_POLICY (BOP-471). Tests: 25 new (happy + revert-order for both functions); layout-pin, slot codec, and removals regression tests updated for the new lane/feature. Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
stephancill
requested review from
amiecorso,
eric-ships,
ilikesymmetry,
rayyan224 and
stevieraykatz
as code owners
July 27, 2026 15:49
Interface Coverage✅ All interface functions have test coverage. |
📊 Forge Coverage (
|
| File | Lines | Stmts | Branches | Funcs |
|---|---|---|---|---|
| 🟢 B20FactoryLib.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🔴 test/lib/ForceFeeder.sol | 0.00% | 0.00% | 100.00% | 0.00% |
| 🔴 test/lib/PrecompileProbe.sol | 0.00% | 0.00% | 0.00% | 0.00% |
| 🟢 MockActivationRegistry.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockActivationRegistryStorage.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockB20.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockB20Asset.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟡 MockB20Factory.sol | 98.96% | 99.10% | 100.00% | 100.00% |
| 🟢 MockB20Stablecoin.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockB20Storage.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockPolicyRegistry.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockPolicyRegistryStorage.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| Total | 96.98% | 97.53% | 98.57% | 97.12% |
Full report: download artifact. To browse locally: make coverage (runs forge coverage + genhtml + opens the HTML report).
stephancill
removed request for
amiecorso,
eric-ships,
ilikesymmetry,
rayyan224 and
stevieraykatz
July 27, 2026 15:50
stephancill
commented
Jul 27, 2026
Comment on lines
-58
to
+60
| /// bits 192..255 : reserved (implicit, no field declared) | ||
| /// bits 192..255 : seizable (`SEIZABLE_POLICY`) |
Author
There was a problem hiding this comment.
does adding seizable here use up the reserved bits? no more reserved bits after adding it?
Author
There was a problem hiding this comment.
AI: Yes — this fills the slot. TransferPolicyIds is one 256-bit slot = 4 × uint64 lanes; sender/receiver/executor used lanes 0–2, and seizable takes lane 3 (bits 192..255), which was the last reserved lane. No reserved bits remain in this slot. A future transfer-side policy would need a new appended slot. I picked this lane specifically because it avoids shifting any existing field offset on the already-shipped layout.
Member
There was a problem hiding this comment.
These bits were reserved for exactly this kind of purpose. Good to flag that we won't have any headroom after this lands, but it's correct to consume them like this.
Collaborator
There was a problem hiding this comment.
@rayyan224 another candidate for automated agent fuzzing would be any changes to storage layout (including appending)
Collaborator
There was a problem hiding this comment.
I'm a bit concerned to take the last space here. Main thing on my mind is if we have some other idea that definitely needs to be on the hotpath transfer. The seizing operations are cold-path and we won't be using any of the other policies here in its execution, so I'm not seeing as clear of a benefit tbh. I'd opt to make a new slot for seize policy entirely by default.
We also recognize seize as an operation applies to burning too. Placing this policy in the "transfer policies" group is not consistent with our separation of "mint policies" as a distinct group by operation.
stephancill
requested review from
amiecorso,
ilikesymmetry,
rayyan224 and
stevieraykatz
July 27, 2026 15:57
stephancill
marked this pull request as draft
July 27, 2026 15:57
|
…TURES_PAUSED Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
stephancill
removed request for
amiecorso,
ilikesymmetry,
rayyan224 and
stevieraykatz
July 27, 2026 16:01
stephancill
commented
Jul 27, 2026
Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
stephancill
marked this pull request as ready for review
July 27, 2026 16:26
stephancill
requested review from
amiecorso,
ilikesymmetry,
rayyan224,
robriks and
stevieraykatz
July 27, 2026 16:56
…emo natspec Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
| MINT, | ||
| BURN | ||
| BURN, | ||
| SEIZE |
Member
There was a problem hiding this comment.
Why do we need to be able to pause seizing? Seems like the kind of admin action you don't want to stop.
Author
There was a problem hiding this comment.
the intention here was that you should be able to pause the other functions of the contract without pausing seizing. one option for implementing that is to not make seizing pausable at all, the other one is this one where seizing becomes its own pause vector
Member
There was a problem hiding this comment.
given transferFromBlockedWithMemo ignores our other safeguards i think it could make sense that we would also ignore pause state.
I would prefer us to know that we need to be able to pause seizing action rather than implement something to avoid its use.
Author
There was a problem hiding this comment.
I think that makes sense
@ilikesymmetry mind weighing in here?
Collaborator
There was a problem hiding this comment.
@stevieraykatz reference pause options and tradeoffs here
We landed on a new pause option to separate concerns. Including an option to pause seizing operations helps asset issuers reduce risk of a buggy/compromised role-holding account by having a separate system able to killswitch the operation.
Comment on lines
+109
to
+110
| /// `burnBlocked` (authorized under `TRANSFER_SENDER_POLICY`), or `transferFromBlockedWithMemo` / | ||
| /// `burnBlockedWithMemo` (authorized under `SEIZABLE_POLICY`). |
Member
There was a problem hiding this comment.
It feels wrong to me that these don't share a policy. I know its because we've got BC to worry about, but it still smells. Is this discussed elsewhere?
Author
There was a problem hiding this comment.
i think this is actually a mistake - we are willing to break BC for burnBlocked under the assumption that it isn't being used yet. intention is to migrate it to the same policy
Author
There was a problem hiding this comment.
ah i see what happened - that breaking change diff was slated for a separate PR. gonna fold it into this one
stephancill
commented
Jul 27, 2026
| bytes32 internal constant TRANSFER_RECEIVER_POLICY = keccak256("TRANSFER_RECEIVER_POLICY"); | ||
| bytes32 internal constant TRANSFER_EXECUTOR_POLICY = keccak256("TRANSFER_EXECUTOR_POLICY"); | ||
| bytes32 internal constant MINT_RECEIVER_POLICY = keccak256("MINT_RECEIVER_POLICY"); | ||
| bytes32 internal constant SEIZABLE_POLICY = keccak256("SEIZABLE_POLICY"); |
Author
There was a problem hiding this comment.
@stevieraykatz thoughts on the naming here? an alternative that was floated was SEIZE_BLOCKED_POLICY
Member
There was a problem hiding this comment.
Thanks for drawing my attention. The verbiage is strange but seizable is actually accurately following convention. We want to describe what the policy does by describing who the policy applies to. So since we're talking about the holders who can be seized from, SEIZABLE is fine.
I'm wondering if there's a way to incorporate the holder though? SEIZEABLE_HOLDER_POLICY doesn't feel right but I also don't like that we don't name the account.
Author
There was a problem hiding this comment.
SEIZEABLE_HOLDER_POLICY could work. Another option could be SEIZEABLE_ACCOUNT_POLICY?
Member
There was a problem hiding this comment.
Are we intending for this policy to be an allowlist or a blocklist? I think that might change how we frame the policy name.
Collaborator
There was a problem hiding this comment.
Are we intending for this policy to be an allowlist or a blocklist? I think that might change how we frame the policy name.
It's most likely going to be a blocklist model, but theoretically we can't guarantee how people will use the scope
Collaborator
There was a problem hiding this comment.
I'm wondering if there's a way to incorporate the holder though?
SEIZEABLE_HOLDER_POLICYdoesn't feel right but I also don't like that we don't name the account.
This was the intent of "SEIZE_BLOCKED" where we describe the entity as being seized from as a "blocked" address. It's a weird word to use as a noun, but it does feel consistent with our naming conventions "transfer from a blocked account" -> "blocked" is the noun
Author
There was a problem hiding this comment.
It's most likely going to be a blocklist model, but theoretically we can't guarantee how people will use the scope
This is my main hesitation against SEIZE_BLOCKED_POLICY. Adding an address to the seizing policy doesn't enforce any blocking logic
Author
There was a problem hiding this comment.
This is also making me second guess the transferFromBlocked function naming. seize or transferFromSeizable is perhaps more accurate
Member
There was a problem hiding this comment.
+1 to transferFromSeizable and then naming the policy according to whether the account can be seized from, i.e. SEIZABLE_ACCOUNT_POLICY
Folds the legacy burnBlocked into the seize operation class: its blocked check now uses SEIZABLE_POLICY (was TRANSFER_SENDER_POLICY) and it gates on the SEIZE pause vector (was BURN), matching burnBlockedWithMemo and transferFromBlockedWithMemo. Breaking change, acceptable because burnBlocked is not yet in use. Simplifies the AccountNotBlocked error to a single seizable-policy statement. Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
| /// (mirroring the `burnBlocked` "blocked" semantics). An unset slot reads as `0` (always-allow), | ||
| /// which makes no account seizable until an issuer configures the slot. | ||
| /// @return Policy scope constant. | ||
| function SEIZABLE_POLICY() external view returns (bytes32); |
Collaborator
There was a problem hiding this comment.
I don't remember aligning on this as the name and recall we had SEIZE_BLOCKED_POLICY. What's the thought process of this new name?
Author
There was a problem hiding this comment.
discussed some options with @rayyan224 async and felt SEIZABLE_POLICY was maybe more descriptive so proceeded with it as a placeholder with the intention of discussing here
Collaborator
There was a problem hiding this comment.
I think we can do as discussed in the P/PS ? Seems' like it's related to this duscusion here
#177 (comment)
Comment on lines
+456
to
+457
| /// Emits `Transfer(from, to, amount)`, `TransferredFromBlocked(caller, from, to, amount)`, and | ||
| /// `Memo(caller, memo)`. A memo of `bytes32(0)` is permitted. |
Collaborator
There was a problem hiding this comment.
for clarity, should enumerate events in order of emission, so Memo would immediately follow Transfer
Author
There was a problem hiding this comment.
AI: Fixed in f0660e2 — natspec now lists emission order Transfer, Memo, TransferredFromBlocked, and the impl emits Memo immediately after Transfer (before TransferredFromBlocked) to satisfy the IB20 Memo invariant.
| /// @param amount Amount to burn. | ||
| function burnBlocked(address from, uint256 amount) external; | ||
|
|
||
| /// @notice Same as `burnBlocked`, plus emits `Memo` after the standard `Transfer` and `BurnedBlocked` events. |
Collaborator
There was a problem hiding this comment.
for clarity, we emit Memo immediately after Transfer and before BurnBlocked
Author
There was a problem hiding this comment.
AI: Fixed in f0660e2 — burnBlockedWithMemo now emits Transfer -> Memo -> BurnedBlocked, and the natspec reflects that order.
Comment on lines
+331
to
+332
| emit BurnedBlocked(msg.sender, from, amount); | ||
| emit Memo(msg.sender, memo); |
Collaborator
There was a problem hiding this comment.
Memo always needs to follow Transfer per spec. @rayyan224 can we get some automated agent workflow going that consistently looks for accidental divergence from previously defined invariants? This one should be caught before it hits human review
Author
There was a problem hiding this comment.
AI: Fixed in f0660e2 — reordered to Transfer -> Memo -> BurnedBlocked so Memo immediately follows Transfer per the spec invariant.
Collaborator
There was a problem hiding this comment.
Yes, agreed @stevieraykatz we had some encoded smart contract skills, that you were working on right ?
Member
There was a problem hiding this comment.
Our solidity harness is the shared repo for agent rules. If this skill/rule isn't defined there, it should be added!
Comment on lines
+510
to
512
| if (policyScope == SEIZABLE_POLICY) return $.transferPolicyIds.seizable; | ||
| if (policyScope == MINT_RECEIVER_POLICY) return $.mintPolicyIds.receiver; | ||
| revert UnsupportedPolicyType(policyScope); |
Collaborator
There was a problem hiding this comment.
I'm concerned that our UnsupportedPolicyType revert is overly restrictive. Thinking through the rollout of these new changes, compliant assets live in production will be temporarily stripped of their ability to block users from holding their assets. To re-enable blocking, they'll need to manually queue up a transaction to set the seize policy to whatever blocklist they were previously managing to keep going. Setting ourselves up for a race condition of sequencing Cobalt activation -> update seize policy -> continue blocking+seizing feels like an unnecessary friction.
Proposal: Consider removing restriction of defined policy types and treat policies more like roles (open-ended with B20-aware values). If we had this, we could have enabled teams live in production to queue up their seize policy ahead of the Cobalt upgrade and have zero race conditions or risk of service disturbance. We'll still hit this for Cobalt as we can't change this logic until the upgrade goes live, but I think this sets us up better for changes like this in the future.
Collaborator
There was a problem hiding this comment.
Additionally, we should start to temp check this rollout plan with active customers inside Coinbase on stock and wrapped asset products
ilikesymmetry
requested changes
Jul 27, 2026
ilikesymmetry
left a comment
ilikesymmetry
left a comment
Collaborator
There was a problem hiding this comment.
Seeking alignment on policy name, storage location, and revisiting behavior of only supporting specific policy scopes.
…k on seize - Reorder seize emissions so Memo immediately follows Transfer (per the IB20 Memo invariant): Transfer -> Memo -> BurnedBlocked/TransferredFromBlocked. - transferFromBlockedWithMemo: drop the from==address(0) check (keep to!=0), matching the burn-blocked family; a zero/empty from fails the seizable or balance check anyway. Removes the now-obsolete InvalidSender test. Addresses review from @ilikesymmetry. Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
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
4 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.
Summary
Adds the seize operation class to the base-std reference + ABI (the source of truth the fork harness cross-validates against base/base), per the BOP-439 PPS. Implements BOP-440 and, per review, also folds in the BOP-471
burnBlockedrepoint (base-std portion).New surface:
transferFromBlockedWithMemo(address from, address to, uint256 amount, bytes32 memo)— admin seize that reassigns a blocked account's balance in one call. Skips allowance and the transfer policies; the only membership check is thatfromis blocked under the newSEIZABLE_POLICY.tois not policy-checked (admin op). EmitsTransfer+TransferredFromBlocked+Memo.burnBlockedWithMemo(address from, uint256 amount, bytes32 memo)— memo variant of burn-based seizure.SEIZABLE_POLICYscope,TRANSFER_FROM_BLOCKED_ROLE, andSEIZEPausableFeature.Seize operation class (all three gate uniformly on
SEIZABLE_POLICY+ theSEIZEpause vector):burnBlockedrepointed fromTRANSFER_SENDER_POLICY/BURNpause toSEIZABLE_POLICY/SEIZEpause. This is a breaking change, acceptable becauseburnBlockedis not yet in use; intent is a single uniform seize gate rather than a transient split.Design notes
SEIZABLE_POLICY's id is packed into the previously-reserved lane 3 (bits 192..255) of theTransferPolicyIdsslot. No existing field offsets move.SEIZEpause → role →SEIZABLE_POLICYblock check) and moves balance via a shared_moveBalanceprimitive; it does not reuse the factory-bootstrapprivilegedpath.Testing
forge test(non-fork): 684 passed, 0 failed.forge fmt --checkclean.burnBlockedtests updated to the seize gating; new happy-path + revert-order suites for both new functions.