Improve error messages for ERC721 and 1155

[kanedaaaa]

Fixes #3120

As discussed in the issue, the ERC721 approve error message was misleading.

All tho, we can say the same thing on a very similar error message:
"ERC1155: the caller is not owner nor approved" or "ERC721: approve caller is not owner nor approved for all"

I didn't modify this one since the first error message fix was accepted because it's more meaningful and shorter than the previous one, which won't apply in this. And also, there is an open PR that fixes the same issue. But still, would be nice if I could add token in mentioned messages.

P.S The PR mentioned above also goes against my logic (removing the "transfer" keyword in my version) so definitely check it.

• Tests
• Documentation
• Changelog entry

[Amxx]

LGTM

[Amxx]

Wait, you are missing ERC1155Burnable

[kanedaaaa]

@Amxx Refer to what i said above. These messages aren't consistent. Thus, I had to modify only ones that contained "transfer" before "caller". I pointed out that there is another PR that completely modified every single "token owner" error message.

If it's on me, I would modify the remaining unmodified error messages with the token keyword.

I can add edits to make everything more consistent if you wish so.

[Amxx]

In ERC1155 there is ERC1155: caller is not owner nor approved. IMO the confusion with a possible contract owner also exists

[kanedaaaa]

Yes, I will definitely make the messages more consistent so wait before reviewing this PR, will add changes soon.

[frangio]

This PR is now increasing the length of revert reasons, which we initially thought would just be shortened in #3120 (comment), so it now needs some more analysis to see how this is affecting bytecode size before we agree to merge.

[kanedaaaa]

Yes, i think there was bit of misunderstanding here regarding to that. My first commit shortened the error messages, so we can just go back there since other changes that made messages longer is in second commit.

BTW, i was just thinking and since we are discussing same thing: what if there were error codes, rather than full revert message, such as:

ERC1155: ERR_001 - Then, we can write down in docs what this ERR_001 means, with more explanation and examples.

Good example from PancakeSwap: https://docs.pancakeswap.finance/help/troubleshooting

Shortens the bytecode, and it's more organised in general. But anyways, custom error messages are on the way and we might as well wait for it to be fully implementable. As i know there is still no way to trigger them with require()

[frangio]

We're catching up on pending PRs. Sorry for the delay @kanedaaaa.

Following up on my last point, I realized that the change is still net positive because in this PR you are unifying what were different revert reasons into the same revert reason and this gets optimized by the compiler so the constant is present only once in the code.

We used to have the goal to have different revert reasons for every different error condition so that the source of an error would be easy to identify, but I don't think this is a strong argument anymore because tooling has evolved and e.g. Hardhat can show the real source of an error.

---

Regarding your point about error codes: yes, it's a good pattern, but we are also now seeing projects start adopting custom errors which are like "real" error codes plus with superpowers (parameters) and this is something we plan to adopt soon as well. (#2839)

[frangio]

Thank you!