The default test token should not have 18 decimals

[PaulRBerg]

The current default test token is DAI, which has 18 decimals. This is not great because Flow normalizes and denormalizes token amounts to 18 decimals.

The default asset should be a little more complex and not have 18 decimals.

Doing so would avoid cases like this, where the test passes because of an accident not because it is correctly written:

flow/test/integration/concrete/refund/refund.t.sol

Line 137 in 3606035

uint128 actualTransferAmount = flow.refund({ streamId: streamId, amount: REFUND_AMOUNT });

This test works because the default asset has 18 decimals and the REFUND_AMOUNT is already normalized. If the default asset didn't have 18 decimals, the REFUND_AMOUNT would have to be normalized.

[PaulRBerg]

Specifically, the problem is that the current TRANSFER_AMOUNT plays two simultaneous roles:

• Raw deposit amount
• Normalized deposit amount

Whereas if the default token does not have 18 decimals, two variables have to be introduced.

[PaulRBerg]

Related:

A great deal about software engineering is an epistemological problem.

[smol-ninja]

Thanks for opening this issue.

The current default test token is DAI, which has 18 decimals

I don't think this is true. In the concrete tests, wherever the decimal can affect the results, we have always used two assets (usdc and dai). Even in the case of a refund, the tests cover both usdc and dai.

flow/test/integration/concrete/refund/refund.t.sol

Line 91 in 3606035

function test_GivenAssetDoesNotHave18Decimals()

flow/test/integration/concrete/refund/refund.t.sol

Line 108 in 3606035

function test_GivenAssetHas18Decimals()

If the default asset didn't have 18 decimals, the REFUND_AMOUNT would have to be normalized

Now on REFUND_AMOUNT, since the input amount is always expected to be normalized, it has been fixed as a constant.

Whereas if the default token does not have 18 decimals, two variables have to be introduced

The role of the function getTransferAmount in Utils.sol is to denormalize the value. So it essentially is doing the same job of having two values. We have used the return value to compare the transfer event.

flow/test/integration/concrete/refund/refund.t.sol

Line 136 in 3606035

expectCallToTransfer({ asset: asset, to: users.sender, amount: transferAmount });

getTransferAmount is a very poor choice of name and we may rename it to denormalizeValue (or something appropriate).

[PaulRBerg]

I don't think this is true

I think it is.

In the concrete tests, wherever the decimal can affect the results, we have always used two assets (usdc and dai)

Not true because the decimal affects the result whenever a numerical constant is used, and not every test that references a numerical constant tests both DAI and USDC.

I did not say the concrete tests test only one token, which has 18 decimals.

I said that the token used in the default stream has 18 decimals. By extension, the default number of decimals is assumed to be 18 throughout the tests.

Now on REFUND_AMOUNT, since the input amount is always expected to be normalized

I've seen this constant used as both a normalized and non-normalized amount. This works because the default number of decimals is 18.

Also related: #199

getTransferAmount is a very poor choice of name

Yeah, I have renamed it in my branch.

[andreivladbrg]

Wether we use an amount of 18 decimals for TRANSFER_AMOUNT or not, IMO is not that relevant as long as we have tests for different token decimals where that changing of the normalized amount happens, Helpers.calculateNormalizedAmount & Helpers.calculateTransferAmount , i.e. refund withdraw and deposit functions (and we do):

flow/test/integration/concrete/deposit/deposit.t.sol

Lines 33 to 45 in 3606035

	function test_GivenAssetDoesNotHave18Decimals()
	external
	whenNoDelegateCall
	givenNotNull
	whenTransferAmountNotZero
	whenAssetDoesNotMissERC20Return
	{
	// It should make the deposit.
	uint256 streamId = createDefaultStream(IERC20(address(usdc)));
	_test_Deposit(streamId, usdc, TRANSFER_AMOUNT_6D, 6);
	}
	function test_GivenAssetHas18Decimals()

flow/test/integration/concrete/refund/refund.t.sol

Lines 91 to 108 in 3606035

	function test_GivenAssetDoesNotHave18Decimals()
	external
	whenNoDelegateCall
	givenNotNull
	whenCallerSender
	whenRefundAmountNotZero
	whenNoOverRefund
	givenNotPaused
	whenAssetDoesNotMissERC20Return
	{
	uint256 streamId = createDefaultStream(IERC20(address(usdc)));
	depositDefaultAmount(streamId);
	// It should make the refund.
	_test_Refund(streamId, IERC20(address(usdc)), 6);
	}
	function test_GivenAssetHas18Decimals()

flow/test/integration/concrete/withdraw-at/withdrawAt.t.sol

Lines 186 to 219 in 3606035

	function test_GivenAssetDoesNotHave18Decimals()
	external
	whenNoDelegateCall
	givenNotNull
	whenTimeBetweenLastTimeUpdateAndCurrentTime
	whenWithdrawalAddressNotZero
	whenWithdrawalAddressIsOwner
	givenBalanceNotZero
	whenAmountOwedDoesNotExceedBalance
	{
	// Go back to the starting point.
	vm.warp({ newTimestamp: MAY_1_2024 });
	resetPrank({ msgSender: users.sender });
	uint256 streamId = createDefaultStream(IERC20(address(usdc)));
	// Deposit to the stream.
	depositDefaultAmount(streamId);
	// Simulate the one month of streaming.
	vm.warp({ newTimestamp: WARP_ONE_MONTH });
	// Make recipient the caller for subsequent tests.
	resetPrank({ msgSender: users.recipient });
	// It should withdraw the amount owed.
	_test_Withdraw({
	streamId: streamId,
	to: users.recipient,
	depositedAmount: DEPOSIT_AMOUNT,
	expectedWithdrawAmount: WITHDRAW_AMOUNT
	});
	}
	function test_GivenAssetHas18Decimals()

For example, in pause the token decimals are not relevant.
In the end, wether we call and declare the "default" amount with 6 or 18 decimals it just about wording, so we could do either way.

[PaulRBerg]

@andreivladbrg I'm afraid I don't think you have understood my argument. I have explained why we want a non-18 default number of decimals in my previous reply. Could you please re-read it and tell me what you think about it?

wether we call and declare the "default" amount with 6 or 18 decimals it just about wording

No. It's not just about wording. When the default number of decimals is 18, the constants have duplicate meanings.

[andreivladbrg]

the REFUND_AMOUNT would have to be normalized

no, as Shub said, the input param is a 18 decimal number only

I've seen this constant used as both a normalized and non-normalized amount. This works because the default number of decimals is 18.

where exactly it has been used as non-normalized amount?
REFUND_AMOUNT = is a normalized only amount

once again, that input param in refund is not a transfer amount which means that it has to be of 18-decimals number. if you have a token with 6 decimals, the param it still has to be of 18 decimal

[andreivladbrg]

I think if we make this change: #184 it would make the distinction between them clearer.

[PaulRBerg]

the input param is a 18 decimal number only

yes, and as I said in the OP, the same REFUND_AMOUNT was used as a non-normalized value throughout the tests. This was possible only because the default number of decimals was 18.

For example:

flow/test/integration/concrete/refund/refund.t.sol

Line 130 in 3606035

emit RefundFromFlowStream({ streamId: streamId, sender: users.sender, refundAmount: REFUND_AMOUNT });

that input param in refund is not a transfer amount which means that it has to be of 18-decimals number.

Yes.

have a token with 6 decimals, the param it still has to be of 18 decimal

Exactly. We have to explicitly normalize it and maintain multiple constants.

I think if we make this change: #184 it would make the distinction between them clearer.

The PRBMath types are orthogonal to this issue.

[andreivladbrg]

i guess the misunderstanding between us comes from the fact that, according to the current design (main), the amount implicitly means an 18-decimal number. you’re referring to the refactor you’ve just made, right?

For example

the event correctly emits the refund amount (18 decimal, not the transfer amount)

possible only because the default number of decimals was 18

that's because any amount is expected to be of 18, if "transfer" is not specified

The PRBMath types are orthogonal to this issue

i don't think so, using UD21x18, clearly points out the fact that amount is of 18 decimal format (similar to brokerFee):

/// @notice Emitted when assets are refunded from a Flow stream.
/// @param streamId The ID of the Flow stream.
/// @param sender The address of the stream's sender.
/// @param refundAmount The amount of assets refunded to the sender, denoted in 18 decimals.
/// @param transferAmount The amount transferred to the sender, denoted in asset's decimals.
event RefundFromFlowStream(
    uint256 indexed streamId, address indexed sender, UD21x18 refundAmount, uint128 transferAmount
);

/// @dev Emits `RefundFromFlowStream` event
/// @param streamId The ID of the stream to refund from.
/// @param amount The amount to refund, denoted in 18 decimals.
function refund(uint256 streamId, UD21x18 amount) external returns (uint128 transferAmount);

abstract contract Constants {
    UD21x18 internal constant REFUND_AMOUNT = UD21x18.wrap(10_000e18);
}

[PaulRBerg]

Yeah I think you're right, there doesn't appear to be any double use of REFUND_AMOUNT.

I still find it helpful to use a default token with 6 decimals to draw a line in the sand between normalized and non-normalized amounts.

At a minimum, it would avoid confusions like mine.

that's because any amount is expected to be of 18, if "transfer" is not specified

Yes, and that's too simplistic as per our discussion in #199

[andreivladbrg]

At a minimum, it would avoid confusions like mine.

it is impossible to prevent everyone's confusion.
this doesn’t mean that we shouldn’t try to minimize it as much as possible (see natspec for the amounts - specifies the number of decimals):

flow/src/interfaces/ISablierFlow.sol

Line 61 in f82aedc

/// @param refundAmount The amount of assets refunded to the sender, denoted in 18 decimals.

flow/src/interfaces/ISablierFlow.sol

Line 336 in f82aedc

/// @param amount The amount to refund, denoted in 18 decimals.

Yes, and that's too simplistic as per our discussion in #199

you stated this, but we have not yet agreed on whether it is 'simplistic' on that discussion

---

however, i left a relevant answer here: #202 (comment)

[PaulRBerg]

it is impossible to prevent everyone's confusion ... this doesn’t mean that we shouldn’t try to minimize it

Indeed. This was my goal with changing the default test token to use 6 decimals.

(see natspec for the amounts - specifies the number of decimals):

Unrelated to this issue/ discussion. Here we're talking about constant names in the tests.

The explicit names I have added in #205 are clearer because they are explicit.

you stated this, but we have not yet agreed on whether it is 'simplistic' on that discussion

It doesn't say anything about what happened to the amount.

The normalized terminology (or scaled) indicates that a normalization/ scaling process has occurred.