EnsoWalletV2 #55
EnsoWalletV2 #55
Conversation
vnavascues
left a comment
vnavascues
left a comment
There was a problem hiding this comment.
Tiny changes and suggestions
|
|
||
| import { Initializable } from "openzeppelin-contracts/proxy/utils/Initializable.sol"; | ||
|
|
||
| contract EnsoWalletV2 is AbstractMultiSend, AbstractEnsoShortcuts, Initializable, Withdrawable { |
There was a problem hiding this comment.
It misses inheriting from IEnsoWalletV2, document the interface, and then NatSpec can just inherit from it.
| @@ -0,0 +1,91 @@ | |||
| // SPDX-License-Identifier: GPL-3.0-only | |||
| pragma solidity ^0.8.20; | |||
There was a problem hiding this comment.
pragma solidity ^0.8.28; (determined by AbstractMultiSend.sol.
| import { IERC721 } from "openzeppelin-contracts/token/ERC721/IERC721.sol"; | ||
| import { LibClone } from "solady/utils/LibClone.sol"; | ||
|
|
||
| contract EnsoWalletV2Factory { |
There was a problem hiding this comment.
I'd create the IEnsoWalletV2Factory interface where to locate the errors, events and methods
|
|
||
| address public immutable implementation; | ||
|
|
||
| event EnsoWalletV2Deployed(address wallet, address indexed account); |
There was a problem hiding this comment.
I'd index wallet too here just in case
| } | ||
| } | ||
|
|
||
| function _getSalt(address account) internal pure returns (bytes32) { |
There was a problem hiding this comment.
I'd replace internal with private
|
|
||
| bool success; | ||
| (success, response) = wallet.call{ value: msg.value }(data); | ||
| if (!success) { |
There was a problem hiding this comment.
if (response.length > 0) {
assembly ("memory-safe") {
revert(add(0x20, response), mload(response))
}
}
revert ExecutionFailed(); // NOTE: this custom error should be createdThis no only bubbles up any existing revert reason (like you did but adding memory-safe), but also forces a revert if there isn't one (custom error should be created). More about this:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L88
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/LowLevelCall.sol#L122
I reckon we should ideally update EnsoRouter with this change too
There was a problem hiding this comment.
Also, we should probably add this low-level call response handling to EnsoWalletV2.sol::execute (but not using assembly for the call, only to handle the error):
(bool success, bytes memory response) = target.call{value: msg.value}(data);
if (!success) {
if (response.length > 0) {
assembly ("memory-safe") {
revert(add(0x20, response), mload(response))
}
}
revert ExecutionFailed(); // NOTE: this custom error should be created on the interface
}
No description provided.