ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.11k stars 5.73k forks source link

Response Status Codes via require(expression, errorString, optStatusCode) #12965

Closed QuintonC closed 1 year ago

QuintonC commented 2 years ago

Abstract

Hi!

First and foremost, thanks for taking the time to consider anything that I might outline below.

I'd like to consider, present, and question the addition of a standardized list of response codes that can be returned as an exception within an assertion or requirement (assert or require). I'm not proposing that the status codes be the same status codes as the ones that we see with HTTP responses, but that we implement a similar behavior.

During the creation of my first minting front-end experience, I found myself first looking into ethers to see if this was something that would be documented on their end (handling status responses from the contracts, despite what the contract type might be).

What I then quickly realized was that this wouldn't be a core functionality of an interface to a smart contract, but perhaps something that would rather be a fundamental, core piece of the language involved in writing contracts (therefore being contract-type independent).

While I understand that this could be a breaking change, I think we can consider the possibility of making the parameter optional until a future stable release, allowing individuals who are writing the contract to experiment with the addition.

Motivation

During the creation of a minting site, I was interfacing with our smart contract and looking for specific error response codes. While this doesn't necessarily solve the problem for the front-end, as it would require additional work from the team over at ethers, it certainly is a step in the right direction.

Consider the following snippet of code:

interface MintResponseType {
  mintSuccessful: boolean;
  message: string;
  payload: any;
}

const mintResponseErrorHandling = (err: any): MintResponseType => {
  ...
  } else if (err.message.includes('Sale is not active')) {
    return {
      mintSuccessful: false,
      message:
        'Mint is not active. Please come back once we begin our minting process.',
      payload: err,
    };
  } else if (err.message.includes('Invalid signature')) {
    return {
      mintSuccessful: false,
      message: 'Mint failed due to an invalid signature. Please try again.',
      payload: err,
    };
  }
  ...
};

Now let's consider it with the inclusion of standardized response status codes: Consider the following snippet of code:

interface MintResponseType {
  mintSuccessful: boolean;
  message: string; // could additionally return the message returned from the require Error(string)
  payload: EthersSolidityError;
}

const mintResponseErrorHandling = ({errorString, status}: EthersSolidityError): MintResponseType => {
  ...
  } else if (status.code === 500) {
    return {
      mintSuccessful: false,
      message:
        'Mint is not active. Please come back once we begin our minting process.',
      payload: err,
    };
  } else if (status.code === 501) {
    return {
      mintSuccessful: false,
      message: 'Mint failed due to an invalid signature. Please try again.',
      payload: err,
    };
  }
  ...
};

Specification

The current markup is require(expression, Error(string)), I believe that we could add an optional parameter to the end, called it statusCode.

The resulting markup would be require(expression, Error(string) , statusCode?)

Backwards Compatibility

This should result in a completely backwards compatible integration given that the statusCode could be an optional, standardized number (e.g. 200 (success), 410 (insufficient funds, etc)).

Additional Information / Concerns

While this would result in the potential need for a community-driven vote on which status codes represent what, and could lead to a long implementation time, I think it is a great future for the future of the language and for the future of best-practice contract negotiations between contract and front-end.

This would certainly lead to some additional work from other packages (ethers and moralis to name a couple), it certainly seems like a great forward direction.

Edits

Any time that I edit this issue, I will post a TL;DR or bulleted list of edits here.

Thanks again for reading ✌️

chriseth commented 2 years ago

This looks incompatible to the user-defined error feature. Why do you think the user-defined error feature cannot solve the same problem?

cameel commented 2 years ago

Yeah, your examples seem to be assuming that the client is differentiating errors by their message but this is not necessary at all with the new custom error feature. Currently the compiler allows you to do this:

error SaleIsNotActive();
error InvalidSignature();

...

contract C {
    function doStuff() public {
        ...
        if (signatureNotValid)
            revert InvalidSignature();
        ...
    }
}

And on the client side you can just compare the error signature instead of relying on the string.

You could even implement some custom scheme of error codes that way if that's what you prefer:

error MintError(string message, uint code);

...

        if (signatureNotValid)
            revert MintError("Mint failed due to an invalid signature. Please try again.", 501);

...

I'm not sure if a standard error code scheme would actually be viable. In different domains you'll have very different reasons for reverts. Maybe you could gather some common reasons publish a community-maintained list of recommended error signatures but it will be far from complete. Any project will still need some specific error codes for their own needs.

cameel commented 2 years ago

By the way, here's an earlier feature request for a feature that would allow defining error codes: #9813. Not the same thing but somewhat related.

QuintonC commented 2 years ago

This looks incompatible to the user-defined error feature. Why do you think the user-defined error feature cannot solve the same problem?

Could you provide some more context about what makes this incompatible? I'm looking to add a feature to the core language. Sure, it's not compatible in its current state, this is an addition the core language in the hopes of standardizing errors that are thrown from contracts when conditions are not met, while allowing users to still return a user-defined string that can be shown on the front end.

QuintonC commented 2 years ago

Yeah, your examples seem to be assuming that the client is differentiating errors by their message but this is not necessary at all with the new custom error feature. Currently the compiler allows you to do this:

I think perhaps my code samples under Motivation have thrown people off as those are front-end samples.

I'm reading a bit more through what is available in the docs to try and understand your response as well (I'm a front-end developer and new to working with Solidity contracts)

However, let's consider the following two samples with my suggestion:

Original Code Sample

    /**
     * @notice Mint the specified number of token using a signature
     * @param amount Of tokens to mint
     * @param signature Ethereum signed message of transaction sender's address, created by signer
     */
    function mint(uint256 amount, bytes memory signature) public virtual payable nonReentrant {
        require(saleActive, "Sale is not active");
        require(_totalMinted() + amount <= MAX_SUPPLY, "Insufficient supply");
        require(msg.value == mintPrice * amount, "Invalid Ether amount sent");
        require(addressMinted[_msgSender()] + amount <= maxPerAddress, "Insufficient mints available");

        require(signer == ECDSA.recover(
            ECDSA.toEthSignedMessageHash(keccak256(abi.encodePacked(_msgSender()))),
            signature
        ), "Invalid signature");

        _safeMint(_msgSender(), amount);
        addressMinted[_msgSender()] += amount;
    }

Suggested Code Sample

    /**
     * @notice Mint the specified number of token using a signature
     * @param amount Of tokens to mint
     * @param signature Ethereum signed message of transaction sender's address, created by signer
     */
    function mint(uint256 amount, bytes memory signature) public virtual payable nonReentrant {
        require(saleActive, "Sale is not active", 1000);
        require(_totalMinted() + amount <= MAX_SUPPLY, "Insufficient supply", 1200);
        require(msg.value == mintPrice * amount, "Invalid Ether amount sent", 1404);
        require(addressMinted[_msgSender()] + amount <= maxPerAddress, "Insufficient mints available", 1501);

        require(signer == ECDSA.recover(
            ECDSA.toEthSignedMessageHash(keccak256(abi.encodePacked(_msgSender()))),
            signature
        ), "Invalid signature", 1505);

        _safeMint(_msgSender(), amount);
        addressMinted[_msgSender()] += amount;
    }

To add to the two samples above, the numbers, (1000, 1200, 1404, 1501, 1505) are simply just random numbers for the sake of throwing together a quick sample.

Using the custom error feature you might have something such as this instead (pulled from docs).

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.4;

contract TestToken {
    error InsufficientBalance(uint256 available, uint256 required, StatusCode errorCode);
    function transfer(address /*to*/, uint amount) public pure {
        revert InsufficientBalance(0, amount, 1300);
    }
}

Notes

cameel commented 2 years ago

Contracts currently only return a string

That's not really true. See the section on revert.

Your example would look like this with custom errors:

    error SaleNotActive();
    error InsufficientSupply();
    error InvalidEtherAmount();
    error InsufficientMints();
    error InvalidSignature();

    /**
     * @notice Mint the specified number of token using a signature
     * @param amount Of tokens to mint
     * @param signature Ethereum signed message of transaction sender's address, created by signer
     */
    function mint(uint256 amount, bytes memory signature) public virtual payable nonReentrant {
        if (!saleActive) revert SaleNotActive();
        if (_totalMinted() + amount > MAX_SUPPLY) revert InsufficientSupply();
        if (msg.value != mintPrice * amount) revert InvalidEtherAmount();
        if (addressMinted[_msgSender()] + amount > maxPerAddress) revert InsufficientMints();

        if (signer != ECDSA.recover(
            ECDSA.toEthSignedMessageHash(keccak256(abi.encodePacked(_msgSender()))),
            signature
        )
            revert InvalidSignature();

        _safeMint(_msgSender(), amount);
        addressMinted[_msgSender()] += amount;
    }

And this is already supported. No new feature needed.

On the client side the error does not have to be processed as a random string. The docs on revert show how you can decode the return data to get the selector, which you could basically treat as an error code.

QuintonC commented 2 years ago

On the client side the error does not have to be processed as a random string. The docs on revert show how you can decode the return data to get the selector, which you could basically treat as an error code.

Perhaps there is a disconnect between solidity and ethers then? The approach I'm suggesting would rule out any inconsistencies between all contracts in the future in terms of error handling, while also increasing/stabilizing the ability to control, manage, and detect errors on the front end while giving users more insight as to why their transaction might have failed.

Given your sample above, what would the return look like through the ethers.js? What is the payload? How would the front-end depict this / case through different errors and return relevant error information depending on what the response is/was?

The docs on revert show how you can decode the return data to get the selector, which you could basically treat as an error code.

I read through the docs but am failing to find any clear-cut information that would rule out any inconsistencies across different contracts in the future.

Maybe I can ask you the theoretical question of why HTTP status codes were ever created and what your opinion on them is? Are they pointless because a back-end developer could instead throw an error with a string? I think the answer is no, but I'm happy to hear you out on what you might have to say.

I think this is a fundamental core capability that should be added to prevent inconsistencies going into the future, but perhaps my previous web2 experience is taking control of my thought process here?

Edit

If I'm understanding you properly, then perhaps this isn't an issue of adding functionality to the language itself, but perhaps a best practice rule that should be abided by?

Is there a style guide that points users to the best method for handling errors? If not, maybe that is something that should be added. Given the documentation, it looks like there are multiple ways of handling errors, why is that? Why not one, clear, concise method to handle errors (the method you've suggested), to enable front-end developers the ability to better understand errors that are thrown by contract transactions?

cameel commented 2 years ago

Given your sample above, what would the return look like through the ethers.js? What is the payload?

Sorry, I'm not intimately familiar with how specific libraries handle custom errors. What I can tell you is that this is how returndata looks like for Error(string):

0x08c379a0                                                         // Function selector for Error(string)
0x0000000000000000000000000000000000000000000000000000000000000020 // Data offset
0x000000000000000000000000000000000000000000000000000000000000001a // String length
0x4e6f7420656e6f7567682045746865722070726f76696465642e000000000000 // String data

For other errors it will be similar, according the ABI encoding rules. The selector part is what the library can present to you as an "error code". It's a hash of the error signature so it can also be matched against specific error type to see if it was that error. Not sure how ethers.js makes this information available. If it doesn't then perhaps a feature request in that project would be a good idea.

I read through the docs but am failing to find any clear-cut information that would rule out any inconsistencies across different contracts in the future.

I don't really think this is a language-level concern. Having a convention would perhaps be nice but I'm a bit skeptical about the possibility to come up with a universal convention that will fit all contracts ever.

The HTTP error codes are not directly comparable in my opinion. They really deal with transport issues and the REST model, not with actual business logic of each web application. For example the equivalent of HTTP 200 with contracts is just a lack of revert. For HTTP 4xx we have Error() and custom errors. Panic() codes are more akin to HTTP 5xx.

There is probably some benefit in adding some generic error types for things that could be considered problems in the input and are handled by the compiler automatically. For example ABI decoding error or the failure of extcodesize check (when you try to call an address that's not a contract). We already have an issue to improve that: #11664. Just like with HTTP 4xx though, that's a much more coarse-grained classification than the business-logic-related errors you're showing in your examples. HTTP 400 is out of necessity used as a catch-all for a wide variety of error conditions and you need to look inside the application-specific output to figure out what really happened and what specific kind of error this is.

Is there a style guide that points users to the best method for handling errors?

We have a style guide but that just deals with simple syntax concerns. There are some third-party "best practices" guides so maybe it would fit one of them. Maybe @fulldecent would be interested in collaborating on that. In any case, I think this is a layer above the compiler. Would be great to see some community effort but I can't see how compiler can impose something like that without limiting potential use cases too much so that's out of scope here.

github-actions[bot] commented 1 year ago

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

github-actions[bot] commented 1 year ago

Hi everyone! This issue has been automatically closed due to inactivity. If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen. However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.