feat(ethexe/contracts): use ERC-2612 for many actions with ERC20 token, add requestCodeValidationOnBehalf#5362
feat(ethexe/contracts): use ERC-2612 for many actions with ERC20 token, add requestCodeValidationOnBehalf#5362StackOverflowExcept1on wants to merge 11 commits intomasterfrom
Conversation
…n, add requestCodeValidationOnBehalf
StackOverflowExcept1on
added
A0-pleasereview
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
| // | ||
| // We might also need to update `ERC1967Utils.IMPLEMENTATION_SLOT` in all `Mirror`-s that are set to | ||
| // `isSmall = true` to the new `Mirror` address so that this method is visible on Etherscan. | ||
| function executableBalanceTopUpWithPermit(uint128 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) external { |
There was a problem hiding this comment.
need to this on-chain (migration)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements initial executable balance transfers during program creation and integrates EIP-2612 permit support for code validation and balance top-ups. It updates the Router and Mirror contracts, CLI, SDK, and Ethereum service layer to support these new flows and associated fees. Review feedback identifies a high-severity issue in the EIP-712 signature implementation for blob hashes where dynamic arrays must be hashed according to the standard to avoid signature mismatches. Additionally, it is recommended to remove try-catch blocks around permit calls to ensure that specific revert reasons are not masked by generic transfer failures.
|
@claude opus review |
|
Claude finished @StackOverflowExcept1on's task in 5m 14s —— View job Claude Opus ReviewSummary. The PR adds ERC-2612 permit flows across three call paths ( Disclosure up-front on a point already in the thread: the severity: high — EIP-712 encoding of File: bytes32 structHash = keccak256(
abi.encode(REQUEST_CODE_VALIDATION_ON_BEHALF_TYPEHASH, _requester, _codeId, _blobHashes, _deadline)
);Per EIP-712, dynamic arrays must contribute The POC test in Fix: bytes32 blobHashesHash = keccak256(abi.encodePacked(_blobHashes));
bytes32 structHash = keccak256(
abi.encode(REQUEST_CODE_VALIDATION_ON_BEHALF_TYPEHASH, _requester, _codeId, blobHashesHash, _deadline)
);And update the POC test to the same. Ideally add a second test that constructs the signature from an off-chain reference (e.g. alloy's severity: medium — File: The relayer (
If the intent is future-proofing for slashing / quota enforcement per requester, a comment explaining that would help reviewers. severity: medium — No governance setter for File: Fees are set to severity: medium — Signature replay window for File: When a code commitment is invalid, severity: low — File: for (uint256 i = 0; i < _blobHashes.length; i++) {
require(_blobHashes[i] == blobhash(i), ...);
}If the relayer attaches more blobs than severity: low — 120s permit deadline in SDK may be tight File: let deadline = AlloyU256::from(timestamp.checked_add(120).expect("infallible"));120s after the latest-block timestamp is fine on fast chains but can expire under L1 congestion or if the user signs then waits to broadcast. Bumping to 300–600s, or making it configurable on severity: low — Redundant File: require(initializer == address(0), InitializerAlreadySet());
require(!isSmall, IsSmallAlreadySet());The severity: low — Old File: The author already flagged this in the code comment and in the PR review ( Nits
Checklist
|
|
@claude opus review |
|
Claude finished @StackOverflowExcept1on's task in 4m 1s —— View job Claude Opus ReviewSummary. The "fix review" commit correctly addressed the earlier high-severity EIP-712 encoding issue (now uses severity: critical — File: function setRequestCodeValidationExtraFee(uint256 newExtraFee) external {
_router().protocolData.requestCodeValidationExtraFee = newExtraFee;
}Compare the sibling setter immediately above it (line 480): function setRequestCodeValidationBaseFee(uint256 newBaseFee) external onlyOwner {Anyone can call severity: medium — Signature for File: The EIP-712 typehash is: No The financial harm is bounded — whoever replays has to pay the mapping(address => uint256) public requesterNonces;
// ...
bytes32 structHash = keccak256(abi.encode(
REQUEST_CODE_VALIDATION_ON_BEHALF_TYPEHASH,
_requester, _codeId, keccak256(abi.encodePacked(_blobHashes)),
requesterNonces[_requester]++, _deadline
));severity: medium — File: emit CodeValidationRequested(_codeId);The relayer ( If severity: low — File: const TX_GAS_LIMIT_CAP: u64 = 16_777_216;
// ...
let gas_limit =
Self::TX_GAS_LIMIT_CAP.max(estimated_gas_limit + Self::GEAR_BLOCK_IS_PREDECESSOR_GAS);
severity: low — Redundant File: require(initializer == address(0), InitializerAlreadySet());
require(!isSmall, IsSmallAlreadySet());
severity: low — Permit deadline of 120s may be tight under congestion File: let deadline = AlloyU256::from(timestamp.checked_add(120).expect("infallible"));Fine on Anvil / fast chains; tight on mainnet if the signer waits to broadcast. Consider 300–600s, or making it configurable on Nits
Already handled by "fix review" commit (
|
This reverts commit ce1ac02.
2 participants
Resolves #5357
Resolves #4506