ERC-8109: Diamonds, Simplified

[mudgen]

@wjmelements

I think the events can be simplified into one. The current design has 3:

• DiamondFunctionAdded(selector, facet)
• DiamondFunctionReplaced(selector, oldFacet, newFacet)
• DiamondFunctionRemoved(selector, oldFacet)

I think one method would be better:

• SetDiamondFacet(selector, facet)

This would mean that configuration functions would not have to SLOAD the previous value. Since these are logs, reading the prior value can already be done by loading prior logs.

@wjmelements

Here is the information I have so far:
Pros for SetDiamondFacet(selector, facet)

• It is simpler because it is one event instead of three.
• Perhaps it is easier to process by tools because it is one event, and not three.
• It is more gas efficient than the DiamondFunctionReplaced(selector, oldFacet, newFacet) event.
• Diamonds can be implemented with it in a way that anSLOAD is not done for each selector to see if it already exists before adding/replacing/removing it. This can significantly reduce gas costs for adding/replacing/removing selectors.

In general I like the simplicity of SetDiamondFacet.

Cons:

• After an upgrade, the SetDiamondFacet events in the upgrade transaction do not say specifically if selectors were added or replaced. To understand that from events, tooling has to retrieve and process the initial deployment and the entire history of upgrades. Or tooling can examine transactions. The three events are more explicit and provide more information – if a selector is added or replaced, and what the old facet is.

SetDiamondFacet(selector, facet) does not save an extra SLOAD if current integrity and safety checks are performed. Current diamond implementations verify intent. If a person specifies to add functions – but if one or more of those functions already exist, then the diamond will revert. Similarly a diamond won’t let a person replace a function that doesn’t exist, and won’t let a person remove a function that doesn’t exist. This functionality also prevents selector clashes from being added to a diamond – two different functions with the same function selector but different function signatures. Though that is highly unlikely.

The SetDiamondFacet(selector, facet) event does not save an extra SLOAD if introspection functions are used. The introspection could be made optional, but I feel that would weaken tooling. Blockscout relies on an introspection function to be present and uses it to retrieve facet addresses. And I think etherscan does the same. I think it could be much harder for them to implement and reliably support diamonds using events, and they may not want to. @vbaranov what do you think?

On the other hand it is up to implementers to decide how their diamonds work.

I do see the advantage of implementing a diamond without an SLOAD for each selector to check if it exists already before adding/replacing/removing it to the diamond. That saves significant gas.