-
Notifications
You must be signed in to change notification settings - Fork 34
Expose additional interface id in wrapper #267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdded a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5–10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol (1)
66-69: AlignsupportsInterfacewith theERC7984Rwapattern and fix NatSpec.The
supportsInterfaceimplementation should follow the established pattern inERC7984Rwaand the baseERC7984contract:
NatSpec correction (line 66): Change
/// inheritdoc IERC165to/// @inheritdoc ERC165(missing@prefix and incorrect interface reference).Pattern consistency (line 68): To expose a wrapper-specific ERC-165 interface ID idiomatically, introduce an
IERC7984ERC20Wrapperinterface describing the wrapper's public API, have this contract implement it, and usetype(IERC7984ERC20Wrapper).interfaceIdinstead. This mirrors the approach used inERC7984RwawithIERC7984Rwaand makes the intent explicit.Once
IERC7984ERC20Wrapperexists:- /// inheritdoc IERC165 + /// @inheritdoc ERC165 function supportsInterface(bytes4 interfaceId) public view virtual override(ERC7984) returns (bool) { - return interfaceId == type(ERC7984ERC20Wrapper).interfaceId || super.supportsInterface(interfaceId); + return interfaceId == type(IERC7984ERC20Wrapper).interfaceId || super.supportsInterface(interfaceId); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.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: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: coverage
- GitHub Check: slither
- GitHub Check: tests
| function unwrap(address from, address to, externalEuint64 encryptedAmount, bytes calldata inputProof) external; | ||
|
|
||
| /// @dev Fills an unwrap request for a given cipher-text `burntAmount` with the `cleartextAmount` and `decryptionProof`. | ||
| function finalizeUnwrap(euint64 burntAmount, uint64 burntAmountCleartext, bytes calldata decryptionProof) external; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @arr00 , what is the rationale behind not including finalizeUnwrap in the interface ? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to clarify the context for @arr00 we need to know if the interface of the wrapper changed, because if it did, then we will need to do an upgrade to the 6 already deployed wrappers on mainnet (not a big issue for now, but better know asap before registering more wrappers, actually we already stumbled on few other issues which will require an upgrade soon, so this might also be included in same future upgrade).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The finalizeUnwrap function we use is certainly implementation specific—the goal is to allow all confidential wrappers of ERC-20 use this interface. I could imagine many wrappers that would prefer to have different parameters in their finalize unwrap function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I understand your point. We will then remove it from our upgradeable wrapper extension ( https://github.com/zama-ai/fhevm/blob/main/protocol-contracts/confidential-wrapper/contracts/interfaces/IERC7984ERC20Wrapper.sol#L22 ) , and upgrade contracts already deployed on mainnet soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that makes sense yes, too bad we cannot highlight the 2-step nature of the unwrap process in the interface but I indeed don't seel alternatives, we'll update ours as well later then yes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.