exploring gas of using oz #25
Conversation
| } | ||
|
|
||
| Clones.cloneDeterministic(implementation, salt); | ||
| account.initialize{value: msg.value}(owners); |
There was a problem hiding this comment.
msg.value kinda has unpredictable behavior here, in that if the account is already deployed, this will just sit on the factory. Maybe should just remove payable on this function
There was a problem hiding this comment.
What about returning the ETH if already existing? Or we could have a similar approach to Oz's one and simply revert if already deployed
|
before after Looks pretty good except 170k gas hit on upgrade 😬 |
|
Alchemy recently used Solady UUPS with OZ Clone, we could do the same https://github.com/alchemyplatform/modular-account |
src/CoinbaseSmartWallet.sol
Outdated
| @@ -209,6 +210,10 @@ contract CoinbaseSmartWallet is MultiOwnable, UUPSUpgradeable, Receiver, ERC1271 | |||
| } | |||
| } | |||
|
|
|||
| function upgradeToAndCall(address newImplementation, bytes memory data) public payable override { | |||
| ERC1967Utils.upgradeToAndCall(newImplementation, data); | |||
There was a problem hiding this comment.
this is not actually safe, but OZ is a little annoying to work with here. Doesn't have a clone1967
There was a problem hiding this comment.
For context, this override is unsafe (and not effective) but temporarily necessary to test gas.
OZ's Clones library deploys 1167 minimal proxies but our smart account implementation is a UUPSUpgradeable contract which, when calling upgradeToAndCall, uses the 1967 standard to check if the IMPLEMENTATION_SLOT is set to __self. This reverts as 1167 does not use IMPLEMENTATION_SLOT.
|
Interestingly if I had Solady UUPS back in the gas doesn't get much better, investigating. here
only think I can think is bytecode size change allowing for more optimizations? But I can't really think how that would actually be the case. |
|
Not pursuing for now |
No description provided.