Move shared utilities to separate repository

[PaulRBerg]

As discussed here.

The task is to move the following logic to a separate @sablier/solidity-utils package/ repo (name TBD):

• Adminable.sol
• NoDelegateCall.sol
• Batch.sol
• Some portions of Base.s.sol
• Some test files inherited by Flow from Lockup
• A shared contract for collectFee logic

[PaulRBerg]

Idea: create an abstract contract that implements the collectFee logic that is currently duplicated across the Lockup and Flow repos.

[andreivladbrg]

Similar to your idea, we could also:

1. Add a contract that inherits ERC721 and implements the _update function. (it would require a virtual function isTransferable)
2. Declare the MAX_BROKER_FEE and nextStreamId.

Though I don't have a strong opinion about these, I am not sure where we should draw the line on what to move there or not. 🤔

[smol-ninja]

The contracts mentioned in the OP are already abstract contracts. So moving them to the Utilities repo sounded like a good plan. On the other hand, _update, MAX_BROKER_FEE, and nextStreamId are contract-specific functions and variables, and we should keep them where they belong.

collectFee technically is a part of admin-related functions, so it makes sense to move it to a new abstract contract, along with other admin-related functions if there are any.

[andreivladbrg]

The contracts mentioned in the OP are already abstract contracts. So moving them to the Utilities repo sounded like a good plan. On the other hand, _update, MAX_BROKER_FEE, and nextStreamId are contract-specific functions and variables, and we should keep them where they belong

I could argue that _update function is a part of the abstract ERC721 contract.

collectFee technically is a part of admin-related functions, so it makes sense to move it to a new abstract contract, along with other admin-related functions if there are any

the transfer is made to the admin address, is not necessarily callable only by him

[smol-ninja]

I could argue that _update function is a part of the abstract ERC721 contract.

Yes it is but so what?

Another argument against it is that it is not necessary that Flow and Lockup will share the same requirements for the implementation of _update. For example, see this discussion which would have required a different implementation in Lockup.

The contracts in Utilities should be very generic and should not depend on protocol specific requirements.

the transfer is made to the admin address, is not necessarily callable only by him

Fair point.

[andreivladbrg]

Another argument against it is that it is not necessary that Flow and Lockup will share the same requirements for the implementation of _update. For example, see this discussion which would have required a different implementation in Lockup

good point