Refactor input validation #45
Conversation
| @@ -60,18 +55,16 @@ contract Blue { | |||
| // Markets management. | |||
|
|
|||
| function createMarket(Market calldata market) external { | |||
| Id id = market.toId(); | |||
| Id id = Id.wrap(keccak256(abi.encode(market))); | |||
There was a problem hiding this comment.
Annoying that we have to do that, but this solution ensures that we cannot miss the input validation of id
There was a problem hiding this comment.
Did not see that my changes were already merged lol
There was a problem hiding this comment.
why not externalizing the logic? Keeping toId as it was and creating a specific function for the input check?
There was a problem hiding this comment.
This is to ensure that we cannot miss the input validation of id: when you externalize the logic then it's easy to use the wrong function and not do the require on the id
|
I don't like it, for the same arguments exposed here: #38 (comment) |
| @@ -60,18 +55,16 @@ contract Blue { | |||
| // Markets management. | |||
|
|
|||
| function createMarket(Market calldata market) external { | |||
| Id id = market.toId(); | |||
| Id id = Id.wrap(keccak256(abi.encode(market))); | |||
There was a problem hiding this comment.
Did not see that my changes were already merged lol
| @@ -60,18 +55,16 @@ contract Blue { | |||
| // Markets management. | |||
|
|
|||
| function createMarket(Market calldata market) external { | |||
| Id id = market.toId(); | |||
| Id id = Id.wrap(keccak256(abi.encode(market))); | |||
There was a problem hiding this comment.
why not externalizing the logic? Keeping toId as it was and creating a specific function for the input check?
|
|
||
| // Input validation | ||
|
|
||
| modifier nonZero(uint amount) { |
There was a problem hiding this comment.
I would put the modifier at the top of the contract instead
There was a problem hiding this comment.
The idea was to group the input validation functions, we want them to look for them easily
I don't think they apply here: the functions do things that are clearly defined. The The benefits are that we don't risk missing to validate the id, and that we ensure that the revert strings are the same for the same revert causes |
|
For the |
This kind of things are very easy to test and prove, that's why I prefer maximizing the readability (for the rest of the logic, that is less trivial). |
The concern about the current way it's tested is that we may forget to update them. But yes I agree, it's not a big deal, I just find it a bit more elegant to not have to repeat literal values like |
|
I prefer it like that. The nonZero amount is not really useful to me then though. |
QGarchery
requested review from
Rubilmax,
pakim249CAL,
makcandrov and
MerlinEgalite
|
Closing this because maybe it's not that useful to have only one input validation |
|
Sorry seems to late, but I think this is still an interesting idea, maybe we can reopen the PR later on? |
No description provided.