search

morpho-org / pre-liquidation

Other
3 stars 1 forks source link

refactor: later requires in constructor, for readability #50

Closed adhusson closed 1 month ago

adhusson commented 2 months ago

Costs more gas on failed contract creations but much easier to read.

peyha commented 2 months ago

My initial implementation also had the require later in the constructor, no feeling on this one for me neither

colin-morpho commented 2 months ago

Same, I agree that names are shorter as we don't have to go through structs. On the other hand, it feels weird to compute (i.e. the constructor body) stuff that may fail just afterwards. What's your take on that @adhusson ?

colin-morpho commented 2 months ago

As a compromise I can only imagine using shorter names for the constructor parameters like _preLiquidationParams -> preLiqParams/pLP. But, I find these less readable actually.

adhusson commented 1 month ago

On the other hand, it feels weird to compute (i.e. the constructor body) stuff that may fail just afterwards. What's your take on that @adhusson ?

There's always a simulation first, and the parameters do not depend on things that will evolve between the simulation and the actual tx inclusion. So I think there will be ~no failed onchain deployments of this contract, the wasted gas does not matter, and we can focus on readability.

MathisGD commented 1 month ago

Still not a big fan tbh. If there is no majority for it, i would say we close it.