♻️ Move `event`s, `error`s, and `struct`s Into the Interface `ICreateX`

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

♻️ Move `event`s, `error`s, and `struct`s Into the Interface `ICreateX` #38

Closed QYuQianchen closed 11 months ago

QYuQianchen commented 11 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.

In the CreateX.sol and ICreateX.sol files, events, errors, and the Values struct are currently defined separately. While a diff check of ABIs is part of the CI workflow as seen in this PR, explicitly inheriting the interface or defining ICreateX.sol as an abstract contract within the CreateXcontract would ensure alignment and eliminate any discrepancies.

🐶 Cute Animal Picture

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

pcaversaccio commented 11 months ago

@QYuQianchen thanks for this PR and your work. I would like to share two comments:

We provide the Solidity-based interface ICreateX for builders that want to build on top of CreateX and not for the core contract CreateX itself. The core contract CreateX should be a non-inheritance-based contract to follow the KISS principle.
Inheritance makes block explorer verification more annoying. We will provide presigned deployment transactions that other people can broadcast, and they should verify the CreateX contract accordingly. Inheritance means that simple "copy + paste" doesn't work for verification, but people need a flattened file or the standard json input. Thus an additional step of complexity, which we should prevent.

For these reasons, I suggest not merging this PR. However, I will wait for @mds1's feedback.

mds1 commented 11 months ago

Thanks @QYuQianchen! I do agree with @pcaversaccio here—for Multicall3 having a flat file with no inheritance has made management and verification a lot simpler and less error prone, so I think it would be beneficial keep the same approach here. The CI check ensures the two are always in sync which is the main risk of keeping them separate

QYuQianchen commented 11 months ago

I see. Depending on the tooling and the block explorer, verification indeed can become cumbersome. Appreciate the detailed explanation on the design decision!