refactor: R\refactor authorization check in ERC1155 to reduce code duplication #6133
Conversation
|
WalkthroughThe changes refactor the ERC1155 contract to introduce an internal Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/token/ERC1155/ERC1155.sol(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5914
File: contracts/crosschain/bridges/BridgeERC20.sol:57-58
Timestamp: 2025-10-03T13:14:57.679Z
Learning: In cross-chain bridge contracts like BridgeERC20, when processing incoming messages in _processMessage, avoid validation checks that revert on malformed addresses. Reverting would create cross-chain inconsistency where tokens are locked/burned on the source chain but never minted on the destination. Instead, use best-effort address extraction (e.g., address(bytes20(toBinary))) to maintain atomicity across chains. If tokens are minted to an incorrect address due to user error, recovery may be possible through admin controls rather than leaving funds permanently locked.
⏰ 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: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
- GitHub Check: slither
- GitHub Check: tests-foundry
- GitHub Check: tests
- GitHub Check: halmos
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
🔇 Additional comments (2)
contracts/token/ERC1155/ERC1155.sol (2)
105-109: LGTM - Clean refactoring.The delegation to
_checkAuthorized(from)successfully eliminates code duplication while maintaining the same authorization semantics.
112-121: LGTM - Consistent refactoring.The change mirrors the refactoring in
safeTransferFrom, successfully consolidating the authorization check through the shared helper function.
| function _checkAuthorized(address from) internal view { | ||
| address sender = _msgSender(); | ||
| if (from != sender && !isApprovedForAll(from, sender)) { | ||
| revert ERC1155MissingApprovalForAll(sender, from); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider making the function virtual and add NatSpec documentation.
The refactoring successfully consolidates the authorization logic. However, consider two improvements:
-
Add the
virtualmodifier: OpenZeppelin contracts are designed for inheritance, and marking internal helper functions asvirtualallows derived contracts to customize authorization behavior if needed. -
Add NatSpec documentation: OpenZeppelin maintains comprehensive documentation standards. Consider adding a docstring like:
/**
* @dev Checks whether `_msgSender()` is authorized to transfer tokens from `from`.
*
* Requirements:
*
* - `_msgSender()` must be either `from` or approved for all transfers from `from`.
*/Apply this diff to add the virtual modifier:
- function _checkAuthorized(address from) internal view {
+ function _checkAuthorized(address from) internal view virtual {
address sender = _msgSender();
if (from != sender && !isApprovedForAll(from, sender)) {
revert ERC1155MissingApprovalForAll(sender, from);
}
}🤖 Prompt for AI Agents
In contracts/token/ERC1155/ERC1155.sol around lines 98 to 103, the internal
helper _checkAuthorized lacks NatSpec and is not overridable; add a NatSpec
docblock describing that it checks whether _msgSender() is authorized to
transfer tokens from `from` and the requirement that sender must be `from` or
approved for all, then add the `virtual` modifier to the function signature so
derived contracts can override the authorization behavior.
1 participant
This PR refactors the authorization check logic in
ERC1155.solto eliminate code duplication betweensafeTransferFromandsafeBatchTransferFromfunctions.PR Checklist
npx changeset add)