Provide mock ERC-20 and ERC-721 contracts

[PaulRBerg]

In OpenZeppelin v5, they have made the ERC20 and ERC721 contracts abstract:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/793d92a3331538d126033cbacb1ee5b8a7d95adc/contracts/token/ERC20/ERC20.sol#L34

This means that for testing purposes we all need to define a dummy ERC-20 contract like this:

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.8.20;

import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract ERC20Mock is ERC20 {
    constructor(string memory name, string memory symbol) ERC20(name, symbol) { }
}

The same requirement applies to ERC-721.

It would be helpful if Forge Std provided these mocks for users so they don't end up re-implemented all over GitHub.

[andreivladbrg]

I can work on this, but I don't think it would be a good decision for forge-std to have openzeppelin installed as a dependency.

The only way to make it work is to copy and paste the ERC20 and ERC721 contracts into a mocks dir.

wdyt?

[PaulRBerg]

Great point @andreivladbrg; let's see what @mds1 and other Forge Std contributors have to say.

[mds1]

This seems like a good idea. Agreed with the requirement of not adding and dependencies to forge-std.

Similar to the interfaces directory, these should not be imported anywhere by default to avoid impacting compilation times, so they can live as standalone contracts in src/mocks/

One consideration is the tokens added should support solidity 0.6.2 through 0.8.x. This means we'll have to implement checked math in the token directly, so we likely just can't copy an existing token out of the box. The easiest is probably to copy solmate's because it's a flat file and easy to read, then edit it to add checked math. The file should have comments indicating it's not for use in production, and include a comment with permalink to the original source it was based on.

What devex do you think this better here?

1. Include permissionless mint(address to, uint256 amount) and burn(address from, uint256 amount) methods to make it clear it's a mock
2. Keep it as a standard token with no mint/burn functionality and use deal to set balances. With this approach, users will likely want to use the deal(address token, address to, uint256 give, bool adjust) version with the last arg as true to ensure total supply is updated, otherwise it will stay at zero

I think I'd lean towards (2) to keep the token minimal, as it's always easier to add features if they end up being needed later, as opposed to removing them once users already rely on them.

@andreivladbrg I've assigned this to you since you mentioned you can work on it, if that's no longer the case just let me know!

[andreivladbrg]

This means we'll have to implement checked math in the token directly

I think this is the case only for ERC20 balances, and not for ERC721 counters, am I right?

I think I'd lean towards (2) to keep the token minimal

I agree, we should go for option 2.

I've assigned this to you since you mentioned you can work on it,

Yes, will work on this

[PaulRBerg]

also agree that option (2) is the way to go

[zerosnacks]

Hi all,

Personally I don't think it makes a lot of sense to include (mock) implementations in forge-std and it doesn't appear that many people have been using it - I haven't been able to find examples on Github. I think it is better delegated to libraries like OpenZeppelin and Solady that people are already using in their projects and have extensive test suites. Given the moving landscape of ERCs being proposed it is also arguable that we are missing mocks like ERC4626, ERC6909, etc..

Interfaces on the other hand totally make sense to me to include with forge-std as it is an easy way to interact with contracts in a type-safe way without having to pull in external dependencies.

The reason why I'm raising this is because of the proposed removal of testFail* in Foundry.

For testFail* I'll either need to spend time updating the test suites of MockERC20 / MockERC721 that make heavy use of testFail*:
#643 or, what I'm proposing - remove them. Other alternative is just to remove the outdated test suite.

Do you have an opinion on whether we should keep or remove this?

[andreivladbrg]

Hey there, I understand and it totally makes sense not to keep them given that they are not being used.

Also, I want to mention that since the Solmate version (minimal version) is being used, someone might prefer to use the OpenZeppelin version, which includes the _update hook.