Modify _deposit and other functions to accept the exact amount of the asset's decimals and internally normalizing it

[smol-ninja]

Suggestions

1. Rename _calculateTransferAmount to _normalizeDecimalsTo18 to accurately represents its functionality and can be set as a pure function.
2. Modify _deposit to use the provided user value without requiring pre-normalization. Currently, the _deposit function expects users to input a normalized value, causing inconvenience and potential errors. By allowing this function to accept the exact amount of the asset's decimals and internally normalizing it will simplify user interactions and prevent miscalculations.

Context

The _deposit function currently requires users to manually convert their asset amounts to match the contract's expected 18-decimal format. By modifying this function to handle the normalization internally, users can directly input their desired asset amount, reducing complexity.

Current _deposit Function:

    function _deposit(uint256 streamId, uint128 amount) internal {
        // Effects: update the stream balance.
        _streams[streamId].balance += amount;

        // Retrieve the ERC-20 asset from storage.
        IERC20 asset = _streams[streamId].asset;

        // Calculate the transfer amount.
        uint128 transferAmount = _calculateTransferAmount(streamId, amount);

        // Interactions: transfer the deposit amount.
        asset.safeTransferFrom(msg.sender, address(this), transferAmount);

        // Log the deposit.
        emit ISablierV2OpenEnded.DepositOpenEndedStream(streamId, msg.sender, asset, amount);
    }

Consider the following scenario: A user wishes to deposit 1000 USDC (= ${1000}*10^{6}$). He will have to back calculate the normalized amount, ${1000}*10^{18}$ in this case, and use that as an input to the above function.

Updated _deposit Function:

    function _deposit(uint256 streamId, uint128 amount) internal {
        // Effects: update the stream balance.
        _streams[streamId].balance += normalizeDecimalsTo18(amount);

        // Retrieve the ERC-20 asset from storage.
        IERC20 asset = _streams[streamId].asset;

        // Interactions: transfer the deposit amount.
        asset.safeTransferFrom(msg.sender, address(this), amount);

        // Log the deposit.
        emit ISablierV2OpenEnded.DepositOpenEndedStream(streamId, msg.sender, asset, amount);
    }

In this approach, user does not have to do any additional calculation.

New _normalizeDecimalsTo18 function:

    function _normalizeDecimalsTo18(
        uint256 streamId,
        uint128 amount
    )
        internal
        view
        returns (uint128 normalizedAmount)
    {
       // ............

        // Calculate the difference in decimals.
        uint8 normalizationFactor = isGreaterThan18 ? assetDecimals - 18 : 18 - assetDecimals;

        // Change the transfer amount based on the decimal difference.
        normalizedAmount = isGreaterThan18
            ? (amount / (10 ** normalizationFactor)).toUint128()
            : (amount * (10 ** normalizationFactor)).toUint128();
    }

Similarly, all other public functions will use the same design where they accept exact amount of the asset's decimals and internally normalize it.

Advantages

1. Users won't have to worry about the normalization before making a deposit. This simplies user interactions and reduce the chance of errors.
2. _normalizeDecimalsTo18 accurately represents its functionality and can be set as a pure function.

Would love to have comments from @andreivladbrg and @IaroslavMazur what do you think?

[andreivladbrg]

Thanks for opening this.

Interesting idea.

to accurately represents its functionality

i am not sure what you mean, _calculateTransferAmount also accurately represents its functionality - it calculates the amount that needs to be passed in the ERC20.transfer function

can be set as a pure function

i'm not sure this is true. we would still need to get the assetDecimals from storage, so it has to remain a view function.

He will have to back calculate the normalized amount, ${1000}*10^{18}$ in this case, and use that as an input to the above function.

my counterargument to this is that the user still needs to perform a conversion to 18 decimals for rps (the reasons why we need the 18-format for rps are explained in the README). i find it inconsistent to have one amount converted (rps) and one not.

one more thing about the _normalizeDecimalsTo18 function is that it won't address the cases when we need to convert the internal calculated amounts (withdrawable, refundable) to a transfer amount:

https://github.com/sablier-labs/v2-open-ended/blob/77cf2b629fc264423058281afced94894051b51a/src/SablierV2OpenEnded.sol#L376

https://github.com/sablier-labs/v2-open-ended/blob/77cf2b629fc264423058281afced94894051b51a/src/SablierV2OpenEnded.sol#L404

https://github.com/sablier-labs/v2-open-ended/blob/77cf2b629fc264423058281afced94894051b51a/src/SablierV2OpenEnded.sol#L627

this means we would need two functions for these conversions (lower decimals to 18 decimals and 18 decimals to higher decimals): _normalizeDecimalsTo18 and _normalizeDecimalsFrom18

New _normalizeDecimalsTo18 function:

we would also need a _normalizeDecimalsFrom18, which would have the same implementation as _calculateTransferAmount.

Advantages

1. Users won't have to worry about the normalization before making a deposit. This simplies user interactions and reduce the chance of errors.
2. _normalizeDecimalsTo18 accurately represents its functionality and can be set as a pure function.

for 1. although i agree that some users might find it problematic to normalize before deposit, i believe that we would offer a better DX once we release the UD21x18 type in prb-math: PaulRBerg/prb-math#212

for 2.: as covered at the beginning, i don't see it as an advantage. whether it is a pure or view function, it's still a constant function, which is what matters after all

if we weigh the advantages and disadvantages, i think keeping only _calculateTransferAmount would be better.

please correct me if i misunderstood something, and also @IaroslavMazur what is your take here?

[smol-ninja]

Thanks for taking the time to reply to this Andrei.

i'm not sure this is true. we would still need to get the assetDecimals from storage, so it has to remain a view function.

Right! It has to be a view function since we need assetDecimals from the storage. I was thinking of having assetDecimal as an input parameter to _normalizeDecimalsFrom18 but that does not change anything.

we would need two functions for these conversions (lower decimals to 18 decimals and 18 decimals to higher decimals): _normalizeDecimalsTo18 and _normalizeDecimalsFrom18

I agree.

my counterargument to this is that the user still needs to perform a conversion to 18 decimals for rps (the reasons why we need the 18-format for rps are explained in the README). i find it inconsistent to have one amount converted (rps) and one not.

Fair point. My argument is that ratePerSecond is an input parameter for create which is called once per stream (and restartStream as well). While the call to deposit is going to be more frequent for every stream so having depositAmount with asset's decimals can offer a better experience to the API.

[IaroslavMazur]

Having the assets represented throughout the smart contracts in both the normalized and their original form would be fairly (and unnecessarily) complicated and error-prone.

Therefore, unless we come up with a way of applying the suggested approach to also address this issue from README, the current solution is better, imo.

[smol-ninja]

I didn't mean to represent assets in their original form. What I meant was to allow deposit to take the amount input with original decimals instead of normalized.

    /// @inheritdoc ISablierV2OpenEnded
    function deposit(uint256 streamId, uint256 amount) external noDelegateCall notCanceled(streamId) {
        // Checks, Effects and Interactions: deposit on stream.
        _deposit(streamId, amount);
    }

instead of

    /// @inheritdoc ISablierV2OpenEnded
    function deposit(uint256 streamId, uint128 amount) external noDelegateCall notCanceled(streamId) {
        // Checks, Effects and Interactions: deposit on stream.
        _deposit(streamId, amount);
    }

I understand there is no additional benefit with this approach except that the deposit API would be friendlier for users as they won't have to normalize the asset decimal before calling deposit. But since we will have periphery contracts to interact with the EOES core, we are fine with this approach too.

[smol-ninja]

Closing this discussion since I also agree with the time based implementation of deposit.