ERC20 Price #792
ERC20 Price #792
Conversation
WalkthroughAdds an L2TokenRegistry (interface + upgradeable implementation), registers its predeploy address and bindings (including deployed bytecode), wires genesis/config (EmeraldTime) and immutables, adds Hardhat tasks and token dataset, provides on-chain storage utilities/tests and Forge tests, and updates related bindings, hexes, and deployment checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Owner
participant Proxy as L2TokenRegistry Proxy
participant Impl as L2TokenRegistry Impl
participant Storage
rect rgba(200,220,255,0.12)
Note over Owner,Proxy: Owner registers tokens (owner-only)
Owner->>Proxy: registerToken(id, addr, slot, scale)
Proxy->>Impl: delegatecall registerToken(...)
Impl->>Storage: write tokenRegistry[id], tokenRegistration[addr]
Impl-->>Owner: emit TokenRegistered
end
rect rgba(200,255,220,0.12)
Note over Caller,Proxy: Price update (onlyAllowed)
Caller->>Proxy: updatePriceRatio(id, price)
Proxy->>Impl: delegatecall updatePriceRatio(...)
Impl->>Storage: write priceRatio[id]
Impl-->>Caller: emit PriceRatioUpdated
end
rect rgba(255,240,200,0.10)
Note over Caller,Proxy: Read flow
Caller->>Proxy: getTokenInfo(id)
Proxy->>Impl: delegatecall getTokenInfo(id)
Impl->>Storage: read tokenRegistry[id]
Impl-->>Caller: return TokenInfo
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
contracts/contracts/l2/system/ERC20PriceOracle.sol (4)
335-355: Use mulDiv to avoid overflow and define rounding.Multiplications can overflow on extreme inputs; mulDiv is safer and explicit.
Apply:
+import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; @@ - uint256 scaledPrice = _ethGasPrice * (10 ** decimals); - // Convert to token price - tokenGasPrice = scaledPrice / ratio; + tokenGasPrice = Math.mulDiv(_ethGasPrice, 10 ** decimals, ratio); @@ - uint256 scaledPrice = _tokenGasPrice * ratio; - ethGasPrice = scaledPrice / (10 ** decimals); + ethGasPrice = Math.mulDiv(_tokenGasPrice, ratio, 10 ** decimals);Also applies to: 365-380
4-4: Bind to the interface for compile-time conformance.Declare implementation of IERC20PriceOracle to catch signature drift.
Apply:
-import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import {IERC20PriceOracle} from "./IERC20PriceOracle.sol"; @@ -contract ERC20PriceOracle is OwnableUpgradeable { +contract ERC20PriceOracle is OwnableUpgradeable, IERC20PriceOracle {Also applies to: 10-16
95-108: Initialize Ownable via initializer helper (version-dependent).If using OZ v5 upgradeable, prefer _Ownable_init(owner); for v4, use __Ownable_init() then transferOwnership(owner).
Confirm OZ version and apply accordingly:
- function initialize(address owner_) external initializer { - _transferOwnership(owner_); - allowListEnabled = true; - } + function initialize(address owner_) external initializer { + // For OZ v5: + // __Ownable_init(owner_); + // For OZ v4.x: + __Ownable_init(); + _transferOwnership(owner_); + allowListEnabled = true; + }
318-323: Consider reverting when price not set.Returning 0 for unset price may mask misconfiguration; callers might treat 0 as a valid price.
Revert with InvalidPrice() if priceRatio[_tokenID] == 0.
contracts/contracts/l2/system/IERC20PriceOracle.sol (1)
58-69: Leverage InvalidTokenID in impl.Error is defined but not used by the contract; see suggested impl changes to validate nonzero IDs.
contracts/contracts/test/ERC20PriceOracle.t.sol (1)
441-456: Add tests for duplicate protection and tokenID=0.To lock in the fixes and sentinel semantics.
Apply:
+ function test_registerToken_reverts_when_duplicate_tokenID() public { + vm.prank(owner); + priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC); + vm.expectRevert(bytes4(keccak256("TokenAlreadyRegistered()"))); + vm.prank(owner); + priceOracle.registerToken(TOKEN_ID_USDC, address(usdt), BALANCE_SLOT_USDT); + } + + function test_registerToken_reverts_when_duplicate_tokenAddress() public { + vm.startPrank(owner); + priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC); + vm.expectRevert(bytes4(keccak256("TokenAlreadyRegistered()"))); + priceOracle.registerToken(TOKEN_ID_USDT, address(usdc), BALANCE_SLOT_USDC); + vm.stopPrank(); + } + + function test_registerToken_reverts_when_tokenID_zero() public { + vm.expectRevert(bytes4(keccak256("InvalidTokenID()"))); + vm.prank(owner); + priceOracle.registerToken(0, address(usdc), BALANCE_SLOT_USDC); + } + + function test_updateTokenInfo_reverts_when_address_already_registered() public { + vm.startPrank(owner); + priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC); + priceOracle.registerToken(TOKEN_ID_USDT, address(usdt), BALANCE_SLOT_USDT); + vm.expectRevert(bytes4(keccak256("TokenAlreadyRegistered()"))); + priceOracle.updateTokenInfo(TOKEN_ID_USDC, address(usdt), BALANCE_SLOT_USDC, true); + vm.stopPrank(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/contracts/l2/system/ERC20PriceOracle.sol(1 hunks)contracts/contracts/l2/system/IERC20PriceOracle.sol(1 hunks)contracts/contracts/test/ERC20PriceOracle.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
contracts/contracts/l2/system/ERC20PriceOracle.sol (1)
131-144: Anyone can set prices/discounts when allow list is disabled—intentional?Disabling allow list permits arbitrary callers to update priceRatio and feeDiscountPercent. High-risk if toggled in production.
Would you like these functions to always require owner or an explicit role even when allowListEnabled == false? If so, we can gate with “owner() || allowList[msg.sender]” regardless of the flag, or introduce roles (e.g., AccessControl).
Also applies to: 285-312, 407-422
contracts/contracts/l2/system/IERC20PriceOracle.sol (1)
14-21: Interface looks consistent with implementation.Surface matches impl; good event and error coverage.
Also applies to: 26-53, 58-69, 91-110, 135-148, 175-187, 193-206, 211-217
contracts/contracts/test/ERC20PriceOracle.t.sol (4)
85-95: Good coverage of registration basics and auto-decimals.Tests read well and validate expected state.
Also applies to: 130-153, 182-206
155-164: Array-length error selection is correct.Matches InvalidArrayLength in impl.
366-376: Fee discount access/limits covered.Boundaries and allow-list behavior are exercised.
Also applies to: 387-395
249-260: Double-check intent: price updates allowed by anyone when allow list disabled.Test encodes this policy. Confirm it’s desirable for mainnet.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
contracts/contracts/l2/system/ERC20PriceOracle.sol (3)
205-232: Critical: Registration allows overwrites and tokenID=0.This issue was flagged in a previous review and remains unaddressed. The uniqueness check at lines 210-212 is broken:
- Silent overwrites: If
tokenRegistry[_tokenID].tokenAddress != address(0)(tokenID already registered), the check doesn't revert, allowing the existing entry to be silently overwritten.- tokenID=0 permitted: Zero is used as a sentinel value in
getTokenIdByAddress(line 398), creating ambiguity.- Incomplete address check: The condition only checks address reuse when the tokenID is new.
As per the previous review, apply this fix:
function _registerSingleToken(uint16 _tokenID, address _tokenAddress, bytes32 _balanceSlot, uint256 _scale) internal { // Check token address if (_tokenAddress == address(0)) revert InvalidTokenAddress(); - - // Check if already registered - if (tokenRegistry[_tokenID].tokenAddress == address(0) && tokenRegistration[_tokenAddress] != 0) { - revert TokenAlreadyRegistered(); - } + + // Forbid zero ID and enforce uniqueness for both ID and address + if (_tokenID == 0) revert InvalidTokenID(); + if (tokenRegistry[_tokenID].tokenAddress != address(0)) revert TokenAlreadyRegistered(); + if (tokenRegistration[_tokenAddress] != 0) revert TokenAlreadyRegistered();
242-281: Critical: Address collision allowed during updates.This issue was flagged in a previous review and remains unaddressed. When updating a token's address, the code doesn't verify that the new address isn't already registered to a different tokenID (lines 275-278).
Impact: Updating tokenID 1 to an address already owned by tokenID 2 will corrupt
tokenRegistration, causinggetTokenIdByAddressto return inconsistent results.As per the previous review, apply this fix:
function updateTokenInfo( uint16 _tokenID, address _tokenAddress, bytes32 _balanceSlot, bool _isActive, uint256 _scale ) external onlyOwner { // Check if token exists if (tokenRegistry[_tokenID].tokenAddress == address(0)) revert TokenNotFound(); // Check new information if (_tokenAddress == address(0)) revert InvalidTokenAddress(); + + // Prevent address being shared across different tokenIDs + uint16 existing = tokenRegistration[_tokenAddress]; + if (existing != 0 && existing != _tokenID) revert TokenAlreadyRegistered();
470-474: Critical: Missing storage gap for upgrade safety.This issue was flagged in a previous review and remains unaddressed. The contract is upgradeable but lacks a reserved storage gap. Adding new state variables in a future upgrade will shift existing storage slots, corrupting data.
As per the previous review, add a storage gap before the closing brace:
function isTokenActive(uint16 _tokenID) external view returns (bool) { if (tokenRegistry[_tokenID].tokenAddress == address(0)) return false; return tokenRegistry[_tokenID].isActive; } + + /// @dev Reserved storage space to allow for layout changes in future upgrades + uint256[50] private __gap; }
🧹 Nitpick comments (2)
contracts/contracts/l2/system/ERC20PriceOracle.sol (2)
109-112: Redundant storage write in initialize.Line 111 sets
allowListEnabled = true, but this variable is already initialized totrueat its declaration (line 50). This wastes gas during initialization.Apply this diff to remove the redundant assignment:
function initialize(address owner_) external initializer { _transferOwnership(owner_); - allowListEnabled = true; }
386-473: Inconsistent handling of non-existent tokens in view functions.View functions handle non-existent tokens inconsistently:
getTokenInfo(line 387),getTokenPrice(line 342),getTokenScale(line 458),getFeeDiscountPercent(line 429): revert withTokenNotFoundisTokenActive(line 471): returnsfalsegetTokenIdByAddress(line 398): reverts only iftokenID == 0andtokenAddress != address(0)Consider standardizing this behavior—either always revert or always return safe defaults—to avoid confusion for integrators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/contracts/l2/system/ERC20PriceOracle.sol(1 hunks)contracts/contracts/l2/system/IERC20PriceOracle.sol(1 hunks)contracts/contracts/test/ERC20PriceOracle.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
contracts/contracts/test/ERC20PriceOracle.t.sol (6)
74-83: LGTM! Initialization tests are solid.The tests correctly verify proxy initialization behavior and guard against direct implementation initialization.
237-318: LGTM! Price management tests are comprehensive.The tests effectively cover price updates, access control via allow-list, and batch operations.
324-373: LGTM! Gas price calculations are correct.The tests verify the price conversion formulas for tokens with different decimals (6 and 18) and include inverse calculation verification.
379-407: LGTM! Fee discount tests cover key scenarios.The tests properly validate boundary conditions (>100%) and access control.
413-448: LGTM! Allow-list tests are comprehensive.The tests verify batch operations, array validation, and enable/disable toggle functionality.
454-468: LGTM! View function tests are correct.The tests verify both the happy path and edge case for non-existent tokens.
contracts/contracts/l2/system/IERC20PriceOracle.sol (2)
14-21: LGTM! TokenInfo struct is well-defined.The struct documentation clearly explains the scale field's role in the price ratio formula.
61-71: LGTM! Error declarations are comprehensive.The custom errors cover all validation and access control scenarios.
contracts/contracts/l2/system/ERC20PriceOracle.sol (5)
123-149: LGTM! Allow-list implementation is sound.The allow-list mechanism correctly:
- Validates array lengths before batch updates
- Emits events for each change
- Exempts the owner from allow-list checks
- Provides a toggle to enable/disable the feature
287-295: LGTM! Token deactivation logic is correct.The function properly validates token existence before deactivating and emits an appropriate event.
307-316: LGTM! Price ratio update is well-validated.The function correctly checks token existence and rejects zero prices before updating.
411-431: LGTM! Fee discount management is properly bounded.Both functions correctly validate token existence, and the update function enforces a reasonable maximum of 100% (10000 basis points).
470-473: LGTM! Active status check handles unregistered tokens gracefully.The function returns false for unregistered tokens rather than reverting, which is a sensible default.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/contracts/l2/system/ERC20PriceOracle.sol (1)
324-334: Consider surfacing skipped entries in batch updates for better observability.The
batchUpdatePricesfunction silently skips entries when the token is not registered or the price is zero (lines 328-329). While this enables partial success, it may hide configuration errors.For better operational observability, consider emitting events or returning a summary when entries are skipped, so callers can detect which updates were not applied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bindings/Makefile(3 hunks)bindings/bin/erc20priceoracle_deployed.hex(1 hunks)bindings/bin/l2staking_deployed.hex(1 hunks)bindings/bindings/erc20priceoracle_more.go(1 hunks)bindings/bindings/l2staking_more.go(1 hunks)bindings/predeploys/addresses.go(1 hunks)contracts/contracts/l2/system/ERC20PriceOracle.sol(1 hunks)contracts/contracts/l2/system/IERC20PriceOracle.sol(1 hunks)contracts/contracts/test/ERC20PriceOracle.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- bindings/bindings/l2staking_more.go
- bindings/bin/erc20priceoracle_deployed.hex
🧰 Additional context used
🧬 Code graph analysis (1)
bindings/bindings/erc20priceoracle_more.go (1)
bindings/solc/types.go (1)
StorageLayout(47-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
bindings/Makefile (1)
55-56: LGTM! Binding integration is correctly structured.The new ERC20PriceOracle binding target follows the established patterns:
- Added to the bindings list (line 56)
- Proper binding target defined (lines 122-123)
- Included in the contracts list for the
moretarget (line 197)The integration is consistent with existing L2 system contract bindings.
Also applies to: 122-123, 197-197
bindings/bindings/erc20priceoracle_more.go (1)
1-25: Generated binding file follows expected patterns.This is a code-generated file that properly:
- Unmarshals storage layout JSON
- Registers the layout and deployed bytecode in the binding registry
- Follows the standard structure for Morph L2 contract bindings
No manual changes required.
bindings/bin/l2staking_deployed.hex (1)
1-1: Bytecode update noted.This deployed bytecode file is part of the broader L2Staking deployment updates. Binary bytecode cannot be meaningfully reviewed at the source level. Ensure that:
- The bytecode was generated from the correct source contracts
- Compilation flags and optimizer settings match production requirements
- The bytecode has been verified against the source on relevant block explorers
contracts/contracts/l2/system/IERC20PriceOracle.sol (1)
1-183: Interface definition is clean and comprehensive.The interface properly declares:
- TokenInfo struct matching the implementation
- Organized function groups (Allow List, Token Registration, Price Management, Fee Discount, Scale Management, View Functions)
- Clear NatSpec documentation for all functions
The past review comments about event signature mismatches and phantom functions appear to have been resolved - this interface doesn't declare events (they're in the implementation contract) and doesn't include the previously mentioned
updatePriceRatioFromPricesphantom function.contracts/contracts/test/ERC20PriceOracle.t.sol (1)
1-506: Excellent test coverage addressing all previous concerns.The test suite comprehensively covers:
- Initialization and proxy deployment
- Token registration edge cases including all past review concerns:
- Zero tokenID rejection (lines 100-104) ✓
- Duplicate tokenID prevention (lines 106-113) ✓
- Address reuse prevention (lines 115-122) ✓
- Address collision during updates (lines 229-240) ✓
- Price management with access control
- Allow-list functionality
- Fee discount management
- View functions and edge cases
The tests properly use
vm.expectRevertwith specific error signatures and validate both success and failure paths. This addresses all the gaps mentioned in past review comments.contracts/contracts/l2/system/ERC20PriceOracle.sol (1)
1-481: Implementation is solid with all critical past issues resolved.The contract properly implements:
- Upgradeable ownership with initialization protection
- TokenID=0 rejection and uniqueness enforcement (lines 208-211) ✓
- Address collision prevention on updates (lines 254-255) ✓
- Zero-amount and zero-price validations (lines 378, 450) ✓
- Storage gap for upgrade safety (line 480) ✓
- Access control via allow-list and ownership
All critical and major issues from past reviews have been addressed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/contracts/l2/system/ERC20PriceOracle.sol (1)
320-330: Note: Batch update skip behavior was previously reviewed.The silent
continuestatements on lines 324-325 were flagged in a previous review as potentially hiding errors. This design allows partial batch success, which may be intentional. No further action needed if this behavior is confirmed as desired.
🧹 Nitpick comments (2)
contracts/contracts/l2/system/ERC20PriceOracle.sol (2)
47-47: Redundant default value for allowListEnabled.Line 47 sets
allowListEnabled = truein storage, and line 106 sets it again in the initializer. For upgradeable contracts, state should be initialized only in the initializer to avoid ambiguity.Consider removing the default value here:
- bool public allowListEnabled = true; + bool public allowListEnabled;The initializer (line 106) will set it to
truewhen the proxy is initialized.
250-251: Fix indentation.Lines 250-251 have inconsistent indentation with extra leading spaces.
- // Prevent address being shared across different tokenIDs - uint16 existing = tokenRegistration[_tokenAddress]; - if (existing != 0 && existing != _tokenID) revert TokenAlreadyRegistered(); + // Prevent address being shared across different tokenIDs + uint16 existing = tokenRegistration[_tokenAddress]; + if (existing != 0 && existing != _tokenID) revert TokenAlreadyRegistered();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/contracts/l2/system/ERC20PriceOracle.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
contracts/contracts/l2/system/ERC20PriceOracle.sol (12)
1-27: LGTM: Contract setup and data structures are well-defined.The contract properly inherits from OwnableUpgradeable, defines a minimal IERC20Infos interface for decimals queries, and structures TokenInfo appropriately for token metadata management.
50-89: LGTM: Comprehensive events and clear error definitions.Events properly cover all state changes, and custom errors provide clear, gas-efficient validation failure messages.
95-107: LGTM: Standard upgradeable initialization pattern.The constructor correctly disables initializers, and the initialize function properly sets up ownership and configuration using the
initializermodifier.
109-144: LGTM: Allow list implementation is sound.The batch setter properly validates array lengths, the toggle function is straightforward, and the
onlyAllowedmodifier correctly implements three-way access control (allowList, owner override, and enable/disable flag).
146-226: LGTM: Token registration logic is robust and properly validates uniqueness.The batch and single registration functions correctly implement validation to forbid tokenID=0 and prevent duplicate registrations (both by ID and address). The decimals retrieval with try-catch fallback is a good defensive pattern.
236-278: LGTM: Update logic correctly handles address changes and prevents collisions.The updateTokenInfo function properly validates the new address doesn't collide with another tokenID's registration, re-fetches decimals when the address changes, and efficiently updates the reverse mapping only when necessary.
284-292: LGTM: Token deactivation is straightforward.The deactivateToken function safely sets isActive to false after validating the token exists.
304-313: LGTM: Single price update has proper validation.The updatePriceRatio function correctly validates token existence and non-zero price before updating.
332-377: LGTM: Price queries and calculations are well-validated.The getTokenPrice and calculateTokenAmount functions properly validate token existence and handle edge cases. The zero-result check on line 374 prevents returning misleading zero amounts for small ETH values.
379-398: LGTM: View functions correctly handle lookups and edge cases.The getTokenInfo function validates existence before returning data. The getTokenIdByAddress function correctly treats 0 as the sentinel for "not found" while still allowing queries for address(0) itself (which would validly return 0).
400-429: LGTM: Scale management properly validates non-zero values.The updateTokenScale function correctly prevents setting scale to zero (line 415), which would break calculateTokenAmount. The getTokenScale function is straightforward.
431-446: LGTM: View function and upgrade safety gap are properly implemented.The isTokenActive function safely handles non-existent tokens, and the storage gap (line 445) reserves space for future upgrades without breaking storage layout.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ops/l2-genesis/morph-chain-ops/genesis/setters.go (1)
89-91: Remove unnecessary no-op code.This conditional block serves no functional purpose - it checks if the name is "ERC20PriceOracle" and then discards the value. This appears to be leftover debugging code or a placeholder that should be removed.
Apply this diff to remove the no-op:
- if name == "ERC20PriceOracle" { - _ = name - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bindings/bin/erc20priceoracle_deployed.hex(1 hunks)bindings/bindings/erc20priceoracle_more.go(1 hunks)bindings/predeploys/addresses.go(3 hunks)ops/l2-genesis/morph-chain-ops/genesis/config.go(1 hunks)ops/l2-genesis/morph-chain-ops/genesis/setters.go(2 hunks)ops/l2-genesis/morph-chain-ops/immutables/immutables.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- bindings/bin/erc20priceoracle_deployed.hex
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/predeploys/addresses.go
🧰 Additional context used
🧬 Code graph analysis (3)
ops/l2-genesis/morph-chain-ops/genesis/config.go (1)
ops/l2-genesis/morph-chain-ops/state/state.go (1)
StorageValues(24-24)
ops/l2-genesis/morph-chain-ops/immutables/immutables.go (1)
bindings/bindings/erc20priceoracle.go (1)
DeployERC20PriceOracle(56-70)
bindings/bindings/erc20priceoracle_more.go (1)
bindings/solc/types.go (1)
StorageLayout(47-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
bindings/bindings/erc20priceoracle_more.go (1)
1-25: LGTM! Generated binding follows standard patterns.The generated Go binding correctly follows the established pattern for storage layout and deployed bytecode registration. The panic on JSON unmarshal error in init() is appropriate for generated code, ensuring fail-fast behavior if the embedded data is malformed.
ops/l2-genesis/morph-chain-ops/immutables/immutables.go (2)
122-124: LGTM! ERC20PriceOracle deployment entry added correctly.The new deployment entry follows the established pattern and is properly positioned in the deployments list.
239-240: LGTM! Deploy function call follows the established pattern.The deployment case correctly invokes
bindings.DeployERC20PriceOraclewithout constructor arguments, consistent with the contract's deployment requirements.ops/l2-genesis/morph-chain-ops/genesis/config.go (1)
458-463: LGTM! Storage initialization follows the established pattern.The ERC20PriceOracle storage initialization correctly sets up the OpenZeppelin Initializable state (
_initialized,_initializing), ownership viaFinalSystemOwner, and enables the allow-list. This matches the pattern used by other system contracts like GasPriceOracle.ops/l2-genesis/morph-chain-ops/genesis/setters.go (1)
113-113: Change from shadowing to variable reuse is intentional and correct.Verification confirms the modification on line 113 was intentional. In the
SetTouchablefunction, theerrvariable is declared on line 102 and properly reused with=on line 113. This is consistent with the function's scope and follows Go best practices. The pattern differs from line 129 inSetUntouchableonly because that function has no prior error variable declared, requiring it to use:=. Both patterns are correct for their respective contexts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
contracts/contracts/test/ERC20PriceOracle.t.sol (6)
79-83: Consider using a specific revert error.The test uses a generic
vm.expectRevert(). For better clarity, specify the expected "Initializable: contract is already initialized" error.Apply this diff for more precise error checking:
function test_initialize_reverts_when_not_called_via_proxy() public { ERC20PriceOracle impl = new ERC20PriceOracle(); - vm.expectRevert(); + vm.expectRevert("Initializable: contract is already initialized"); impl.initialize(owner); }
89-210: Add test for zero scale validation.The registration tests are comprehensive, but there's no test verifying that a zero
scaleparameter is rejected. A zero scale would cause division-by-zero errors incalculateTokenAmount.Add this test case:
function test_registerToken_reverts_when_scale_is_zero() public { vm.expectRevert(bytes4(keccak256("InvalidScale()"))); vm.prank(owner); priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, 0); }
216-268: Add missing tests for update error cases.The update tests cover the happy path well, but several error scenarios are missing:
updateTokenInfowith an unregistered tokenupdateTokenInfowith zero addressupdateTokenInfowith zero scaledeactivateTokenaccess control (non-owner)deactivateTokenwith unregistered tokenupdateTokenScalefunction (mentioned in the AI summary but not tested)Add these test cases:
function test_updateTokenInfo_reverts_when_token_not_registered() public { vm.expectRevert(bytes4(keccak256("TokenNotFound()"))); vm.prank(owner); priceOracle.updateTokenInfo(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, true, SCALE_USDC); } function test_updateTokenInfo_reverts_when_address_zero() public { vm.prank(owner); priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, SCALE_USDC); vm.expectRevert(bytes4(keccak256("InvalidTokenAddress()"))); vm.prank(owner); priceOracle.updateTokenInfo(TOKEN_ID_USDC, address(0), BALANCE_SLOT_USDC, true, SCALE_USDC); } function test_updateTokenInfo_reverts_when_scale_is_zero() public { vm.prank(owner); priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, SCALE_USDC); vm.expectRevert(bytes4(keccak256("InvalidScale()"))); vm.prank(owner); priceOracle.updateTokenInfo(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, true, 0); } function test_deactivateToken_reverts_when_not_owner() public { vm.prank(owner); priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, SCALE_USDC); vm.expectRevert("Ownable: caller is not the owner"); vm.prank(alice); priceOracle.deactivateToken(TOKEN_ID_USDC); } function test_updateTokenScale_succeeds() public { vm.prank(owner); priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, SCALE_USDC); uint256 newScale = 1e8; vm.prank(owner); priceOracle.updateTokenScale(TOKEN_ID_USDC, newScale); assertEq(priceOracle.getTokenScale(TOKEN_ID_USDC), newScale); }
270-355: Add tests for price retrieval and calculation error cases.The price management tests cover access control well, but miss several error scenarios:
updatePriceRatiowith unregistered tokengetTokenPricewith unregistered tokenbatchUpdatePriceswith mismatched array lengthsbatchUpdatePricesaccess control validationcalculateTokenAmountwith unregistered tokenAdd these test cases:
function test_updatePriceRatio_reverts_when_token_not_registered() public { vm.expectRevert(bytes4(keccak256("TokenNotFound()"))); vm.prank(owner); priceOracle.updatePriceRatio(TOKEN_ID_USDC, 1e12); } function test_getTokenPrice_reverts_when_token_not_registered() public { vm.expectRevert(bytes4(keccak256("TokenNotFound()"))); priceOracle.getTokenPrice(TOKEN_ID_USDC); } function test_batchUpdatePrices_reverts_when_array_length_mismatch() public { uint16[] memory tokenIDs = new uint16[](2); uint256[] memory prices = new uint256[](1); vm.expectRevert(bytes4(keccak256("InvalidArrayLength()"))); vm.prank(owner); priceOracle.batchUpdatePrices(tokenIDs, prices); } function test_calculateTokenAmount_reverts_when_token_not_registered() public { vm.expectRevert(bytes4(keccak256("TokenNotFound()"))); priceOracle.calculateTokenAmount(TOKEN_ID_USDC, 1 gwei); }
361-410: Gas calculation math is correct; consider edge case tests.The calculation tests correctly verify conversions for both 6-decimal (USDC) and 18-decimal (DAI) tokens. The arithmetic is accurate.
Optionally, consider adding tests for:
- Very large ETH prices (near uint256 max) to verify no overflow
- Very small ETH prices to check precision/rounding behavior
- A test using an actual on-chain function for ETH-to-token conversion (line 377-394 manually computes the inverse)
412-471: Add missing access control and view function tests.The allow list tests work correctly but miss access control validation, and view function coverage is limited.
Add these test cases:
function test_setAllowList_reverts_when_not_owner() public { address[] memory users = new address[](1); bool[] memory allowed = new bool[](1); users[0] = alice; allowed[0] = true; vm.expectRevert("Ownable: caller is not the owner"); vm.prank(alice); priceOracle.setAllowList(users, allowed); } function test_setAllowListEnabled_reverts_when_not_owner() public { vm.expectRevert("Ownable: caller is not the owner"); vm.prank(alice); priceOracle.setAllowListEnabled(false); } function test_getTokenInfo_reverts_when_token_not_registered() public { vm.expectRevert(bytes4(keccak256("TokenNotFound()"))); priceOracle.getTokenInfo(TOKEN_ID_USDC); } function test_getTokenScale_succeeds() public { vm.prank(owner); priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, SCALE_USDC); assertEq(priceOracle.getTokenScale(TOKEN_ID_USDC), SCALE_USDC); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/contracts/test/ERC20PriceOracle.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
contracts/contracts/test/ERC20PriceOracle.t.sol (1)
1-68: LGTM! Test setup is well-structured.The proxy pattern setup, mock token deployments, and test constants are correctly configured. The use of different decimal precisions (6 and 18) for mock tokens ensures comprehensive coverage of decimal handling.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/Makefile(3 hunks)bindings/bin/l2tokenregistry_deployed.hex(1 hunks)bindings/bindings/l2tokenregistry_more.go(1 hunks)bindings/predeploys/addresses.go(3 hunks)contracts/contracts/l2/system/IL2TokenRegistry.sol(1 hunks)contracts/contracts/l2/system/L2TokenRegistry.sol(1 hunks)contracts/contracts/test/ERC20PriceOracle.t.sol(1 hunks)ops/l2-genesis/morph-chain-ops/genesis/config.go(1 hunks)ops/l2-genesis/morph-chain-ops/genesis/setters.go(1 hunks)ops/l2-genesis/morph-chain-ops/immutables/immutables.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ops/l2-genesis/morph-chain-ops/genesis/config.go
- bindings/predeploys/addresses.go
- bindings/Makefile
🧰 Additional context used
🧬 Code graph analysis (2)
ops/l2-genesis/morph-chain-ops/immutables/immutables.go (1)
bindings/bindings/l2tokenregistry.go (1)
DeployL2TokenRegistry(56-70)
bindings/bindings/l2tokenregistry_more.go (1)
bindings/solc/types.go (1)
StorageLayout(47-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
ops/l2-genesis/morph-chain-ops/immutables/immutables.go (1)
122-124: Entry addition acknowledged.Routing
L2TokenRegistrythrough the immutable deployment list makes sense and keeps the constructor ordering intact.bindings/bindings/l2tokenregistry_more.go (1)
1-25: Generated bindings look good.Storage layout parsing and deployed bytecode registration follow our usual pattern—no issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
contracts/contracts/l2/system/L2TokenRegistry.sol (1)
1-409: Implement the missing fee discount API declared in IL2TokenRegistry.The interface
IL2TokenRegistrydeclaresupdateFeeDiscountPercent(uint16, uint256)andgetFeeDiscountPercent(uint16)functions, but this contract does not implement them. Calls to these functions will revert with a function selector mismatch, breaking the advertised interface contract.Based on the past review comment, you need to add storage, validation, and the two public functions. Apply this implementation:
mapping(address => bool) public allowList; + + /// @notice Mapping from tokenID to fee discount percentage (in basis points, max 10000) + mapping(uint16 => uint256) public feeDiscountPercent; /// @notice Whether whitelist is enabled bool public allowListEnabled = true;Add the event after existing events:
event AllowListEnabledUpdated(bool isEnabled); + event FeeDiscountPercentUpdated(uint16 indexed tokenID, uint256 newPercent);Add the functions after
updateTokenScale:function updateTokenScale(uint16 _tokenID, uint256 _newScale) external onlyAllowed { // existing logic } + + /** + * @notice Update fee discount percentage for a token + * @param _tokenID Token ID + * @param _newPercent New discount percentage in basis points (0-10000, where 10000 = 100%) + */ + function updateFeeDiscountPercent(uint16 _tokenID, uint256 _newPercent) external onlyAllowed { + if (tokenRegistry[_tokenID].tokenAddress == address(0)) revert TokenNotFound(); + if (_newPercent > 10_000) revert InvalidPercent(); + feeDiscountPercent[_tokenID] = _newPercent; + emit FeeDiscountPercentUpdated(_tokenID, _newPercent); + } + + /** + * @notice Get fee discount percentage for a token + * @param _tokenID Token ID + * @return Discount percentage in basis points + */ + function getFeeDiscountPercent(uint16 _tokenID) external view returns (uint256) { + if (tokenRegistry[_tokenID].tokenAddress == address(0)) revert TokenNotFound(); + return feeDiscountPercent[_tokenID]; + }
🧹 Nitpick comments (1)
contracts/scripts/oracle-testing/token.go (1)
56-62: Clarify the comment about uint16 encoding.The comment states "right-padded" but the encoding is actually left-padded with zeros (the uint16 value is placed at bytes 30-31, the rightmost positions). Consider updating the comment to "left-padded with zeros" or "placed at the rightmost bytes" for clarity.
Apply this diff:
-// CalculateUint16MappingSlot calculates the storage slot for a mapping key -// For mapping(key => value), the slot is: keccak256(abi.encode(key, mappingSlot)) -func CalculateUint16MappingSlot(key uint16, mappingSlot common.Hash) common.Hash { - // Convert key to 32 bytes (right-padded) +// CalculateUint16MappingSlot calculates the storage slot for a mapping key +// For mapping(key => value), the slot is: keccak256(abi.encode(key, mappingSlot)) +func CalculateUint16MappingSlot(key uint16, mappingSlot common.Hash) common.Hash { + // Convert key to 32 bytes (left-padded with zeros, value at rightmost bytes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
bindings/bin/l2tokenregistry_deployed.hex(1 hunks)bindings/bindings/l2tokenregistry_more.go(1 hunks)contracts/contracts/l2/system/IL2TokenRegistry.sol(1 hunks)contracts/contracts/l2/system/L2TokenRegistry.sol(1 hunks)contracts/scripts/oracle-testing/token.go(1 hunks)contracts/scripts/oracle-testing/token_test.go(1 hunks)contracts/src/tokens/tokens.json(1 hunks)contracts/tasks/token_deploy.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/bin/l2tokenregistry_deployed.hex
🧰 Additional context used
🧬 Code graph analysis (2)
bindings/bindings/l2tokenregistry_more.go (1)
bindings/solc/types.go (1)
StorageLayout(47-50)
contracts/scripts/oracle-testing/token_test.go (1)
contracts/scripts/oracle-testing/token.go (11)
TokenRegistryAddress(21-21)AllowListEnabledSlot(44-44)CalculateUint16MappingSlot(58-76)TokenRegistrySlot(32-32)ParseTokenInfoFromStorage(111-155)CalculateAddressMappingSlot(90-107)TokenRegistrationSlot(35-35)PriceRatioSlot(38-38)AllowListSlot(41-41)CalculateStructFieldSlot(80-86)TokenConfig(11-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
contracts/scripts/oracle-testing/token.go (2)
23-45: Storage slots verified and correct.The hardcoded slots (151-155) in
token.gomatch exactly with the L2TokenRegistryStorageLayout from the contract bindings. The storage layout properly accounts for inherited state from OwnableUpgradeable (slot 51) and ReentrancyGuardUpgradeable (slot 101), with gaps reserved by each. No action needed.
21-21: Hardcoded address should be externalized for deployment flexibility.The address
0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0in token.go (line 21) is a hardcoded global variable in the oracle-testing utility scripts. While this directory contains development and testing code, the fact that your tests already override this address with different values (lines 17 and 131 in token_test.go show overrides to0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512and0x34B40BA116d5Dec75548a9e9A8f15411461E8c70respectively) indicates multiple deployments may be targeted.Consider:
- Making this an environment variable or CLI parameter instead of a global constant
- Or loading it from a configuration file that varies by deployment environment
- This improves reusability across different L2TokenRegistry deployments without code changes
contracts/contracts/l2/system/L2TokenRegistry.sol (1)
321-340: Verify the priceRatio composition against the documented specification.The review comment correctly identifies an inconsistency in the code and documentation. Here's what was found:
Key findings from code inspection:
- TokenInfo.scale field documentation states: "rateScaled = tokenScale * (tokenPrice / ethPrice) * 10^(ethDecimals - tokenDecimals)"
- Test data confirms scale = 1e6 for USDC (6 decimals), indicating scale represents 10^tokenDecimals
- The calculateTokenAmount implementation multiplies by scale: tokenAmount = (ethAmount * scale) / ratio
- Line 334 comment claims "ratio already contains tokenScale and decimals adjustment", yet the implementation multiplies by scale anyway
The contradiction:
Line 26 specifies: "priceRatio = tokenScale * (tokenPrice / ethPrice) * 10^(ethDecimals - tokenDecimals)", suggesting priceRatio includes the tokenScale factor. However, the actual implementation stores priceRatio without any scale adjustment, then multiplies by scale separately in the calculation. This means either:
- The specification at line 26 is incorrect, or
- The implementation doesn't follow the intended design
While tests show the code produces correct results (because scale equals 10^tokenDecimals), the contradiction between the documented formula and implementation logic remains unresolved. Please verify with domain experts whether the priceRatio calculation should follow line 26's specification or whether the documentation needs updating to reflect the current design.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
contracts/contracts/test/L2TokenRegistry.t.sol (8)
12-13: Consider renaming variables to match the contract type.The variables
priceOracleandpriceOracleImplare misleading since the contract type isL2TokenRegistry, not a price oracle. This naming mismatch reduces code clarity.Apply this diff to improve naming consistency:
- L2TokenRegistry internal priceOracle; - L2TokenRegistry internal priceOracleImpl; + L2TokenRegistry internal tokenRegistry; + L2TokenRegistry internal tokenRegistryImpl;Then update all references throughout the file accordingly.
78-82: Consider specifying the expected revert reason.Using
vm.expectRevert()without specifying the error is less precise and makes the test intent unclear. If the contract reverts with a different error, the test would still pass.If the implementation uses a specific error (e.g., "Initializable: contract is already initialized"), specify it:
function test_initialize_reverts_when_not_called_via_proxy() public { L2TokenRegistry impl = new L2TokenRegistry(); - vm.expectRevert(); + vm.expectRevert("Initializable: contract is already initialized"); impl.initialize(owner); }
88-97: Consider adding event assertions.Token registration likely emits events (e.g.,
TokenRegistered), but the test doesn't verify them. Event assertions improve test thoroughness by confirming that the contract communicates state changes properly.Add event expectations:
function test_registerToken_succeeds() public { + vm.expectEmit(true, true, true, true); + emit TokenRegistered(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, SCALE_USDC); vm.prank(owner); priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, SCALE_USDC);This pattern applies to other mutation operations throughout the test suite.
157-185: Verify batch registration maintains individual token constraints.The batch registration test confirms addresses are registered but doesn't verify that all individual constraints (decimals auto-fetch, isActive state, balance slots, scales) are correctly set for each token in the batch.
Add comprehensive assertions:
assertEq(priceOracle.getTokenInfo(TOKEN_ID_USDC).tokenAddress, address(usdc)); assertEq(priceOracle.getTokenInfo(TOKEN_ID_USDT).tokenAddress, address(usdt)); assertEq(priceOracle.getTokenInfo(TOKEN_ID_DAI).tokenAddress, address(dai)); + + // Verify all fields for each token + L2TokenRegistry.TokenInfo memory infoUSDC = priceOracle.getTokenInfo(TOKEN_ID_USDC); + assertEq(infoUSDC.decimals, 6); + assertEq(infoUSDC.balanceSlot, BALANCE_SLOT_USDC); + assertEq(infoUSDC.scale, SCALE_USDC); + assertFalse(infoUSDC.isActive);
254-273: Deactivation test uses batch operation indirectly.The deactivation test exercises
batchUpdateTokenStatuswith a single token rather than testing deactivation throughupdateTokenInfodirectly. While this works, it's less direct and could miss issues specific to the single-token update path.Consider adding a dedicated test that uses
updateTokenInfofor deactivation:function test_updateTokenInfo_can_deactivate() public { vm.prank(owner); priceOracle.registerToken(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, SCALE_USDC); // Activate vm.prank(owner); priceOracle.updateTokenInfo(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, true, SCALE_USDC); // Deactivate via updateTokenInfo vm.prank(owner); priceOracle.updateTokenInfo(TOKEN_ID_USDC, address(usdc), BALANCE_SLOT_USDC, false, SCALE_USDC); assertFalse(priceOracle.getTokenInfo(TOKEN_ID_USDC).isActive); }
362-415: Consider testing edge cases for price calculations.The current tests use reasonable values but don't exercise edge cases that could reveal precision loss, overflow, or division issues:
- Very small ETH gas prices (e.g., 1 wei)
- Very large ETH gas prices (e.g., 10000 gwei)
- Price ratios that result in fractional token amounts (precision loss)
- Zero price ratio (division by zero - should be prevented)
Add tests for these scenarios to ensure robust handling:
function test_calculateTokenAmount_with_small_eth_gas_price() public { // Test precision with 1 wei } function test_calculateTokenAmount_prevents_division_by_zero() public { // Ensure zero price ratio is rejected }
458-476: Expand view function test coverage.The view function tests only cover
isTokenActive. Other view functions likegetTokenPrice,getTokenInfo, andcalculateTokenAmountshould be tested with edge cases (non-existent tokens, inactive tokens, zero prices, etc.) to ensure they handle invalid states gracefully.Add tests for other view functions:
function test_getTokenPrice_returns_zero_for_nonexistent_token() public { // Verify behavior for unregistered token } function test_calculateTokenAmount_reverts_for_inactive_token() public { // Verify behavior when token is registered but inactive }
279-290: Add test for updating price on non-existent token to close test coverage gap.The implementation correctly validates token existence at line 269 of L2TokenRegistry.sol, but no existing test verifies this behavior. All five current updatePriceRatio tests register the token first. Add the suggested test case to ensure this edge case is covered:
function test_updatePriceRatio_reverts_when_token_not_registered() public { vm.expectRevert(L2TokenRegistry.TokenNotFound.selector); vm.prank(owner); priceOracle.updatePriceRatio(TOKEN_ID_USDC, 1e12); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/contracts/test/L2TokenRegistry.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
contracts/contracts/test/L2TokenRegistry.t.sol (5)
37-67: Well-structured test setup.The setUp function properly establishes the proxy pattern and mock token environment with appropriate labels for debugging.
366-380: Gas price calculation logic is correct.The test correctly verifies the token amount calculation:
(ethGasPrice * scale) / priceRatio. The math is accurate for the USDC case with 6 decimals.
382-399: Good validation of inverse calculation.This test validates the bidirectional conversion between ETH and token amounts by manually computing the inverse using on-chain values. This helps ensure the price ratio model is consistent in both directions.
421-435: Allowlist management tests are well-structured.The test correctly verifies batch allowlist operations with mixed true/false values, confirming independent user status management.
1-477: Well-structured test suite with room for improvement.This test file provides solid coverage of the L2TokenRegistry contract's core functionality including token registration, price management, and allowlist controls. The tests are well-organized with clear section markers and exercise both happy paths and basic error cases.
Strengths:
- Comprehensive coverage of main features
- Proper proxy pattern testing
- Multiple token decimal scenarios (6 and 18)
- Good organization with section comments
Areas for improvement:
- Replace fragile
bytes4(keccak256(...))error matching with error selectors- Add event assertions for state-changing operations
- Expand edge case coverage (non-existent tokens, zero values, overflows)
- Test more view functions with invalid inputs
- Consider variable naming consistency (priceOracle → tokenRegistry)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contracts/tasks/token_deploy.ts (3)
756-774: Handle missing token configuration gracefully.If a tokenID slot is available but the corresponding configuration is not found in
allTokensData(line 762), the code silently skips it. This could mask data integrity issues where the tokens.json file is incomplete or malformed.Consider adding a warning when a tokenID is available but no configuration exists:
if (!registeredTokenIDs.has(currentTokenID)) { const tokenData = allTokensData.find((t: any) => t.tokenID === currentTokenID) if (tokenData) { tokensToDeploy.push({ ...tokenData, tokenID: currentTokenID, scale: ethers.BigNumber.from(tokenData.scale), priceRatio: ethers.BigNumber.from(tokenData.priceRatio) }) deployedCount++ + } else { + console.warn(`⚠ TokenID ${currentTokenID} is available but no configuration found in tokens.json`) } }
732-741: Remove redundant 100-token limit check.The check at line 738 is redundant with the check at line 732. Both verify that
registeredTokenIDs.size >= 100.Apply this diff to remove the duplicate:
if (registeredTokenIDs.size >= maxTokenID) { console.log(`\nAll ${maxTokenID} token slots are already registered. Nothing to deploy.`) return } - // Check if total registered tokens would exceed 100 - if (registeredTokenIDs.size >= 100) { - console.log(`\n⚠ Warning: Already have ${registeredTokenIDs.size} registered tokens. Cannot register more than 100 tokens.`) - return - } - // Calculate how many tokens we can deploy (max 100 total)
655-971: Consider refactoring for maintainability and resilience.This task handles multiple concerns (validation, deployment, registration, price updates, verification) in a single 300+ line function. Additionally, there are potential resilience issues:
- Complexity: The nested error handling and fallback logic makes the control flow difficult to follow.
- Atomicity: If the task fails midway, the system is left in a partial state (some tokens deployed but not registered).
- Long execution: Sequential operations with many on-chain transactions could timeout.
Consider these improvements:
Extract helper functions for cleaner code:
loadTokenConfiguration(filePath: string)findAvailableTokenSlots(registry, maxSlots: number)deployToken(factory, config)registerTokenBatch(registry, tokens)Add constants for magic numbers:
const MAX_REGISTRY_TOKENS = 100 const DEFAULT_REGISTRY_ADDRESS = "0x5300000000000000000000000000000000000021"Add state persistence: Save deployed token addresses to a file so the task can resume if interrupted.
Add dry-run mode: Allow users to validate the deployment plan without executing transactions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/scripts/oracle-testing/token_test.go(1 hunks)contracts/src/tokens/tokens.json(1 hunks)contracts/tasks/token_deploy.ts(2 hunks)ops/l2-genesis/morph-chain-ops/genesis/layer_two.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/tokens/tokens.json
🧰 Additional context used
🧬 Code graph analysis (2)
ops/l2-genesis/morph-chain-ops/genesis/layer_two.go (1)
bindings/predeploys/addresses.go (1)
L2TokenRegistryAddr(60-60)
contracts/scripts/oracle-testing/token_test.go (1)
contracts/scripts/oracle-testing/token.go (11)
TokenRegistryAddress(21-21)AllowListEnabledSlot(44-44)CalculateUint16MappingSlot(58-76)TokenRegistrySlot(32-32)ParseTokenInfoFromStorage(111-155)CalculateAddressMappingSlot(90-107)TokenRegistrationSlot(35-35)PriceRatioSlot(38-38)AllowListSlot(41-41)CalculateStructFieldSlot(80-86)TokenConfig(11-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
contracts/tasks/token_deploy.ts (4)
7-8: LGTM!The fs and path imports are necessary for reading the tokens.json configuration file in the new deployment task.
617-653: LGTM!The deployment task follows the standard proxy pattern correctly with proper address validation and post-deployment verification.
656-656: Verify the hardcoded default registry address is appropriate.The default tokenregistry address
0x5300000000000000000000000000000000000021appears to be a predeploy address. Ensure this address is valid for all target networks where this task will be executed, or consider making it network-aware.
824-839: Verify the balance slot assumption for all token types.The comment at line 837 states "For MockERC20, balance mapping is typically at slot 0" but this is an assumption. The code reads
token.balanceSlotfrom the configuration, which is good, but ensure the tokens.json file has correct balanceSlot values for all token configurations. Incorrect slot values will cause balance queries to fail.Consider adding validation that balanceSlot is defined:
for (const token of deployedTokens) { if (token.balanceSlot === undefined) { throw new Error(`balanceSlot not defined for token ${token.symbol}`) } tokenIDs.push(token.tokenID) tokenAddresses.push(token.address) balanceSlots.push(ethers.utils.hexZeroPad(ethers.BigNumber.from(token.balanceSlot).toHexString(), 32)) scales.push(token.scale.toString()) }
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
go-ethereum(1 hunks)ops/l2-genesis/deploy-config/devnet-deploy-config.json(1 hunks)ops/l2-genesis/deploy-config/qanet-deploy-config.json(1 hunks)ops/l2-genesis/morph-chain-ops/genesis/config.go(2 hunks)ops/l2-genesis/morph-chain-ops/genesis/genesis.go(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- go-ethereum
- ops/l2-genesis/deploy-config/devnet-deploy-config.json
🧰 Additional context used
🧬 Code graph analysis (1)
ops/l2-genesis/morph-chain-ops/genesis/config.go (1)
ops/l2-genesis/morph-chain-ops/state/state.go (1)
StorageValues(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
ops/l2-genesis/deploy-config/qanet-deploy-config.json (1)
28-28: LGTM!The
emeraldTimefield addition is consistent with the existing time-based configuration fields and correctly positioned in the JSON structure.ops/l2-genesis/morph-chain-ops/genesis/genesis.go (2)
48-51: LGTM!The
emeraldTimevariable initialization follows the same pattern asmorph203TimeandviridianTime, ensuring consistency in handling fork activation times.
75-75: LGTM!The
EmeraldTimefield is correctly propagated into theChainConfig, consistent with the existing fork time handling.ops/l2-genesis/morph-chain-ops/genesis/config.go (1)
459-464: LGTM!The
L2TokenRegistrystorage configuration is correctly structured with appropriate initialization values and follows the same pattern as other predeploy contracts.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
contracts/contracts/l2/system/L2TokenRegistry.sol (2)
232-251: Silent skip may mask configuration errors.Line 239 silently skips unregistered tokens with
continue. Callers cannot distinguish between a successful batch operation and one where some entries were invalid. This was previously flagged in past reviews.As noted in the past review comment, consider emitting an event (e.g.,
TokenStatusUpdateSkipped(uint16 tokenID)) when an entry is skipped so off-chain systems can detect partial failures.
279-289: Silent skip may mask configuration errors.Lines 283-284 silently skip entries when
tokenAddress == address(0)or_prices[i] == 0. This matches the pattern flagged in past reviews forbatchUpdateTokenStatus.As noted in the past review comment for this function, consider emitting descriptive events (e.g.,
PriceUpdateSkippedInvalidToken(uint16 tokenID)andPriceUpdateSkippedZeroPrice(uint16 tokenID)) so callers can reconcile which updates succeeded and which were skipped.
🧹 Nitpick comments (3)
contracts/contracts/l2/system/L2TokenRegistry.sol (3)
62-65: Remove redundant initialization ofallowListEnabled.Line 64 sets
allowListEnabled = true, but this state variable already defaults totrueat line 33. The assignment is redundant and wastes gas.Apply this diff:
function initialize(address owner_) external initializer { _transferOwnership(owner_); - allowListEnabled = true; }
105-122: Add reentrancy guard for consistency.
registerToken(line 136) usesnonReentrant, butregisterTokensdoes not. Both eventually call_registerSingleToken, which makes an external call toIERC20Infos.decimals()(line 159). For consistency and defense-in-depth, apply the same protection to the batch function.Apply this diff:
function registerTokens( uint16[] memory _tokenIDs, address[] memory _tokenAddresses, bytes32[] memory _balanceSlots, uint256[] memory _scales - ) external onlyOwner { + ) external onlyOwner nonReentrant {
353-357: Consider validating input againstaddress(0).The function accepts
address(0)and returns0, but this edge case may confuse callers. Consider adding an explicit check at the start:if (tokenAddress == address(0)) revert InvalidTokenAddress();This makes the behavior more predictable and aligns with the validation pattern used elsewhere in the contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/bin/l2tokenregistry_deployed.hex(1 hunks)bindings/bindings/l2tokenregistry_more.go(1 hunks)contracts/contracts/l2/system/IL2TokenRegistry.sol(1 hunks)contracts/contracts/l2/system/L2TokenRegistry.sol(1 hunks)ops/l2-genesis/morph-chain-ops/genesis/config.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- bindings/bin/l2tokenregistry_deployed.hex
🚧 Files skipped from review as they are similar to previous changes (3)
- ops/l2-genesis/morph-chain-ops/genesis/config.go
- bindings/bindings/l2tokenregistry_more.go
- contracts/contracts/l2/system/IL2TokenRegistry.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
contracts/contracts/l2/system/L2TokenRegistry.sol (4)
317-336: LGTM!The calculation logic correctly implements the documented formula with proper ordering (multiplication before division) to preserve precision. The zero-result check at line 333 prevents unexpected behavior.
184-225: LGTM!The function correctly prevents address collisions across different token IDs (lines 198-199) and properly maintains both forward and reverse mappings when the address changes (lines 219-222). The use of
nonReentrantis appropriate given the external call todecimals().
403-404: LGTM!The storage gap of 50 slots follows OpenZeppelin conventions and addresses the upgrade safety concern raised in past reviews.
1-405: The original review comment is based on incorrect information.The interface
IL2TokenRegistrydoes not defineupdateFeeDiscountPercentorgetFeeDiscountPercentfunctions. These functions were never required by the interface, so their absence from the implementation is not an issue. The contract properly implements all functions declared in the interface.However, there is a minor code quality observation: the
InvalidPercent()error is declared in the interface but never used in the implementation, making it unused dead code.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
contracts/tasks/token_deploy.ts (2)
790-804: Consider simplifying the token selection logic.The while loop has multiple complex conditions and builds the
tokensToDeployarray. This logic could be clearer and more maintainable.Consider this refactored approach:
- while (deployedCount < tokensToDeployCount && currentTokenID <= maxTokenID && registeredTokenIDs.size + deployedCount < 100) { - if (!registeredTokenIDs.has(currentTokenID)) { - const tokenData = allTokensData.find((t: any) => t.tokenID === currentTokenID) - if (tokenData) { - tokensToDeploy.push({ - ...tokenData, - tokenID: currentTokenID, - scale: ethers.BigNumber.from(tokenData.scale), - priceRatio: ethers.BigNumber.from(tokenData.priceRatio) - }) - deployedCount++ - } - } - currentTokenID++ - } + for (const tokenData of allTokensData) { + if (deployedCount >= tokensToDeployCount) break + if (tokenData.tokenID > maxTokenID) continue + if (registeredTokenIDs.has(tokenData.tokenID)) continue + if (registeredTokenIDs.size + deployedCount >= 100) break + + tokensToDeploy.push({ + ...tokenData, + scale: ethers.BigNumber.from(tokenData.scale), + priceRatio: ethers.BigNumber.from(tokenData.priceRatio) + }) + deployedCount++ + }
685-1001: Consider refactoring this task into smaller helper functions.This task is 316 lines long, making it difficult to maintain and test. Consider extracting logical sections into helper functions.
For example:
async function loadAndValidateTokensConfig(filePath: string): Promise<TokenConfig[]> { // Lines 702-712 + validation } async function findAvailableTokenSlots(registry: Contract, maxSlots: number): Promise<Set<number>> { // Lines 737-751 } async function deployTokens(hre: any, tokensConfig: TokenConfig[]): Promise<DeployedToken[]> { // Lines 827-848 } async function registerTokensBatch(registry: Contract, tokens: DeployedToken[]): Promise<void> { // Lines 854-887 + fallback 970-1000 } async function setPricesBatch(registry: Contract, tokens: DeployedToken[]): Promise<void> { // Lines 894-931 }This would make the main task more readable and each function testable independently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/tasks/token_deploy.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: check
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
contracts/tasks/token_deploy.ts (2)
7-8: LGTM: Required imports for file operations.These imports are necessary for reading the
tokens.jsonconfiguration file in thedeploy-test-tokens-and-registertask.
863-868: Review comment is based on incorrect assumptions about the code.The code does not assume a hardcoded balance slot at position 0. Instead,
token.balanceSlotis read from configuration (tokens.json), where different tokens specify different slots (e.g., USDC/USDT/DAI use slot 0; T4-T100 use slot 3). This configuration-driven approach is actually more robust than the review suggests.However, the comments at lines 863-866 are misleading. They state "balance mapping is typically at slot 0" and "use slot 0 as the base slot," which contradicts the actual configuration showing mixed slot values. These comments should be clarified to accurately reflect that balanceSlot is per-token configuration, not a universal slot 0 assumption.
The actual risk is configuration mismatch if token configs get out of sync with actual contract storage layouts—not a hardcoded assumption that would break silently.
Likely an incorrect or invalid review comment.
| const upgradeTx = await proxyAdmin.upgradeAndCall( | ||
| taskArgs.proxy, | ||
| tokenRegistry.address, | ||
| TokenRegistryFactory.interface.encodeFunctionData('initialize', [ | ||
| taskArgs.owner // owner | ||
| ]) // data | ||
| ) |
There was a problem hiding this comment.
Critical: Calling initialize during upgrade will likely fail.
Using upgradeAndCall with initialize() will fail if the proxy is already initialized, as the initializer modifier prevents re-initialization. You should only call initialize() when deploying a new proxy, not when upgrading an existing one.
Apply this diff to fix the upgrade logic:
- // Upgrade the proxy to new implementation
- const upgradeTx = await proxyAdmin.upgradeAndCall(
- taskArgs.proxy,
- tokenRegistry.address,
- TokenRegistryFactory.interface.encodeFunctionData('initialize', [
- taskArgs.owner // owner
- ]) // data
- )
+ // Upgrade the proxy to new implementation (without calling initialize)
+ const upgradeTx = await proxyAdmin.upgrade(
+ taskArgs.proxy,
+ tokenRegistry.address
+ )If you need to initialize new storage variables, add a versioned initializer (e.g., initializeV2) to the contract and call that instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const upgradeTx = await proxyAdmin.upgradeAndCall( | |
| taskArgs.proxy, | |
| tokenRegistry.address, | |
| TokenRegistryFactory.interface.encodeFunctionData('initialize', [ | |
| taskArgs.owner // owner | |
| ]) // data | |
| ) | |
| // Upgrade the proxy to new implementation (without calling initialize) | |
| const upgradeTx = await proxyAdmin.upgrade( | |
| taskArgs.proxy, | |
| tokenRegistry.address | |
| ) |
🤖 Prompt for AI Agents
In contracts/tasks/token_deploy.ts around lines 647 to 653, the script calls
proxyAdmin.upgradeAndCall with TokenRegistry.initialize which will revert if the
proxy is already initialized; change the upgrade logic to either (A) use
proxyAdmin.upgrade (no call) when upgrading an existing proxy, or (B) if new
storage needs initialization, add a versioned initializer in the contract (e.g.,
initializeV2) and call upgradeAndCall with
TokenRegistryFactory.interface.encodeFunctionData('initializeV2', [/* params
*/]) instead; ensure the deployment flow distinguishes new proxy deployments
(call initialize) from upgrades (no initialize) and update the taskArgs/encoding
accordingly.
Summary by CodeRabbit
New Features
Tests
Chores