Custom errors or require?

[MerlinEgalite]

No description provided.

[MathisGD]

I personally prefer require because with them you write invariants whereas with if () revert you write what you don't want to see, which is a little bit less natural to me.

[QGarchery]

If I remember correctly, the main downside of requires was bytecode side. That won't be a problem here

[Rubilmax]

Custom errors allow to have debug variables (also useful in production):

error Unauthorized(uint256 var);

[MerlinEgalite]

Agree, but we never used it yet, and it would be replaced by offchain tool as well (tenderly, phalcon, etc.)

[MerlinEgalite]

New poll haha?

[MathisGD]

Except @Rubilmax's point, which I think is fair but not mandatory, there is nothing in favor of custom errors. Except if new points arise we can conclude in favor of requires.

[pakim249CAL]

I think custom errors are useful for other reasons as well. They allow us to standardize the errors and reuse them throughout the code, rather than having to synchronize the same error messages across different contexts. This makes writing and maintaining tests throughout code changes easier. Also, bytecode size does impact the gas cost of calling the contract. In particular, 3 extra gas is used per byte to call the contract.

[MerlinEgalite]

They allow us to standardize the errors and reuse them throughout the code, rather than having to synchronize the same error messages across different contexts.

You could do the same using constants no?

Also, bytecode size does impact the gas cost of calling the contract. In particular, 3 extra gas is used per byte to call the contract.

Really? Do you have the source please?

[MathisGD]

Also, bytecode size does impact the gas cost of calling the contract. In particular, 3 extra gas is used per byte to call the contract.

I don't think so (https://www.evm.codes/#f1?fork=shanghai)

[pakim249CAL]

Also, bytecode size does impact the gas cost of calling the contract. In particular, 3 extra gas is used per byte to call the contract.

I don't think so (https://www.evm.codes/#f1?fork=shanghai)

If you dive into the footnotes of call, there is an additional dynamic cost.

[MerlinEgalite]

It's about the memory size no?

[pakim249CAL]

It's about the memory size no?

When calling a contract, the code must be copied. (Otherwise, how can you run the code?)

[MathisGD]

I'm pretty sure that the code of the called contract is not copied in the EVM memory (otherwise you would not even need a contract size limit). So the cost of calling a big contract is the same than calling a small contract.

[MerlinEgalite]

Agree with @MathisGD

[MathisGD]

quick proof:

• https://gist.github.com/MathisGD/eee8c4c2e8783b3a49bf4daf99c19b46
• https://gist.github.com/MathisGD/af514c4d707117c4e6df0ee1f632d90b

[QGarchery]

quick proof:

• https://gist.github.com/MathisGD/eee8c4c2e8783b3a49bf4daf99c19b46
• https://gist.github.com/MathisGD/af514c4d707117c4e6df0ee1f632d90b

Nice ! As a side note, this is a good use case for morpho-sandbox, where I can reproduce and tweak your code by just doing git checkout. For example, I might want to check the contracts size here

[MathisGD]

Good point, I'll open a PR (but FYI I checked that the long contract's bytecode is longer)

https://github.com/morpho-labs/morpho-sandbox/pull/10

[pakim249CAL]

Okay, yes I see I'm wrong on the gas cost point. I guess do whatever you like then, though I still think it's strange that we would choose to use string errors over a solidity feature that is becoming widely used and integrated across many tools. Using strings means that every contract that wants to handle the error has to adapt to matching the string rather than using the error hash if they want to catch the error. And the error would not appear in the ABI.

[MerlinEgalite]

That's a good point actually

[MathisGD]

The best thing would be require with custom error ? PR in solidity 0.8.21 ?

[MerlinEgalite]

Not me 🙈

[QGarchery]

Ok so it comes down to:

1. require condition feeling more natural than revert condition. Do we want to write the happy case of the revert case ?
   Happy case for me.
2. bytecode size.
   It should not matter for blue.
3. ensuring that revert messages are consistent at compilation.
   It's a nice benefit but not extremely important IMO
4. using the one that is getting the most support.
   I don't know enough to say, but it seems like an important point
5. custom errors cost less gas than requires
   If I remember well it was not by much

[MerlinEgalite]

There's also a gas discussion. Custom errors being cheaper than requires.

[Rubilmax]

1., 2., and gas diff are not important to me.

3. can have an impact on integrators: if it's easy for them to have error messages, it's easier for them to integrate us (debug their txs).

4. is the most important one to me

On this topic (4.), try/catching in solidity does not support custom errors for now: it only supports string/bytes. It's a syntax thing only, but it counts to me.

I also don't want the decision we're taking now influence the choice of dependencies (such as Ownable, Owned, EIP712, etc): I don't think we should select a dependency based on whether they throw custom errors or reason strings (because I don't value this debate much, nor its impact).

I'm ok to move on with require strings or custom errors, as I don't have a strong opinion either.

[QGarchery]

3. can have an impact on integrators: if it's easy for them to have error messages, it's easier for them to integrate us (debug their txs).

When I wrote point 3 I had in mind that it prevents typos, so I don't see how it's related to making it easier for integrators to have error messages. You are talking about point 4 right ?

There's also a gas discussion. Custom errors being cheaper than requires.

Thanks I'll edit my message so that it is exhaustive

[Rubilmax]

When I wrote point 3 I had in mind that it prevents typos, so I don't see how it's related to making it easier for integrators to have error messages. You are talking about point 4 right ?

This is indeed a nice benefit of having errors extracted to a single place. Let's forget about my point because indeed, integrators debugging an integration would either have the reason string or the custom error, each being enough to debug.
Also, 3. can be achieved with reason strings as well as custom errors (it's only about defining them in a single place)?

[MerlinEgalite]

I think the difference it that custom errors are added to the ABI

[MerlinEgalite]

On this topic (4.), try/catching in solidity does not support custom errors for now: it only supports string/bytes. It's a syntax thing only, but it counts to me.

Just for that I think I'd go for the require with strings as @Rubilmax proposed in #126 unless solc 0.8.21 fixes that

[julien-devatom]

The problem is that the revert adds nothing to the transaction payload and fills the error message with "execution reverted". We have to go through the whole call trace to retrieve the error.

require uses the error message and fills it with the message written in the code. The error is therefore raised to the highest level and simplifies the reading of the transaction.

That's why etherscan is always displaying "execution error" on morpho optimizers errors

[MerlinEgalite]

@julien-devatom we're going toward the require for your information