update FeeVault #80
update FeeVault #80
Conversation
…yment configuration
|
Claude finished @tac0turtle's task —— View job Code Review CompleteReviewing the FeeVault constructor refactoring changes:
Overall AssessmentThis PR successfully refactors the FeeVault deployment from a multi-step configuration process to a single-transaction atomic deployment. The changes are well-structured and maintain backward compatibility for administrative functions. ✅ Strengths
🚨 Critical Issues
🔧 Recommendations
// After deployment, verify address matches prediction
require(address(feeVault) == computedAddress, "FeeVault: address mismatch");
// Add validation for otherRecipient if bridgeShareBps < 10000
if (_bridgeShareBps < 10000) {
require(_otherRecipient != address(0), "FeeVault: other recipient required");
}
🔍 Minor Issues
🎯 Security & Performance
🧪 Test CoverageExcellent test coverage including:
The refactoring significantly improves the deployment experience while maintaining security and functionality. Address the critical CREATE2 verification issue and consider the validation consistency improvements before merging. |
Summary of ChangesHello @tac0turtle, 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 significantly refactors the deployment and initialization of the Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 refactors the FeeVault contract to accept configuration parameters in its constructor, which simplifies the deployment process. The deployment script and tests have been updated to reflect this change. My review focuses on the correctness and security of the new deployment method. I have found two high-severity issues that should be addressed:
- The deployment script in
DeployFeeVault.s.solno longer verifies the pre-computed CREATE2 address. This is a critical safety check for deterministic deployments. - The
FeeVaultconstructor inFeeVault.soldoes not validate that the owner address is non-zero, which could result in deploying an ownerless contract with locked administrative functions.
I have provided specific code suggestions to resolve these issues.
| vm.startBroadcast(); | ||
|
|
||
| // Deploy FeeVault with CREATE2 | ||
| FeeVault feeVault = new FeeVault{salt: salt}(owner); | ||
| console.log("FeeVault deployed at:", address(feeVault)); | ||
| require(address(feeVault) == predicted, "Address mismatch"); | ||
|
|
||
| // Configure if values provided | ||
| if (destinationDomain != 0 && recipientAddress != bytes32(0)) { | ||
| feeVault.setRecipient(destinationDomain, recipientAddress); | ||
| console.log("Recipient set - domain:", destinationDomain); | ||
| } | ||
|
|
||
| if (minimumAmount > 0) { | ||
| feeVault.setMinimumAmount(minimumAmount); | ||
| console.log("Minimum amount set:", minimumAmount); | ||
| } | ||
|
|
||
| if (callFee > 0) { | ||
| feeVault.setCallFee(callFee); | ||
| console.log("Call fee set:", callFee); | ||
| } | ||
|
|
||
| if (bridgeShareBps != 10000) { | ||
| feeVault.setBridgeShare(bridgeShareBps); | ||
| console.log("Bridge share set:", bridgeShareBps, "bps"); | ||
| } | ||
|
|
||
| if (otherRecipient != address(0)) { | ||
| feeVault.setOtherRecipient(otherRecipient); | ||
| console.log("Other recipient set:", otherRecipient); | ||
| } | ||
| FeeVault feeVault = new FeeVault{salt: salt}( | ||
| owner, destinationDomain, recipientAddress, minimumAmount, callFee, bridgeShareBps, otherRecipient | ||
| ); | ||
|
|
||
| vm.stopBroadcast(); | ||
|
|
||
| console.log("FeeVault deployed at:", address(feeVault)); | ||
| console.log("Owner:", owner); | ||
| console.log("Destination domain:", destinationDomain); | ||
| console.log("Minimum amount:", minimumAmount); | ||
| console.log("Call fee:", callFee); |
There was a problem hiding this comment.
The CREATE2 address prediction and verification check has been removed. This is a crucial safety measure to ensure the contract is deployed at the expected address, protecting against unintended changes in bytecode, constructor arguments, or salt. It's highly recommended to re-add this verification within the deployment script.
// Compute address before deployment
bytes32 initCodeHash = keccak256(
abi.encodePacked(
type(FeeVault).creationCode,
abi.encode(
owner,
destinationDomain,
recipientAddress,
minimumAmount,
callFee,
bridgeShareBps,
otherRecipient
)
)
);
address predicted = address(
uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, initCodeHash))))
);
console.log("Predicted FeeVault address:", predicted);
vm.startBroadcast();
// Deploy FeeVault with CREATE2
FeeVault feeVault = new FeeVault{salt: salt}(
owner,
destinationDomain,
recipientAddress,
minimumAmount,
callFee,
bridgeShareBps,
otherRecipient
);
require(address(feeVault) == predicted, "FeeVault: address mismatch");
vm.stopBroadcast();
| recipient, | ||
| minAmount, | ||
| fee, | ||
| 10000, // 100% bridge share |
tac0turtle
force-pushed
the
marko/revamp_constructor
branch
from
6fb2393 to
688323a
Compare
tac0turtle
force-pushed
the
marko/revamp_constructor
branch
from
201f06b to
beccaa2
Compare
constructor to accept additional parameters for deployment configuration
Description
Type of Change
Related Issues
Fixes #(issue)
Checklist
Testing
Additional Notes