Custom errors start with the address of the emitter

[pcaversaccio]

As previously discussed here, custom errors should start with the address of the emitter.

PR Checklist

• Tests
• Documentation
• Changelog entry

[Amxx]

LGTM. Just waiting for @frangio opinion

[frangio]

I don't think I agree with this change.

The emitter of the error can be found in the trace of the transaction... What I mean is the information is already there, somewhere. Part of the motivation for using custom errors is optimizing the gas usage, so increasing the gas cost to add redundant information feels like it defeats the purpose.

[pcaversaccio]

@frangio while I do understand your argument, this change is more about the end-user experience. This change helps end-users easily identify which contract was reverted with a failed transaction, which is especially useful for complex transactions involving multiple contracts. An example of what I mean by that using Etherscan (here you see immediately that the main contract I interacted with threw the error which is actually awesome from a debugging perspective):

My Rinkeby test trx: https://rinkeby.etherscan.io/tx/0x906a1b789f33418228d6d31f4432c50614a66315af8a278069f7e0fe99da3f7f

The same argument you are giving could be applied to a certain extent applied to other angles with redundant information such as events for known future internal transactions. Including events for known internal transactions can enhance your debugging experience significantly even though all the information is already part of the transaction. In any case, what I want to achieve is kind of a better end-user UX at a very tiny additional cost of including an additional address information. Also custom errors should be highly informative and I would argue that the emitter is a piece of important information that belongs to any custom error.

[frangio]

what I want to achieve is kind of a better end-user UX

I understand this but I'm very skeptical that this is where this improvement should be done. If the information of who emitted the error is there, why aren't we asking Etherscan to surface it more visibly for the end user? Why should the smart contract take extra steps to compensate for the lack of this information?

We could argue that the pragmatic decision would be to add it at the contract level as suggested here, instead of waiting for explorers to implement it. But I think this will be counterproductive and allow explorers to go for longer without making this important improvement natively.

[pcaversaccio]

I understand this but I'm very skeptical that this is where this improvement should be done. If the information of who emitted the error is there, why aren't we asking Etherscan to surface it more visibly for the end user? Why should the smart contract take extra steps to compensate for the lack of this information?

The smart contract implementation makes the information application-agnostic, reduces the dependency on custom (backend) logic built by block explorers as well as follows the statement in the Solidity blog on custom errors:

Before the "why" there must be the "where".

Furthermore, I would quickly refer to the warning in the Solidity docs on Errors:

Never trust error data. The error data by default bubbles up through the chain of external calls, which means that a contract may receive an error not defined in any of the contracts it calls directly. Furthermore, any contract can fake any error by returning data that matches an error signature, even if the error is not defined anywhere.

I'm not saying that using the revert SomeError(address(this), ...) scheme can prevent such malicious error implementations, but it sets a good precedence on what to look at. If a custom error uses the revert SomeError(address(this), ...) pattern, you can be safe to look at the right contract for debugging already.

We could argue that the pragmatic decision would be to add it at the contract level as suggested here, instead of waiting for explorers to implement it. But I think this will be counterproductive and allow explorers to go for longer without making this important improvement natively.

I think the smart contract information should be block explorer agnostic also to help new block explorer teams to have a fast time-to-market. Given the current solution, the explorer team must just need to show the custom error whereas for the native implementation you need to build this feature first to process the information. I'm a strong advocate for a competitive landscape and we should support smart contract features that help build Etherscan competitors more easily.

[frangio]

After a brief discussion with @Amxx, both us now agree that we should not do this for the following reasons:

• It's redundant information. For debugging, services like ethtx.info and Tenderly already know how to display the emitter, and we expect block explorers to display it as well.
• It's not reliable information. Custom error parameters can be forged by an attacker.

I share the intention behind the proposal, and agree that we want a competitive landscape for explorers, but I don't think this is the way we achieve that goal.

[pcaversaccio]

@frangio @Amxx I do see the reasoning behind this decision and appreciate your feedback. Will ping the Etherscan team at least about this issue.

[PaulRBerg]

I've just caught up on everything that has been discussed here, and I think that it was the right call not to include the emitter address in the custom error, for precisely the same reasons that @frangio had.

@pcaversaccio is directionally correct - we do need better debugging tools for custom errors. But I think that there isn't any silver bullet for that. It will take a great many piecemeal efforts coming from different parties involved in the ecosystem - not just Etherscan, but Hardhat and Foundry as well, for example.