Remove constructor arguments

[andreivladbrg]

In the last PR, I suggested removing the initialNFTDescriptor from the constructor arguments because we implemented a setter function. This way, we can reduce some code in multiple places such as source code, scripts, and CI deployments.

The same think can be done for comptroller.

What do you think about this idea?

[PaulRBerg]

It's an interesting idea.

On the one hand, you are correct that removing the constructor arguments would simplify the deployment structure, reducing the side of the code base in the process.

On the other hand, not initializing the comptroller and the NFT descriptor in the constructor would mean that there is a risk of the contracts remaining uninitialized after construction (this is the sort of minor issue that gets flagged during audits). However, this risk is tiny and tolerable. We could just make a new deployment if users start interacting with the contracts before the comptroller and/ or the NFT descriptor is set.

Before making a decision, I want to sleep on this idea for 24 hours.

[razgraf]

Technically it is feasible as "not having a comptroller" would just prevent anyone from creating streams until it's been assigned.

It will probably simplify the code-base but it will make our deployment process more complex and costly: instead of the individual contract deployment we would now have to introduce 2 "linking" transactions for each lockup contract, to assign the comptroller and descriptor addresses.

[PaulRBerg]

Don't think the cost is relevant, since what you spend on the separate tsx is saved in the lighter constructor. But yeah, the "linking" txs you talk about is what I was referring to in my "On the other hand" paragraph.

Still unsure if this is worth it.

[razgraf]

We'd pay the base fee for every linking transaction instead of just for the initial deployment(s), no?

Note: not the end of the world, but objectively it is more costly than the alternative 😅

[PaulRBerg]

I have considered the base fee; the saving in the constructor might be greater since Solidity does all kinds of things in the constructor.

Anyway, this is not an important aspect.

Security and UX are.

[PaulRBerg]

Based on @razgraf's feedback above, and further thinking I put into this topic, I now think that removing the constructor arguments wouldn't help much. We would inevitably have to move the initialization phase somewhere else.