SimpleAccountFactory avoid extra address calculation #326
Conversation
Optimize SimpleAccountFactory, to try to create the account first, and calculate the counterfactual address only if it fails. The address calculation is only required in static call, and it is a waste to calculate it always on-chain just before "create2") (The downside is the requirement to use assembly to create the account, as we need the catch the creation failure without a revert)
drortirosh
force-pushed
the
SimpleAccount-factory-optim
branch
from
0a52309
to
367f96c
Compare
Slightly different impl with gnosis factory: - the low-level proxy factory always revert if already exists. - it is in a separate contract, so we can try/catch it
|
Actually i tried this one with different factory but seems the factory will consume every gas if create2 tries to deploy to existing address. I'm not sure saving gas for checking address is deployed or not is worth a risk also, since EXTCODESIZE has been called in entrypoint before the factory, address of userOp.sender is warm before entering the factory, gas we are saving is 100 |
| @@ -26,27 +27,35 @@ contract SimpleAccountFactory { | |||
| * This method returns an existing account address so that entryPoint.getSenderAddress() would work even after account creation | |||
| */ | |||
| function createAccount(address owner,uint256 salt) public returns (SimpleAccount ret) { | |||
| address addr = getAddress(owner, salt); | |||
| uint codeSize = addr.code.length; | |||
There was a problem hiding this comment.
the real gas cost saving here is not the addr.code.length (which indeed, is only 100, since the address will get warm), but the getAddress: there is no need to calculate the full create2 address (including bringing into memory the entire initCode and hash it)
Since we only need the address calculation if the account is not deployed, it makes sense to make it only after create2 marks that the account is already deployed (which can only happen if the method is used off-chain.
| @@ -5,6 +5,7 @@ import "@openzeppelin/contracts/utils/Create2.sol"; | |||
| import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; | |||
|
|
|||
| import "./SimpleAccount.sol"; | |||
| import "hardhat/console.sol"; | |||
There was a problem hiding this comment.
Please do not commit console.log code
| // must use assembly and not "new{salt}()", so it doesn't revert on failure. | ||
| // solhint-disable-next-line no-inline-assembly | ||
| assembly { | ||
| ret := create2(0, add(proxyConstructor, 0x20), mload(proxyConstructor), salt) |
There was a problem hiding this comment.
The EIP-1014 states that the creation throws immediately in case there is already a code at the destination address, why do you expect it to return address(0)?
There was a problem hiding this comment.
TL;DR: you're right. it should fail according to the spec, but all node implementations support it. I'm not hesitate to rely on it in our reference impl.
According to the spec, create2 should revert if target already exists.
but we have an explicit test - that shows this method works...
What I did find out that it works - but consumes more gas - estimating "create" takes ~280Kgas, but estimating again after create consumes >500Kgas (and 2M gas on hardhat)
Gas doesn't really matter, as it is meant for being a helper for off-chain method.
I think that what happens on failure is that the GO implementation doesn't revert, but consume all gas - but create behaves like a "call", and set aside 1/64 of the gas.
If that 1/64 gas left is enough, the method may succeed.
So if the view method is called with enough (e.g. default 5M gas), then the 1/64 excess that is left is enough to complete the createSender method....
solidity compiler is also aware if this behavior
let expr_19_address := create2(0, _2, sub(_3, _2), expr_18_salt)
if iszero(expr_19_address) { revert_forward_1() }
There was a problem hiding this comment.
This was the part where i was concerned about, for example, if there is a smart contract that relies on the entrypoint.getSenderAddress(), and do something afterwards that makes it non-view, then won't it be a easy honeypot to make end user to burn the eth gas? Yes it can be avoided by checking the gas limit and preview amount of gas spent but metamask does not show gas limit by default.
Maybe some easy prevention will be making it clear calling the entryPoint.getSenderAddress() in other contract is risky.
eg)
address factory;
...
function claimYourTokenToAA(bytes calldata initCode) external {
address wallet = entryPoint.getSenderAddress(abi.encodePacked(factory,initCode)); // this will cost around 580k as you said
token.transfer(wallet, 1e18);
}
There was a problem hiding this comment.
using initCode in an application seems a bit contrived (considering it requires entire constructor parameters).
It is expected to be filled by wallet software.
There was a problem hiding this comment.
Still, I'm a bit hesitant about including this as-is, as it depends on gas-left (and probably a misinterpretation of the "geth" code of the spec - though this implementation became the new standard)
| @@ -4,6 +4,7 @@ pragma solidity ^0.8.12; | |||
| import "@openzeppelin/contracts/utils/Create2.sol"; | |||
| import "@gnosis.pm/safe-contracts/contracts/proxies/GnosisSafeProxyFactory.sol"; | |||
| import "./EIP4337Manager.sol"; | |||
| import "hardhat/console.sol"; | |||
|
postpone this PR, as it depends on non-deterministic behaviour of create2 when target contract exists (documented as "revert" but implemented as "waste all gas") |
Optimize SimpleAccountFactory, to try to create the account first, and calculate the counterfactual address only if it fails.
The address calculation is only required in static call, and it is a waste to calculate it always on-chain just before "create2")
(The downside is the requirement to use assembly to create the account, as we need the catch the creation failure without a revert)