search

makerdao / dss-test

GNU Affero General Public License v3.0
11 stars 9 forks source link

`GodMode` import on `MCD.sol` causes issues on spells #40

Open amusingaxl opened 8 months ago

amusingaxl commented 8 months ago

This issue can be spotted in the wild throughout different spells:

2024-03-26: https://etherscan.io/address/0xcD672aCc9885796a19b4bAf03Dba46c8cdB0882B#code 2023-07-14: https://etherscan.io/address/0x402D46A20C849390Da96CeB0C3c04832D29e87d7#code 2023-05-01: https://etherscan.io/address/0x77107F74bf30250aFFada0fbD09fa517658B4916#code

The new-ish pattern of self-contained libs in spells for onboarding new modules usually import some contracts that were initially meant for testing purposes only.

This has an undesired side effect of causing the spell deployment to pull a lot of extraneous dependencies, that not only bloat the contract verification, but also inflate the contract deployed bytecode size.

While it's hard to compare directly, because different spells have different bytecode size, comparing the transactions that created the spell for 2024-03-26 against the one of 2024-03-08 shows us that the former costed around $4\,800\,761$ gas, while the latter costed $2\,228\,419$. That's a $115\%$ difference! Part of that can be explained by the imported but unused dependencies.

We should probably refactor MCD.sol and split it in 2 separate files, one meant to be used for testing and the other meant to be used in contracts that will actually be deployed.

SidestreamColdMelon commented 8 months ago

I'm actually not sure if it's strictly dss-test-related problem. Looking at the repository name, it might not be dss-test responsibility to provide reusable primitives useful outside of the tests. If a primitive is indeed useful, it should either be manually copied out and stripped from irrelevant functions and imports or another place should be found (like the exec lib)