Redundant SSTORE in BeaconProxy constructor

[kadenzipfel]

#4435 introduced the usage of an immutable implementation address. However, it still makes use of ERC1967Utils.upgradeBeaconToAndCall, which sets the BEACON_SLOT to newBeacon.

constructor(address beacon, bytes memory data) payable {
    ERC1967Utils.upgradeBeaconToAndCall(beacon, data);
    _beacon = beacon;
}

function upgradeBeaconToAndCall(address newBeacon, bytes memory data) internal {
    _setBeacon(newBeacon);
    emit BeaconUpgraded(newBeacon);

    if (data.length > 0) {
        Address.functionDelegateCall(IBeacon(newBeacon).implementation(), data);
    } else {
        _checkNonPayable();
    }
}

function _setBeacon(address newBeacon) private {
    if (newBeacon.code.length == 0) {
        revert ERC1967InvalidBeacon(newBeacon);
    }

    StorageSlot.getAddressSlot(BEACON_SLOT).value = newBeacon;

    address beaconImplementation = IBeacon(newBeacon).implementation();
    if (beaconImplementation.code.length == 0) {
        revert ERC1967InvalidImplementation(beaconImplementation);
    }
}

Setting BEACON_SLOT here is redundant since we use an immutable implementation address.

[Amxx]

Hello @kadenzipfel

This sstore was kept on purpose. While it is not necessary for the contract to work, it is used for "beacon discovery".

The beacon proxy contains no public function, so that it is fully transparent, but that means it also does not (and cannot) include any getter. Now immagine you want to know which beacon a proxy uses. There is no easy way to do so. Our tolling used to rely (and still relies) on ERC-1967 slots for that kind of information.

Writting the address of the beacon to the BEACON_SLOT sets that slot so that tolling suc as OpenZeppelin's upgrade plugin are able to detect which beacon is used.

[kadenzipfel]

Hello @kadenzipfel

This sstore was kept on purpose. While it is not necessary for the contract to work, it is used for "beacon discovery".

The beacon proxy contains no public function, so that it is fully transparent, but that means it also does not (and cannot) include any getter. Now immagine you want to know which beacon a proxy uses. There is no easy way to do so. Our tolling used to rely (and still relies) on ERC-1967 slots for that kind of information.

Writting the address of the beacon to the BEACON_SLOT sets that slot so that tolling suc as OpenZeppelin's upgrade plugin are able to detect which beacon is used.

Fair enough, but fwiw I think a better solution would be to retrieve the immutable from the end of the bytecode or emit an event instead of using storage

[max-clinch]

Hello @kadenzipfel
This sstore was kept on purpose. While it is not necessary for the contract to work, it is used for "beacon discovery".
The beacon proxy contains no public function, so that it is fully transparent, but that means it also does not (and cannot) include any getter. Now immagine you want to know which beacon a proxy uses. There is no easy way to do so. Our tolling used to rely (and still relies) on ERC-1967 slots for that kind of information.
Writting the address of the beacon to the BEACON_SLOT sets that slot so that tolling suc as OpenZeppelin's upgrade plugin are able to detect which beacon is used.

Fair enough, but fwiw I think a better solution would be to retrieve the immutable from the end of the bytecode or emit an event instead of using storage

Both methods can be effective, and the choice often depends on factors such as gas efficiency.
If gas efficiency is a concern, parsing the bytecode might be more expensive than reading from storage, so emitting an event could be a lighter-weight solution. On the other hand, if the gas cost is acceptable, parsing the bytecode might offer a more compact storage and reduce the reliance on additional storage slots.

[ernestognw]

These are all great reasons to avoid the extra SSTORE. Also, an SSTORE is on the list of things we'd push to optimize for.

Nonetheless, ERC1967 didn't provide a standard way to retrieve the beacon address from a client but it suggests doing it by reading from the slot:

[...] In order to know the logic contract used by a beacon proxy, a client SHOULD:

• Read the address of the beacon for the beacon logic storage slot;
• Call the implementation() function on the beacon contract.

Aside from our upgrades plugins library relying on the slot value, we also agreed that, if following the standard, there's not a more reliable option to retrieve the admin, implementation, and beacon addresses than from the slot.

I think a better solution would be to retrieve the immutable from the end of the bytecode

Yeah this is good, but it's non-standard. It's breaking for platforms and tooling supporting EIP-1967, although it's possible everyone just adapts (not optimistic about this).

or emit an event instead of using storage

The EIP states that storage slot changes SHOULD be notified by events. If we're not changing the slot, then clients should arguably not expect an event to be emitted. This is just too ambiguous to rely on imo.

Since this is only done at construction, we generally thought the extra SSTORE was not the end of the world in this case 😅

[ernestognw]

Closing for inactivity. I'll assume we agreed on the resolution 😋

Comments will be kept open in case anyone wants to add something.