Strategies for lowering the size of the SablierV2LockupPro contract

[PaulRBerg]

Problem

The current size of the SablierV2LockupPro is ~25.49kB, which is ~0.914kB higher than the contract code size limit of ~24.576kB implemented in EIP-170.

To be able to deploy this contract to mainnet, we have to get it under the limit.

Potential Solutions

There is no silver-bullet solution - we will have to be creative and employ a mixed approach. Below I have suggested a few potential solutions, ordered by desirability and implementation difficulty.

1. Move createWithDurations in the periphery

It is not necessary to keep the createWithDurations function in the core contracts - all this function does is to compute the parameters for the createWithRange function dynamically using the block.timestamp at the time of including the transaction in the block.

Similar to the createMultiple function(s) that we plan on implementing in the periphery, we could move this function over there.

2. Move the getStreamed helpers in an external library

As suggested by @andreivladbrg in this comment, we could move the getStreamed helpers in a separate library:

• getStreamedAmountForMultipleSegments
• getStreamedAmountForOneSegment

As I said in that other conversation, one other reason I like this idea is that it makes it possible to perform the pro calculations from outside of the pro contracts - which might be helpful for integrators, or in our periphery contracts.

3. ERC-721 Implementation with Custom Errors

This would reduce the bytecode significantly, but unfortunately there is no good, off-the-shelf implementation of ERC-721 that uses custom errors. I have asked about this on Twitter, and I got no answer.

OpenZeppelin has opened EIP-6093, which purports to retroactively add custom errors to the popular token standards ERC-20, ERC-721, and ERC-1155. However, the EIP is still in draft, and they are still working on implementing it in their own contracts library.

Therefore, the only workaround in the short term would be to implement this ourselves in prb-contracts.

4. Move the getters to the periphery

The idea is to move all the static get functions (that is, all except the calculation functions) to the periphery.

The counter-argument to this is that it would deteriorate the developer experience for integrators, and really anyone else who'd like to query the data from the core contracts directly, e.g. Etherscan users.

5. Remove flash loan support

See the discussion I opened here:

#278

6. Merge the getRecipient function with ownerOf

We have a function getRecipient that does the same thing as the ERC-721's ownerOf function.

We provide the former function to provide context to our users (so we have an on-chain reference to who is the "recipient of the stream"). But we might have to jettison this function to reduce the size of the pro contract .. after all, the vast majority of Sablier users will use a UI to access the protocol. The UI can make the translation for the user, and refer to the NFT owner as the recipient.

Update: the bytecode saving for doing this would be a paltry ~0.05kB. See my comment below.

7. Delete the renounceAdmin function

This wouldn't save much gas, but the renounceAdmin of the Adminable contract is technically redundant. We could refactor the Adminable contract that has no renounceAdmin but which contains only a transferAdmin function that accepts a zero address as an argument.

Update: actually, we should definitely do, see #289.

8. Tune down the optimizer

It just dawned on me that the simplest way to lower the contract size is to lower the number of optimizer runs. This is explained in detail in the Downsizing contracts guide on the Ethereum website, and also in the Solidity docs on the optimizer.

Just by changing the number of optimizer runs to 5,000, I have managed to get the size of the pro contract down to ~24.472kB.

The only caveat to this solution is that it goes against what we want - we would ideally set the optimizer runs to 2^32-1, just like Seaport does, since we expect the Sablier V2 contracts to be called lots and lots of time during their lifetimes (they are singleton contracts, after all - storage and functionality and bundled together).

See related discussion in #290.

[razgraf]

While I take the time to consider the other points, I will say I find points 2 and 4 nice. I don't think you remember, but our initial conversation around the periphery contracts was to have them become "helper contracts". These would get us additional info we need from the main contracts. Anything from simple getters (getStreamedTokenSymbol/Address/ERC20) to data aggregators (getTotalAmountStreamedWithToken).

Therefore, I think we should keep the "actions" (re. point 1) in the main contracts, while moving any non vital methods in a separate contract. For context, keeping important actions in the same place has multiple advantages: events/errors are emitted by the same contract, integrators have a more limited number of endpoints they have to consider etc.

[PaulRBerg]

to have them become "helper contracts"

That plan was sort of broken apart by the necessity of access control - as discussed elsewhere (e.g. in #264), certain functions simply cannot be moved in the periphery because of the need to check who is the caller.

I find points 2 and 4 nice.

2 is cool, but 4 is not.

Moving the getters in the periphery would significantly increase the gas costs for accessing basic information about streams, which should be needed by most users. There would have to be an external call to the getStream function, and then a specific struct field would have to be accessed. Loading the struct would also cost a non-trivial amount of gas.

I think we should keep the "actions" (re. point 1) in the main contracts

I don't think that this distinction is useful. By the same logic, we would define the createMultiple functions in the core contracts, because they are "actionful".

I think we should optimize by (1) necessity and (2) gas costs and (3) user experience.

events/errors are emitted by the same contract

Sure, but the createWithDuration functions don't emit any events. It's only the internal _createWithRange function which does that.

integrators have a more limited number of endpoints they have to consider

This point is not consistent with the rest of your reply - moving such core data functions like getAsset or getSender to the periphery would necessarily demand all integrators to consider the periphery contracts in addition to the core contracts. Whereas if we just move the createWithDuration function there - for instance - only users interested in (i) creating streams and (ii) creating them dynamically via block.timestamp would have to mind the periphery. This is similar to how with Uniswap, one has to use the router to make trades.

[razgraf]

That plan was sort of broken apart by the necessity of access control

You missed my point. I was only referring to public view methods in the initial suggestion for a "helper". Those require no access control.

Moving the getters in the periphery would significantly increase the gas costs

Fair point.

By the same logic, we would define the createMultiple functions

Definitely not the same logic. createMultiple is not a core-methods. It is built on top of the core create method, with just an extra for loop and maybe some extra checks. I was strictly referring to core "actions" e.g. createWithX, cancel etc.

Sure, but the createWithDuration functions don't emit any events

True, no events, but errors for sure. Line 218 contains a call to the fee helper which in turn reverts for incorrect fees.

would necessarily demand all integrators to consider the periphery contracts

Fair point. Although, having access to a single getter such as getStream would still give them all the necessary details about the stream, without us having to provide granular getters. If they really need those granular getters, yes, they could turn to the periphery, but in most cases having the full struct would suffice, no?

[PaulRBerg]

Replies

I was only referring to public view methods in the initial suggestion for a "helper"

Oh, I see. Thanks for explaining.

createMultiple is not a core-method
I was strictly referring to core "actions" e.g. createWithX, cancel etc

Well, as I have explained in point 1 in the OP, I don't see why the createWithDuration has to be classified as "core". After all, what is and isn't a core method is a classification decision that we make.

True, no events, but errors for sure

So what, the periphery will also emit errors. The Helpers.sol library can be imported in the periphery to make that call.

but in most cases having the full struct would suffice

Well, it would suffice, but that does not mean it's a great solution. The point with the granular getters it to please all possible integration use cases - for instance, someone integrating Sablier for NFT collateralization purposes might be especially interested in the depositAmount and the isCancelable methods, and might wish to only make these queries. Refer to the code snippet below to see the magnitude of the gas costs I am referring to here.

Gas Comparison

It costs ~14k more gas to read the startTime field via the external contract Bar, as opposed to reading it directly from the core contract Foo. The explanation is simple - ABI-encoding and decoding a struct is expensive.

Click to see code snippet that shows gas comparison

pragma solidity >=0.8.17;

struct Stream {
    uint128 depositAmount;
    uint128 withdrawnAmount;
    address sender;
    uint40 startTime;
    uint40 cliffTime;
    bool isCancelable;
    uint40 stopTime;
    address asset;
}

contract Foo {
    uint256 public nextStreamId;
    mapping(uint256 => Stream) internal _streams;

    constructor() {
        nextStreamId = 1;
    }

    function addStream(
        uint128 depositAmount,
        uint128 withdrawnAmount,
        address sender,
        uint40 startTime,
        uint40 cliffTime,
        bool isCancelable,
        uint40 stopTime,
        address asset
    ) external {
        _streams[nextStreamId] = Stream({
            depositAmount: depositAmount,
            withdrawnAmount: withdrawnAmount,
            sender: sender,
            startTime: startTime,
            cliffTime: cliffTime,
            isCancelable: isCancelable,
            stopTime: stopTime,
            asset: asset
        });
        nextStreamId += 1;
    }

    function getStream(uint256 streamId) external view returns (Stream memory stream) {
        stream = _streams[streamId];
    }

    // 24,104 gas
    function getStartTime(uint256 streamId) external view returns (uint40 startTime) {
        startTime = _streams[streamId].startTime;
    }
}

contract Bar {
    Foo public foo;

    constructor(Foo foo_) {
        foo = foo_;
    }

    // 38,341 gas
    function getStartTime(uint256 streamId) external view returns (uint40 startTime) {
        Stream memory stream = foo.getStream(streamId);
        startTime = stream.startTime;
    }
}

[razgraf]

Oh, the gas cost alone makes it valuable to have those getters, I take back that point.

[PaulRBerg]

I just added a new idea to the list, which is to delete the getRecipient function.

Update: I implemented this on the prb/recipient-owner-of branch to see what kind of bytecode saving we would get, and unfortunately we wouldn't get much.

• Original: 25.49kB
• Updated: 25.434kB
• Diff: 0.056kB

For ~0.05kB, I think that keeping the getRecipient function is well worth it. The clarity and specificity in nomenclature is helpful.

[andreivladbrg]

1. Move createWithDurations in the periphery

Didn't think about this but you are right, we don't need this function in the core.

True, no events, but errors for sure. Line 218 contains a call to the fee helper which in turn reverts for incorrect fees.

To address @razgraf's concern, we can call the createWithRange external function in the createWithDurations periphery function, which has the same errors.

2. Move the getStreamed helpers in a library

As I said before, this is a MUST.

3. ERC-721 Implementation with Custom Errors

I've always been pro to this idea, if you remember I made a PR in your repo. This will also solve other issues, like the _isApprovedOrOwner function (the size will decrease even more).

4. Move the getters to the periphery

I would suggest leaving this idea as a last resort, given the gas differences.

5. Remove flash loan support

As I said in the other discussions, I don't think this is a good idea.

6. Merge the getRecipient function with ownerOf

I don't think this is possible. The getRecipient function is needed in the SablierV2Lockup contract which does not inherit ERC721.

More potential solutions

• Move as much logic as possible into libraries and declare those function as external, otherwise they will be inlined, this is required only for the non-constant functions. I am even thinking of getting rid of _createWithMilestones internal function.
• Use functions instead of modifiers.

@PaulRBerg I recommend you to take a look to aave's repo to see how they implemented the libraries system. They barely have actual logic in the contracts.

[PaulRBerg]

Thanks @andreivladbrg for your feedback and ideas. I am glad to hear you like the idea of moving the createWithDurations function to the periphery.

we can call the createWithRange external function in the createWithDurations periphery function

I mean, it's not that we "can" - we have to call the external function createWithRange in this context. This would slightly increase the gas cost, though not more than some of the other proposed solutions (e.g. moving the getters in the periphery).

this is a MUST.

Very few things in life are a "must" - this is definitely not one of those cases. E.g. if we are to write an ERC-721 implementation that uses custom errors, then we can keep the calculation functions in the pro contract.

I would suggest leaving this ideea as a last one

Agree.

[flash loan support] I don't think this is a good idea.

Okay.

I don't think this is possible. The getRecipient function is needed in the SablierV2Lockup contract which does not inherit ERC721.

Actually, ISablierV2Lockup inherits from IERC721Metadata, which in turn inherits from IERC721, so the ownerOf function should be available. But we can just make SablierV2Lockup inherit from ERC721, if need be.

Move as much logic as possible into libraries and declare those function as external,

I don't like this approach, not at all. I will explain why in a separate discussion.

Use functions instead of modifiers

Interesting!

I recommend you to take a look to Aave's repo to see how they implemented the libraries system.

Thanks, I'll take a look.

[PaulRBerg]

Looks like we have a winner with moving createWithDurations and createWithDeltas to the periphery!

I removed these functions locally and re-run foundry build --sizes (on the optimized profile), and doing this has reduced the size of the pro contract to ~24.59kB, which is just a tad above the limit, though if we slightly tune down the number of optimizer runs, that will get us below.

[PaulRBerg]

I just added a new idea to the list, which is to tune down the number of optimizer runs.

[PaulRBerg]

I have spent a few hours today benchmarking the gas optimizations we various numbers of optimizer runs (1k, 5k, 10k, 50k, and 4,294,967,295, the maximum allowed), and, based on the findings reported here, I can confidently say that tuning down the optimizer runs is the way to go to reduce the size of the pro contract.

Between 5k runs (a number of optimizer runs that would get the size of the contract down to 24.438kB) and 4,294,967,295, there is an average gas saving of only ~1,751 (that is, for the fuzzed calls made to the withdraw function). Therefore, refactoring the pro contract is simply not worth it - we could just reduce the number of optimizer runs and get approximately the same gas performance.

I think that this is a fair price to pay for our ability to get to mainnet sooner than later and peace of mind regarding the pro contract (fewer refactors means fewer changing assumptions). Also, this gas cost is negligible on non-Ethereum chains.

Do you agree, @andreivladbrg?

[andreivladbrg]

Yes, we should lower the optimizer runs to solve the size issue.

Just to make sure, for the linear contract we will keep the optimizer runs to the highest number is possible, right?

Regarding the .gas-snapshot, there is absolutely no way those reports to be near accurate(more than 300k gas for the withdraw function). Foundry has to solve this.

My guess is that the gas savings will be much lower than "~1,751".

I think to make an accurate comparison we should create a branch where we have a single foundry profile (default), with via_ir = true and a completely new test dir(delete the actual tests) where we simply call each function, cancel create withdraw, with non-reverting inputs.

[PaulRBerg]

Thanks for confirming.

for the linear contract we will keep the optimizer runs to the highest number is possible, right?

No, we won't. The linear and the pro contracts have many common dependencies - compiling them with a different number of optimizer runs would result in different bytecodes for those dependencies, which will be weird.

no way those reports to be near accurate

Those reports, obviously, do not represent only the cost of calling the withdraw function alone. They include the set-up as well (create stream etc.). But this doesn't preclude us from making a decision based off these data.

that the gas savings will be much lower than "~1,751"

Maybe, but this doesn't change our decision. If the gas optimization is even lower, then we can set the optimizer runs to 5k and call it a day, knowing that we wouldn't save much gas anyway had the optimizer runs been set to 2^32-1.

I think to make an accurate comparison

Not necessary, as per the points above.

[andreivladbrg]

Most likely it won't change our decision but conceptually I think we should definitely take our decision based on correct data sets.

Not necessary, as per the points above

Can you explain what you mean by this? We don't need to do this or it won't provide us accurate informations?

[PaulRBerg]

correct data sets.

If something is inaccurate, it does not follow that it is incorrect. The data sets are correctly generated.

Can you explain what you mean by this?

Sure.

We already have all the information we need about the potential gas optimization of tuning up the optimizer runs - more accurate gas reports are no longer needed.

[PaulRBerg]

Locking, ended up reducing the number of optimizer runs to 5,000 in this commit:

7c0bada