Conversation
|
Warning Rate limit exceeded@Kukoomomo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis PR refactors error semantics in the token registry interface (distinguishing tokenID vs address collisions), updates access control logic to explicitly handle owner and allow-list permissions, simplifies price calculation with ceiling division, and adds comprehensive test coverage for data consistency and batch operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Key areas requiring attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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: 0
🧹 Nitpick comments (2)
contracts/contracts/l2/system/L2TokenRegistry.sol (1)
306-308: priceRatio reset ensures data consistency but has operational implications.The unconditional reset of
priceRatioto 0 after anyupdateTokenInfocall ensures consistency, but note that even minor changes (like togglingisActive) will require a subsequentupdatePriceRatiocall. Consider documenting thatbatchUpdateTokenStatusshould be used for status-only changes to avoid the reset.contracts/contracts/test/L2TokenRegistry.t.sol (1)
237-237: Hardcoded storage slot may need updating if contract layout changes.The storage slot
151fortokenRegistrymapping is hardcoded in multiple storage verification tests (lines 237, 271, 297, 331). While this is acceptable for testing internal implementation, consider adding a comment noting that this value must be updated if the contract's storage layout changes, or using Foundry'sstdstoreutilities for more robust slot detection.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/contracts/l2/system/IL2TokenRegistry.sol(1 hunks)contracts/contracts/l2/system/L2TokenRegistry.sol(8 hunks)contracts/contracts/test/L2TokenRegistry.t.sol(7 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 (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
contracts/contracts/l2/system/IL2TokenRegistry.sol (1)
61-72: Improved error semantics for token registration.The new errors
TokenIDAlreadyRegistered()andTokenAddressAlreadyRegistered()provide better granularity compared to a genericTokenAlreadyRegistered()error, making it easier for callers to understand the exact cause of registration failures. TheZeroTokenAmount()error clearly communicates the edge case incalculateTokenAmount.contracts/contracts/l2/system/L2TokenRegistry.sol (5)
47-58: Access control logic is correct and well-documented.The refactored
onlyAllowedmodifier correctly implements the intended behavior:
- Owner always has access regardless of allowList status
- When
allowListEnabledis true, allowList users can access- When
allowListEnabledis false, only owner can accessThe logic
!isOwner && !isAllowedByListproperly handles all cases sinceisAllowedByListis false whenallowListEnabledis false.
224-225: Error semantics correctly distinguish ID vs address collisions.The checks properly use the new granular errors:
TokenIDAlreadyRegisteredwhen the tokenID already has a registered addressTokenAddressAlreadyRegisteredwhen the address is already mapped to another tokenID
275-280: Validation improvements in updateTokenInfo.Adding the non-zero scale check ensures consistency with token registration. The address collision check correctly allows keeping the same address while preventing collisions with other tokenIDs.
442-446: Ceiling division with ZeroTokenAmount error is correct.The ceiling division formula
(numerator + ratio - 1) / ratiois standard and correct. TheZeroTokenAmounterror appropriately signals that the calculation would result in zero tokens (which only happens whenethAmountis 0, since scale is validated to be non-zero).
494-496: Consistent priceRatio reset after scale update.Resetting
priceRatioto 0 when scale changes prevents inconsistent calculations, as the price formula depends on scale. This is consistent with theupdateTokenInfobehavior.contracts/contracts/test/L2TokenRegistry.t.sol (5)
106-122: Error expectations correctly updated for new error semantics.The tests properly distinguish between
TokenIDAlreadyRegistered(same ID, different address) andTokenAddressAlreadyRegistered(different ID, same address) scenarios.
406-415: Good test coverage for scale validation in updateTokenInfo.This test correctly verifies that
updateTokenInforejects zero scale values, maintaining consistency with the registration validation.
639-649: Correct test for ZeroTokenAmount error.This test verifies that passing
ethAmount = 0results in theZeroTokenAmount()revert, which is the expected behavior given the ceiling division formula.
655-861: Excellent comprehensive test coverage for access control.The
onlyAllowedmodifier tests cover all edge cases including:
- Owner access regardless of allowList status
- AllowList user access when enabled
- Reverts for non-allowList users
- Reverts when allowList is disabled (even for listed users)
- User removal from allowList
The truth table documentation at lines 805-815 is particularly helpful for understanding the expected behavior.
1184-1507: Thorough test coverage for priceRatio reset behavior.The test suite comprehensively covers:
- Reset behavior on various update operations
- Data consistency verification after scale changes
- Batch operation isolation (one token's reset doesn't affect others)
- Recovery workflow documentation
- Preservation of other token data during reset
The
test_data_consistency_after_scale_updatetest at lines 1264-1297 is particularly valuable for demonstrating that calculations remain consistent when both scale and priceRatio are updated appropriately.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.