Skip to content

feat(contracts-bedrock): add compliance module for cross-chain transaction screening#19321

Draft
tynes wants to merge 14 commits intodevelopfrom
feat/compliance
Draft

feat(contracts-bedrock): add compliance module for cross-chain transaction screening#19321
tynes wants to merge 14 commits intodevelopfrom
feat/compliance

Conversation

@tynes
Copy link
Copy Markdown
Contributor

@tynes tynes commented Feb 26, 2026

Summary

Design doc: ethereum-optimism/design-docs#367

  • Adds an optional compliance screening layer for cross-chain transactions on OptimismPortal2 (L1 deposits) and L2ToL1MessagePasser (L2 withdrawals)
  • Introduces Compliance abstract contract with configurable rules, pending/approved/rejected status tracking, and ETH escrow for flagged transactions
  • Concrete L1Compliance and L2Compliance implementations override _executeApproved to call the correct bridge
  • Adds approved() and donateETH() callbacks on OptimismPortal2 and L2ToL1MessagePasser for compliance settlement
  • Updates ICompliance, IL2ToL1MessagePasserCGT, and IOptimismPortal2 interfaces
  • Bumps OPContractsManager to 6.0.4 and OPContractsManagerV2 to 7.0.9
  • Comprehensive test coverage across unit, integration, and end-to-end tests

🤖 Generated with Claude Code

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 2, 2026

Codecov Report

❌ Patch coverage is 95.41985% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.4%. Comparing base (69bbccc) to head (a36dd58).
⚠️ Report is 236 commits behind head on develop.

Files with missing lines Patch % Lines
...ges/contracts-bedrock/src/libraries/Predeploys.sol 0.0% 3 Missing ⚠️
...kages/contracts-bedrock/src/L1/OptimismPortal2.sol 93.3% 1 Missing ⚠️
...ntracts-bedrock/src/L2/L2StandardBridgeInterop.sol 0.0% 1 Missing ⚠️
...ges/contracts-bedrock/src/universal/Compliance.sol 98.6% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #19321      +/-   ##
===========================================
+ Coverage     75.8%    79.4%    +3.5%     
===========================================
  Files          683      142     -541     
  Lines        72644     6938   -65706     
===========================================
- Hits         55092     5509   -49583     
+ Misses       17408     1429   -15979     
+ Partials       144        0     -144     
Flag Coverage Δ
cannon-go-tests-64 ?
contracts-bedrock-tests 79.4% <95.4%> (-0.5%) ⬇️
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/contracts-bedrock/src/L1/L1Compliance.sol 100.0% <100.0%> (ø)
...contracts-bedrock/src/L1/OptimismPortalInterop.sol 98.7% <100.0%> (+0.1%) ⬆️
...racts-bedrock/src/L1/opcm/OPContractsManagerV2.sol 97.7% <100.0%> (ø)
...ckages/contracts-bedrock/src/L2/GasPriceOracle.sol 100.0% <ø> (ø)
packages/contracts-bedrock/src/L2/L1BlockCGT.sol 100.0% <100.0%> (ø)
packages/contracts-bedrock/src/L2/L2Compliance.sol 100.0% <100.0%> (ø)
...es/contracts-bedrock/src/L2/L2ContractsManager.sol 0.0% <ø> (ø)
...ontracts-bedrock/src/L2/L2CrossDomainMessenger.sol 84.6% <ø> (ø)
...ckages/contracts-bedrock/src/L2/L2ERC721Bridge.sol 90.0% <ø> (ø)
packages/contracts-bedrock/src/L2/L2ProxyAdmin.sol 100.0% <ø> (ø)
... and 12 more

... and 544 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines +132 to +133
address proxyAdmin = Storage.getAddress(Constants.PROXY_OWNER_ADDRESS);
if (msg.sender != proxyAdmin && msg.sender != IProxyAdmin(proxyAdmin).owner()) {
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.

You can import ProxyAdminOwned here. It was recently moved into src/L2.

}
}

/// @notice Sets the compliance module address. Only callable by the proxy admin or its owner.
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.

We need to put some thought into which account to use here. Should it be the chain operator?

Making it the PAO will be onerous process wise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe the PAO is required here to ensure that stage 1 status can be maintained. You lower the amount of collusion required to censor. Perhaps we can set this in genesis if the feature is on, which should handle most cases, then the option still exists to enable it later or disable it later for chains that want it

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.

OK, I agree that enabling in genesis as the primary mechanism makes the most sense.


/// @notice Adds a compliance rule to the set. Reverts if already present.
/// @param _rule The address of the rule contract.
function addRule(address _rule) external onlyOwner {
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.

rather than using Ownable in this contract, would it make sense to put this value into L1Block?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This contract lives on both L1 and L2, so I do not think it makes sense to enshrine logic in L1Block, unless you also suggest that it is enshrined in SystemConfig as well for symmetry

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.

I see, I did not consider that properly.

I do think it would be nice to have a symmetry like that, as I generally dislike having roles configure via Ownable within a contract, as it is usually harder to inspect and manage. This is especially case when you have a situation where the owner is always meant to be the same as as some other pre-existing entity, for example DelayedWeth used to be OwnableUpgradable, but the owner was just meant to the ProxyAdminOwner (fixed in this PR).

But before we make any changes, I think the first thing to do is figure out who actually should have this power. If stage 1 still matters, then it seems to me that it should be the proxy admin owner based on my understanding of the potential power of a rule.

@tynes tynes force-pushed the feat/compliance branch 3 times, most recently from b7afcfe to 0bd9e40 Compare April 1, 2026 15:08
tynes and others added 13 commits April 3, 2026 14:44
…ction screening

Add a compliance screening layer for cross-chain transactions (deposits and
withdrawals). When enabled, transactions pass through configurable rules before
execution. Flagged transactions are held pending; compliant transactions proceed
without delay.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d version bumps

Expand ICompliance and IL2ToL1MessagePasserCGT interfaces to include
Solady Ownable, ProxyAdminOwnedBase, and ReinitializableBase inherited
events/errors/functions. Apply forge fmt formatting across compliance
contracts and tests. Bump OPContractsManager to 6.0.4 and
OPContractsManagerV2 to 7.0.9. Regenerate ABI and storage layout
snapshots for all affected contracts.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s and interfaces

Exclude IOwnable from the interface checker since Solady's Ownable has a
different ABI than the minimal IOwnable interface. Rename compliance test
contracts and functions to conform to the project's naming conventions:
contract names must map to source contract functions, and test function
names must have 3 or 4 underscore-separated parts.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…le test

Compliance contracts are not deployed as part of the standard deployment
script, so they need to be excluded from the reinitialization test.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n is enabled

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pliance module

Resolve conflicts from rebase on develop: bump COMPLIANCE dev feature
bit to avoid collision with SUPER_ROOT_GAMES_MIGRATION, fix Predeploys
address checksum, fix Compliance.sol import path, and regenerate snapshots.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Passer

Replace hand-rolled proxy admin ownership check in setCompliance() with
ProxyAdminOwnedBase inheritance, aligning with the pattern used by all
other L2 predeploys (FeeVault, L2StandardBridge, L2CrossDomainMessenger, etc.).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Patch-bump semver for contracts with changed initCodeHash due to
  transitive dependency changes from the compliance module
- Fix spacer check to exclude .dispute.json artifacts for legacy spacer
  contracts
- Regenerate nut-bundle and snapshots

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…deHash change

The initCodeHash changed due to a transitive dependency update but the
version was not bumped, causing the semver-diff check to fail.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
OptimismPortalInterop was missing the compliance module integration that
was added to OptimismPortal2. This caused L1Compliance tests to fail
when the OPTIMISM_PORTAL_INTEROP feature flag was enabled.

Adds compliance storage variable, approved() callback, and compliance
check in depositTransaction() to OptimismPortalInterop, matching the
OptimismPortal2 implementation. Updates the initialize() signature to
accept _compliance parameter and updates all call sites.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@tynes tynes force-pushed the feat/compliance branch from 24f87a2 to d124769 Compare April 3, 2026 19:06
…eployments and guard L2_FORK_RPC_URL

Increase deployment gas limits for L2ToL1MessagePasser and L2ToL1MessagePasserCGT after compliance feature
addition, and add an early exit guard for missing L2_FORK_RPC_URL in prepare-l2-upgrade-env.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines +603 to +605
bool allowed = ICompliance(compliance).check{ value: msg.value }(
msg.sender, _to, _value, _gasLimit, _isCreation, _data, 0
);
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.

I think it causes two identical deposit transactions to conflict, since they are gonna generate the same id and they are going to be overwritten. Should we add a nonce for deposits? or add a require(_status[id] == 0

if (!SafeCall.send(_from, _mint)) revert ICompliance.Compliance_TransferFailed();
}
}
// If still Pending, do nothing — leave it for later settlement.
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.

Wondering if a timeout here would make sense, for cases where the owner never acts for any reason, so that pending deposits can get the rejected status and refund. Something like flagTime + T <= block.timestamp check and settle() could auto-resolve to Rejected.

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.

3 participants