Move variables from Defaults to Constants

[smol-ninja]

What if we move constant and immutable variables from Defaults to Constants contract? This can remove several redundant lines from the code where we have to set the default value onto a local variable to make expectRevert work.

For example,

uint256 index1 = defaults.INDEX1();
uint128 amount = defaults.CLAIM_AMOUNT();
bytes32[] memory merkleProof = defaults.index1Proof();
uint256 fee = defaults.FEE();

vm.expectRevert(abi.encodeWithSelector(Errors.SablierMerkleBase_InsufficientFeePayment.selector, 0, fee));
merkleBase.claim{ value: 0 }(index1, users.recipient1, amount, merkleProof);

would become

bytes32[] memory merkleProof = defaults.index1Proof();

vm.expectRevert(abi.encodeWithSelector(Errors.SablierMerkleBase_InsufficientFeePayment.selector, 0, FEE));
merkleBase.claim{ value: 0 }(INDEX1, users.recipient1, CLAIM_AMOUNT, merkleProof);

cc @sablier-labs/evm.

[andreivladbrg]

i don't remember exactly the initial reason for not inheriting the Defaults contract and instead deploying it in BaseTest. but now, i believe it would be better to simply inherit it. (cc @PaulRBerg do you remember?)

so, i’d say not to move just some variables to Constants while keeping others in Defaults, but rather to move all variables to a single contract (either we call it Constants or Defaults) that will be inherited in BaseTest (similarly how we do it in Flow tests). wdyt?

[smol-ninja]

Yes I agree. I just realized we did the same in Flow.

[PaulRBerg]

Your plan sounds good.

The original rationale, AFAIK, was to provide context when reading the texts — seeing defaults.VAR_NAME instead of VAR_NAME points to the fact that that value is a global default used in multiple tests. However, you make a good point @smol-ninja that because of the expect revert behavior of Foundry, we need to pre-define the vars, which makes the original rationale not so strong anymore.