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

♻️ Unnecessary custom errors #1

Open
0xClandestine opened this issue May 14, 2023 · 1 comment
Open

♻️ Unnecessary custom errors #1

0xClandestine opened this issue May 14, 2023 · 1 comment

Comments

@0xClandestine
Copy link

First of all sick proposal 👏👏👏, just thought I would suggest some gas optimizations.

  1. These custom errors aren't really needed, and increase gas since an unnecessary conditional check is needed to throw the error. In both transfer and transferFrom the checked arithmetic would throw a revert regardless of the conditional. The same applies to the InsufficientPermission check within transferFrom.
/// @dev Thrown when owner balance for id is insufficient.
/// @param owner The address of the owner.
/// @param id The id of the token.
error InsufficientBalance(address owner, uint256 id);

/// @dev Thrown when spender allowance for id is insufficient.
/// @param spender The address of the spender.
/// @param id The id of the token.
error InsufficientPermission(address spender, uint256 id);

/// @notice Transfers an amount of an id from the caller to a receiver.
/// @param receiver The address of the receiver.
/// @param id The id of the token.
/// @param amount The amount of the token.
function transfer(address receiver, uint256 id, uint256 amount) public {
    if (balanceOf[msg.sender][id] < amount) revert InsufficientBalance(msg.sender, id);
    balanceOf[msg.sender][id] -= amount;
    balanceOf[receiver][id] += amount;
    emit Transfer(msg.sender, receiver, id, amount);
}
  1. You can also use unchecked when modifying the receivers balance since totalSupply cannot exceed 2**256-1. Checkout transmissions/solmate/tokens/ERC20.sol for reference.
@jtriley-eth
Copy link
Owner

gm, thanks for the issue & pr!

so the reference implementation is meant more for readability and making the specification explicit. so tradeoffs were made in favor of readability over gas efficiency.

would you mind moving the optimized implementation to the alt/ directory? it can take the name of ERC6909Optimized.sol. from there we can make further optimizations and stray further from readability.

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

No branches or pull requests

2 participants