feat: add mock contracts for ERC20 and ERC721

[andreivladbrg]

Closes #469

[mds1]

Thanks for this! Only skimmed the code but lgtm so far, will take a closer look once we get CI passing.

It seems we'll need to make a few changes so it compiles with older versions, see the run here or the screenshot below. The immutable fix is easy, we can just remove that

The chain ID is a bit trickier but we found a good way to do that in StdCheats.sol, which you can copy. In the code you'd call the _pureChainId() method: https://github.com/Tudmotu/forge-std/blob/5dcccff9d9fd2ea1b2ef540308313ed31377a982/src/StdCheats.sol#L615-L636

For the constructor, I'm not sure if there's a solution offhand: We want to make sure there's no warnings regardless of the solc version used (you can of course test this easily by changing the solc_version in the config file), and my guess is that adding the public visibility modifier to the constructor for 0.6.12 will result in a warning for later solc versions. @mattsse if we used ignored_error_codes in forge-std's foundry.toml is that respected in upstream repos without affecting them? In other words, if we silence warning code 4937 in forge-std, does that both (1) hide forge-std's emission of that warning from upstream users, and (2) not hide upstream user's own emission of that warning? If so, that might be a suitable solution. If not, we might have to have two versions of each contract

[andreivladbrg]

@mds1, just wanted to let you know that I've pushed a commit to fix the incompatibilities with older solc versions (there were more 😁).

Regarding the constructor, it's indeed a tricky one. I'm not entirely sure how to address it yet. For now, I've set the public modifier, but there is a warning when compiling (tho it runs).

Another idea, aside from having two versions of the contracts, is to remove the constructor. We could then hard-code the metadata to default values like "Mock ERC20", "MOCK", decimals to 18, and add a changeMetadata method that allows adjustments to these values later on.

wdyt?

[mds1]

I was thinking something similar: remove the constructor and have an initialize method that can only be called once, like proxies use. Then in StdUtils we can add deployMockERC20 and deployMockERC721 helper functions to simplify the deploy + initialization process.

If the ignored error codes approach works though we should prefer that to keep the UX more familiar. It should be straightforward to test out the behavior and make sure it doesn’t suppress things for upstream users

[mds1]

Here's a constructor workaround we used to use in forge-std which should work here

/// @dev To hide constructor warnings across solc versions due to different constructor visibility requirements and
/// syntaxes, we put the constructor in a private method and assign an unused return value to a variable. This
/// forces the method to run during construction, but without declaring an explicit constructor.
uint256 private CONSTRUCTOR = _constructor();

function _constructor() private returns (uint256) {
  // logic here
  return 0;
}

[andreivladbrg]

Here's a constructor workaround we used to use in forge-std which should work here

It is a smart way of doing this, but I don't think it is entirely useful here, given that we also need to pass arguments in the constructor (name, symbol).

remove the constructor and have an initialize method that can only be called once, like proxies use. Then in StdUtils we can add deployMockERC20 and deployMockERC721 helper functions to simplify the deploy + initialization process.

I ended up doing this way.

[mds1]

Good call about the constructor, initialize approach used here LGTM

[andreivladbrg]

@mds1 Addressed all your comments, lmk if there is anything else

[mds1]

This is great, thank you!

Would you mind adding a PR to the book's forge standard library reference documenting these mocks and deploy utils?