Use custom error types

[ItsNickBarry]

Replace error strings with custom error types: https://blog.soliditylang.org/2021/04/21/custom-errors/

[wiasliaw]

Hi, @ItsNickBarry,
I can help with the this issue.
It will be a large commits including replacement and testing. I think it should create a branch for this issue. After everything is done, then merge into the master.

[ItsNickBarry]

Okay, that would be appreciated. There are two open-ended questions that will need to be answered:

1. Can we apply a naming convention to error types? Should we include contract name in the error name? For example, the existing error "Ownable: sender must be owner" could be replaced with something like OwnableSenderMustBeOwner or OwnableUnauthorized, or simply Unauthorized.
2. What arguments should be included in the custom errors? This will vary case-by-case. Use your judgment, and we'll review later.

[wiasliaw]

1. I think it is OK for both included contract name or not. If not included the name, it should be careful that different contracts might have the same reason string. For example, "ERC20: transfer to the zero address" and "ERC721: transfer to the zero address" in Openzeppelin's implementation. But I didn't find that in solidstate-solidity.

2. From my opinion, it is rare that arguments are placed in reason string. For example, "Ownable: ${address} is not the owner". For most of cases, good naming of custom error is enough. Arguments are included if providing better readability.

Some of the cases below providing arguments will be better:

• solidstate-solidity/contracts/utils/EnumerableMap.sol

  Line 167 in 0ba5ba1

  require(keyIndex != 0, 'EnumerableMap: nonexistent key');

  • error NonexistentKey(bytes32 key)

• solidstate-solidity/contracts/utils/EnumerableMap.sol

  Line 146 in 0ba5ba1

  'EnumerableMap: index out of bounds'

  • error OutOfBounds(uint256 bound)

[ItsNickBarry]

I agree. Let's not include contract name in the error names. Go ahead and make the changes on your branch, and PR when ready.

[wiasliaw]

@ItsNickBarry

Addressing some issues here.

1. after adding custom error, abstract contracts and libraries also generated abi in /abi. For example, ECDSA generated the abi but only error selectors. I don't think it is OK.

2. About testing, waffle supports custom error. Here's the PR. However, it combines reason string and custom error and make confusing readability. I might propose hardhat-chai-matchers.

[ItsNickBarry]

Right, I forgot that library ABIs would be generated separately. I suppose that means that the errors defined in libraries only appear in the library ABIs, and not in the contract ABIs? I've had the same issue with events in the past.

If so, that might make this change untenable until tools are available to easily include library ABIs in the relevant contracts.

[wiasliaw]

I don't know if it is OK to generate abi that user don't need. If a contract is import a .sol, which will generate the same abi.

[ItsNickBarry]

Maybe I misunderstood. I did a test, and it seems like errors defined in libraries are properly included in the ABIs of contracts that import them. In that case, I don't see any issues.

I don't know if it is OK to generate abi that user don't need. If a contract is import a .sol, which will generate the same abi.

It's okay if extra ABIs are generated.

[ItsNickBarry]

@Lumyo has opened PR #139.