Skip to content
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

Require Statements Revert Custom Errors #13312

Closed
TradMod opened this issue Jul 28, 2022 · 4 comments
Closed

Require Statements Revert Custom Errors #13312

TradMod opened this issue Jul 28, 2022 · 4 comments

Comments

@TradMod
Copy link

TradMod commented Jul 28, 2022

FEATURE REQUEST

Abstract

The require are very helpful and easy to understand, but the problem with require statements is that they store a string as an error message. And strings are not much gas optimized. To make the contract gas optimized we use custom errors within if-else statements. Which are not quite sometimes easy to understand especially when they get rooted in.

Motivation

I think we can solve this problem by making Require statements revert Custom Error instead of string.
Check This:

pragma solidity ^0.8.0;

error Owner__ChorChorChor();

contract Owner {
    address public owner = msg.sender;

    function newOwner(address _newOwner) public OnlyOwner {
        owner = _newOwner;
    }

    modifier OnlyOwner { //Instead of this 
        require(owner == msg.sender, "Chor! Chor! Chor!");
        _;
    }
    modifier OnlyOwner { // this would be perfect
        require(owner == msg.sender, revert Owner__ChorChorChor(); );
        _;
    }
    
}

Specification

IDk C++, I wish I could make a PR with this functionality.

Backwards Compatibility

N/A

@cameel cameel moved this to Triage in Solidity Focus Board Jul 29, 2022
@TradMod TradMod closed this as not planned Won't fix, can't repro, duplicate, stale Aug 4, 2022
Repository owner moved this from Triage to Done in Solidity Focus Board Aug 4, 2022
@cameel
Copy link
Member

cameel commented Aug 5, 2022

Related: #7877 (comment)

We considered this but so far we could not agree on a good syntax.

@jubeira
Copy link

jubeira commented May 11, 2023

(I'm sorry to revive a closed thread, but...)

Not being able to use require with the new custom errors is indeed a barrier when porting code from older solc versions.
Denying the existing logic and using if / revert doesn't help readability IMO, and adds one more step which can be error prone when porting large codebases. I know you can opt out and keep using strings, but it's a pity to do so given the advantages of the new custom errors.

I don't know how hard it would be to implement or if there are any considerations that are being left out, but moving forward with a proposal like the one described in #7877 (comment) would be very much appreciated.

@basememara
Copy link

I would like this revisited as well please because I believe it is a safety issue. Most dev don't use the braces around the if-statement when using revert mimic the one-liner require syntax: if (owner != msg.sender) revert ChorChorChor()). This is dangerous if anything was learned from the notorious Apple SSL bug. The if-statement doesn't really guard like the require statement. It also makes you think backwards.. instead of "requiring" for a condition to be true to continue, you have to think opposite of require for the if-statement condition. This is very error prone for people who want to migrate from strings to custom errors. Not to mention goes against general coding styles to do the one-line if-statement.

Naturally I tried to do require(owner == msg.sender, ChorChorChor()). Is it possible to overload the require keyword? If not then revert(owner != msg.sender, ChorChorChor()) although that is too similar and have to think opposite require?

In any case, please reconsider due to the risks of allowing braces in an if-statement to be optional which is what most devs are resorting to which doesn't really guard the code after it like require does.

@0xClandestine
Copy link

require(condition, error)
require(condition, revert error)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

5 participants