Constructors should not make unnecessary external calls

[pakim249CAL]

eigenlayer-middleware/src/BLSSignatureCheckerStorage.sol

Lines 27 to 34 in cd43f0a

	constructor(
	ISlashingRegistryCoordinator _registryCoordinator
	) {
	registryCoordinator = _registryCoordinator;
	stakeRegistry = _registryCoordinator.stakeRegistry();
	blsApkRegistry = _registryCoordinator.blsApkRegistry();
	delegation = stakeRegistry.delegation();
	}

This prevents an implementation from being deployed before dependent proxies are initialized. IMO there is no upside to doing this because the caller can do the external view calls themselves.

[samlaf]

@pakim249CAL what's the alternative here? that the contracts that require calls like stakeRegistry are initialized in the initializer instead of the constructor?

[pakim249CAL]

No, I am suggesting that the constructor accept the stake + blsApkRegistry + delegation in its arguments rather than look it up.

[stevennevins]

Afaict, your proposal doesn't solve the problem of being able to deploy the implementations before the proxies since these addresses passed in the constructor are the proxies and need to be known when deploying the implementations. I've tried predicting the addresses with deployer nonce and create2, which sounds like the next step based on what you're suggesting, but it ends up being too brittle and is low ROI complexity.

I would like to move toward what @samlaf suggested, but there isn't consensus on moving away from immutable variables in the constructor due to the gas savings

[pakim249CAL]

@stevennevins For my current use case, I am not actually predicting proxy addresses. The flow I am using is to deploy all proxies with inert implementation contracts, and then submitting a multisig transaction to upgrade/initialize them all at once for a verifiable deployment. IMO this does not add much complexity, and it has the benefit of being verifiable to anyone with just the multisig data + what is on chain.

For reference: Layr-Labs/eigenda#1396

[stevennevins]

You're still in the same situation where you need to deploy the proxies before the implementations in that case though?

[stevennevins]

IMO this does not add much complexity

i was saying if we wanted to have the implementations deployed before the proxies the only way with the current setup is to predict the proxy addresses via create2 or using the deployer nonce which is where the complexity is introduced

Were you only referring to the the inert implementations vs the actual logic contracts that we upgrade to?

[pakim249CAL]

You're still in the same situation where you need to deploy the proxies before the implementations in that case though?

Yes, the proxies would need to be deployed before the implementations.

i was saying if we wanted to have the implementations deployed before the proxies the only way with the current setup is to predict the proxy addresses via create2 or using the deployer nonce which is where the complexity is introduced

In my use case, the proxy addresses would be known and verified, so there is no complexity like having to "predict" a proxy address using a nonce/create2. You can just look it up on any applicable tool like etherscan/tenderly.

The problem is that before the registry coordinator and stake registry proxies are upgraded to the actual implementations, the construction of the above implementation will fail even though all proxy addresses are known.

In other words, the way the contract is currently written demands that the registry coordinator + stake registry are live already. This essentially forces a deployment process to need multiple stages, constructing implementations + initializing proxies for each dependency layer one at a time and eliminating the possibility of atomically verifiable deployments.

[stevennevins]

Makes sense, I think confusion came from the issue description. Agree this makes upgrades less brittle and seems like an easy win so lets do it.

I still think long term we should move to how @samlaf interpretted this. That would solve proxies needing to be deployed before implementations in most cases

[pakim249CAL]

What do you think is a better way to rename the issue? I figured the title captures the more general idea behind the example I gave, being that contract constructors should have minimal dependencies and so should not make external calls if the parameters can be provided by the caller. IMO this should be a general pattern.

[stevennevins]

This part

This prevents an implementation from being deployed before dependent proxies are initialized.

Was the confusing part based on the following discussion. the title was clear

[stevennevins]

What do you think is a better way to rename the issue? I figured the title captures the more general idea behind the example I gave, being that contract constructors should have minimal dependencies and so should not make external calls if the parameters can be provided by the caller. IMO this should be a general pattern.

For config, I find providing everything as an argument without cross contract dependencies is usually a better pattern as you mention. I think this direction of doing constructor external calls was made just to remove params from the constructor and the perceived simplification