Should we change the type of the time variables?

[andreivladbrg]

In the actual implementation of the core contracts we are storing in the Stream struct only uint256 type variables, which are expensive in terms of gas cost.
https://github.com/sablierhq/v2-core/blob/c7cbed688b87facd042dfeaa869882b7a3c66dc5/src/interfaces/ISablierV2Linear.sol#L56-L66
https://github.com/sablierhq/v2-core/blob/c7cbed688b87facd042dfeaa869882b7a3c66dc5/src/interfaces/ISablierV2Pro.sol#L81-L93

I was toying with all the uint types that solidity has and I think the best solution is to change the uint256 type to uint64 for our time variables: cliffTime segmentMilestones startTime and stopTime.

The only assumption we have to make is that all timestamps can fit in uint64, i.e. this number 18446744073709551615, this will be true until the year ~584942417303, means it is totally safe.

I've actually created a branch 0eeba8c to test the gas benefits and I was surprised to find out that this change would save us 43,212 gas in average for the create function in the SablierV2Linear contract and 140,236 in the SablierV2Pro contract.

Here are some of the screenshots of the gas reports:

Before

From this commit: 3003b34
https://imgur.com/a/fooGyFr

After

From this commit: 0eeba8c
https://imgur.com/a/HoIBp0a

[PaulRBerg]

Interesting proposal! I'm tentatively in favor of implementing this - let me just double check that this doesn't introduce any bug or breaking change and that I can reproduce the gas savings you reported.

i.e. this number 18446744073709551615, this will be true until the year ~584942417303, means it is totally safe

LOL. Unix cannot even go that far. Read about the Year 2038 problem.

I was surprised to find out that this change would save us 43,212 gas on average for the create function in the SablierV2Linear contract and 140,236 in the SablierV2Pro contract.

The gas savings are high because storing anything in contract storage is expensive.

Though I'm not sure that they are as high as you claim them to be - to get a more accurate picture, we might have to compare only the cases when the creation of the stream succeeds, because that's when the struct is created and the time variables are actually saved in storage.

[PaulRBerg]

Great, thanks for doing a gas comparison for only the case when the stream creation succeeds.

I saved 3 blocks of 256 bits from the storage, that is equivalent to ~60,000 gas.

Yes, but you didn't order them in the optimal way. For example, in the linear Stream struct, you should have defined the cliffTime after the token, because the latter is encoded as uint160, and the optimal way to order fields in a struct is to place the smallest bit-width variables at the bottom. So, ideally we would put the cliffTime after startTime.

[andreivladbrg]

Yes, but you didn't order them in the optimal way.

Well, the most important thing is to have only 5 blocks of storage, what you suggest would bring only a small change, but it is welcome.

[PaulRBerg]

It would actually be a significant change, since Solidity would pack the 4 variables together in the same EVM word.

Now it only packs the last 3 variables.

Try it and see what results you get!

[andreivladbrg]

Now it only packs the last 3 variables.

It packs only the last 2 variables.

I think you are missing the fact that the sender is packed together with cliffTime and token with startTime because 160+64<256(the block size).

I actually tested and there is a sligthly increase of 9 gas on your suggestion.

[PaulRBerg]

You're right, I did not realize that. My suggestion is more expensive.

May I at least suggestion we swap our the places for cliffTime and startTime?

So startTime to appear after sender, and cliffTime after token, like this:

struct Stream {
    uint256 depositAmount;
    uint256 withdrawnAmount;
    address sender;
    uint64 cliffTime;
    IERC20 token;
    uint64 startTime;
    uint64 stopTime;
    bool cancelable;
}

[PaulRBerg]

This has been addressed by #132. Locking.