Use contracts instead of interfaces declarations in Base_Test

[andreivladbrg]

After the package tethering change, we’ve significantly increased the number of imports in Base.t.sol:

v2-core/test/Base.t.sol

Lines 4 to 32 in a322ba6

	import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
	import { ILockupNFTDescriptor } from "src/core/interfaces/ILockupNFTDescriptor.sol";
	import { ISablierLockupDynamic } from "src/core/interfaces/ISablierLockupDynamic.sol";
	import { ISablierLockupLinear } from "src/core/interfaces/ISablierLockupLinear.sol";
	import { ISablierLockupTranched } from "src/core/interfaces/ISablierLockupTranched.sol";
	import { LockupNFTDescriptor } from "src/core/LockupNFTDescriptor.sol";
	import { SablierLockupDynamic } from "src/core/SablierLockupDynamic.sol";
	import { SablierLockupLinear } from "src/core/SablierLockupLinear.sol";
	import { SablierLockupTranched } from "src/core/SablierLockupTranched.sol";
	import { LockupDynamic, LockupLinear, LockupTranched } from "src/core/types/DataTypes.sol";
	import { ISablierBatchLockup } from "src/periphery/interfaces/ISablierBatchLockup.sol";
	import { ISablierMerkleFactory } from "src/periphery/interfaces/ISablierMerkleFactory.sol";
	import { ISablierMerkleInstant } from "src/periphery/interfaces/ISablierMerkleInstant.sol";
	import { ISablierMerkleLL } from "src/periphery/interfaces/ISablierMerkleLL.sol";
	import { ISablierMerkleLT } from "src/periphery/interfaces/ISablierMerkleLT.sol";
	import { SablierBatchLockup } from "src/periphery/SablierBatchLockup.sol";
	import { SablierMerkleFactory } from "src/periphery/SablierMerkleFactory.sol";
	import { ERC20MissingReturn } from "./mocks/erc20/ERC20MissingReturn.sol";
	import { ERC20Mock } from "./mocks/erc20/ERC20Mock.sol";
	import { RecipientGood } from "./mocks/Hooks.sol";
	import { Noop } from "./mocks/Noop.sol";
	import { Assertions } from "./utils/Assertions.sol";
	import { Calculations } from "./utils/Calculations.sol";
	import { Constants } from "./utils/Constants.sol";
	import { Defaults } from "./utils/Defaults.sol";
	import { DeployOptimized } from "./utils/DeployOptimized.sol";
	import { Events } from "./utils/Events.sol";
	import { Fuzzers } from "./utils/Fuzzers.sol";
	import { Users } from "./utils/Types.sol";

$~$

One idea to reduce the number of imports (assuming we agree that having this many imports isn’t ideal) is to declare the testing contracts as contracts instead of interfaces (see the code below). Since the contracts themselves already inherit the interfaces, I don’t see any downside to this approach, as it would reduce the overall number of imports throughout the test files.

instead of:

ISablierLockupDynamic internal lockupDynamic;
ISablierLockupLinear internal lockupLinear;
ISablierLockupTranched internal lockupTranched;

we could (option 1):

SablierLockupDynamic internal lockupDynamic;
SablierLockupLinear internal lockupLinear;
SablierLockupTranched internal lockupTranched;

---

Another alternative to reducing the number of imports would be to import them directly from the contract (option 2):

import { LockupNFTDescriptor, ILockupNFTDescriptor } from "src/core/LockupNFTDescriptor.sol";
import { SablierLockupDynamic, ISablierLockupDynamic } from "src/core/SablierLockupDynamic.sol";
import { SablierLockupLinear, ISablierLockupLinear } from "src/core/SablierLockupLinear.sol";
import { SablierLockupTranched, ISablierLockupTranched } from "src/core/SablierLockupTranched.sol";

Personally I chose option 1. wdyt @sablier-labs/solidity?

[smol-ninja]

Interesting proposal.

Since the contracts themselves already inherit the interfaces, I don’t see any downside to this approach

By using interface as a datatype for a contract variable, it ensures that the public functions exposed by the contract are also included in the interface. This is because a contract can define more public function than the interface it inherits from. Consider the following case:

interface IA {}

contract A is IA {
    function foo() public {}
}

If we only import A, the test would pass when it calls foo but if you import IA and define instance of A as IA, the test would fail when it calls foo. Thats the advantage option 1 would miss.

assuming we agree that having this many imports isn’t ideal

I dont know what is ideal but having so many imports is common in testing: https://github.com/aave-dao/aave-v3-origin/blob/main/tests/utils/BatchTestProcedures.sol (our approach is rather cleaner than Aave's). I would not call it "not ideal" since its the base contract for testing so that should be fine imo.

[andreivladbrg]

it ensures that the public functions exposed by the contract are also included in the interface

good point, i agree

I would not call it "not ideal" since its the base contract for testing so that should be fine imo.

what do you say about option 2?

[smol-ninja]

Re option 2, no argument against it, just that I haven't seen an imports like these before where components are imported via inherited contracts. Its not typical to import through inherited components imo. But I suppose it works technically, right?

Also, chatgpt's input on this: https://chatgpt.com/share/670bdc9d-869c-8000-9368-7a22eea99fdd.

Even though LockupNFTDescriptor inherits from ILockupNFTDescriptor, they are still distinct contracts/interfaces defined in separate files.

[andreivladbrg]

But I suppose it works technically, right?

it does, yes

Even though LockupNFTDescriptor inherits from ILockupNFTDescriptor, they are still distinct contracts/interfaces defined in separate files.

good point.

---

i changed my mind about this proposal. it is better to keep the current imports closing