-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: later requires in constructor, for readability #50
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong feeling on this one
My initial implementation also had the require later in the constructor, no feeling on this one for me neither |
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 ? |
As a compromise I can only imagine using shorter names for the constructor parameters like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's more readable, although I'm generally against using immutable values in the contructor, as it's not sure if they are initialised yet or not. Also I like to have the require at the top of functions.
An other possibility would be to remove the struct (would be curious to see the code without it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried here I'm not fond of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you @adhusson.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally against using immutable values in the contructor, as it's not sure if they are initialised yet or not.
Are you referring to this part of the via_ir spec? It is a difficult area but I don't think it applies here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that you could do something like
require(ABC ...);
ABC = abc;
without noticing
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. |
src/PreLiquidation.sol
Outdated
require(IMorpho(morpho).market(id).lastUpdate != 0, ErrorsLib.NonexistentMarket()); | ||
MarketParams memory _marketParams = IMorpho(morpho).idToMarketParams(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require(IMorpho(morpho).market(id).lastUpdate != 0, ErrorsLib.NonexistentMarket()); | |
MarketParams memory _marketParams = IMorpho(morpho).idToMarketParams(id); | |
MORPHO = IMorpho(morpho); | |
require(MORPHO.market(id).lastUpdate != 0, ErrorsLib.NonexistentMarket()); | |
MarketParams memory _marketParams = MORPHO.idToMarketParams(id); |
Should we also do something like this then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more consistent, right?
Here's the take of ChatGPT on this matter
https://chatgpt.com/share/e/66fe5eb5-9364-800e-9d25-a2ff7dbc1bf6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm amazed by it's ability to handle pseudo code.
Still not a big fan tbh. If there is no majority for it, i would say we close it. |
Costs more gas on failed contract creations but much easier to read.