♻️ Optimise `EIP-1167` (a.k.a. `0age` More-Minimal Proxy) - Githubissues

pcaversaccio / createx

Factory smart contract to make easier and safer usage of the `CREATE` and `CREATE2` EVM opcodes as well as of `CREATE3`-based (i.e. without an initcode factor) contract creations.

https://createx.rocks

GNU Affero General Public License v3.0

342 stars 26 forks source link

♻️ Optimise `EIP-1167` (a.k.a. `0age` More-Minimal Proxy) #35

Closed atarpara closed 11 months ago

atarpara commented 12 months ago

PR Checklist

[ ] Because this PR includes a bug fix, relevant tests have been included.
[ ] Because this PR includes a new feature, the change was previously discussed in an issue or in the discussions section.
[x] I didn't do anything of this.

Solady is using 0age pattern for the EIP-1167 which has 2 bytes less codesize and saves 4 gas on each call.

🐶 Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

pcaversaccio commented 12 months ago

@atarapra Thanks for the PR. The reason why I/we decided against 0age's version is that it's not the official EIP version. If, for whatever reason, someone relies on the codehash of the EIP-1167, it will fail. I would like to stick with the official version, even though it's not the fully optimised version to keep consistency with the official EIP. @mds1 what's your opinion on this?

atarpara commented 12 months ago

@pcaversaccio totally understood. I didn't understood foundry test is failing on ci but in my local machine passed all tests.

pcaversaccio commented 12 months ago

FWIW, I once made a Dune dashboard for minimal proxies over all available chains, and only a fraction uses 0age's version:

Thus, this is also line with our design principles that CreateX should cover most but not all contract creation use cases. But lets wait for @mds1's feedback :).

pcaversaccio commented 12 months ago

@pcaversaccio totally understood. I didn't understood foundry test is failing on ci but in my local machine passed all tests.

That's a fuzz edge case unrelated to your changes - I will make a PR to fix this.

pcaversaccio commented 12 months ago

@atarapra this PR (https://github.com/pcaversaccio/createx/pull/36) will fix the previously discovered edge case.

pcaversaccio commented 12 months ago

Hmm, seems the coverage report doesn't work for external forks due to some permission issues; will take a look.

pcaversaccio commented 12 months ago

Hmm, seems the coverage report doesn't work for external forks due to some permission issues; will take a look.

This will fix the coverage issues for external forks: https://github.com/pcaversaccio/createx/pull/37.

mds1 commented 11 months ago

Thanks for the PR @atarpara!

I agree with @pcaversaccio that for this factory we should stick to the widely supported standard. Even if it's not the most efficient, it's the most well-supported, and the least-surprising to users deploying minimal proxies. Users that want the 0age version can use CreateX to deploy their deterministically deploy their factory across chains. As a result I'm going to close to this :)