Custom errors or require?

[MerlinEgalite]

Custom errors or require?

[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

[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]

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

[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.

[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]

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?

[MerlinEgalite]

It's about the memory size no?

[MathisGD]

quick proof:

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

[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

[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]

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

[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

[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.

[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.

[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

[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

[MerlinEgalite]

Not me 🙈

[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

[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