feat: add name, introduces struct ConstructorParams

[smol-ninja]

closes #233

Adding name throws stack too deep error because of the large number of variables in function createMerkleStreamerLL. In order to fix that, this PR introduces a new struct ConstructorParams for SablierV2MerkleStreamer constructor parameters.

struct ConstructorParams {
    address initialAdmin;
    IERC20 asset;
    string name;
    bytes32 merkleRoot;
    uint40 expiration;
    bool cancelable;
    bool transferable;
}

Summary

Source

• uses struct ConstructorParams to encapsulate SablierV2MerkleStreamer constructor parameters.
• adds name parameter of type string.
• adds a new internal storage NAME of type bytes32 to store campaign name. bytes32 is preferred over string to optimize for gas.
• adds a public function name() to retrieves the campaign name as string.

Events

• update event CreateMerkleStreamerLL.
• restructures the order of parameters.

Errors

• adds a new error SablierV2MerkleStreamer_CampaignNameTooLong.

Tests

• defines baseParams to retrieve default value of ConstructorParams struct.
• add test_RevertWhen_CampaignNameTooLong to check for name length
• update createMerkleStreamerLL.tree

[andreivladbrg]

Were you getting the stack too deep error in both MerkleStreamer.constructor and MerkleStreamerFactory.create function?

Also, why do we pass a string in the constructor instead of bytes32, wouldn't it be more efficient?

adds a new private storage _NAME of type bytes32 to store name.

smart way of doing this, congrats

I've created a separate branch where I approached things differently. The reason I did not start from your branch is that it would have taken me more time to make the changes.

Could you take a look at my branch and tell me if you consider that approach better? If yes, I will polish it. I personally prefer it because the campaign contract is not modified as much.

[PaulRBerg]

Thanks for the PR, Shub.

It's a shame that this additional parameter forces us to use a struct in the constructor. Structs have caused all sorts of problems for V2 integrators (e.g., Tally).

But I can't see any way out - unless we remove another parameter, which we can't do.

My initial reaction was to suggest you discard this feature, but then I remembered LockupTranched/ LockupDynamic. It is very likely that the implementation of these other contracts will require a struct (because of the segments). So introducing structs in MerkleStreamerLL is fine (and this PR remains open).

[andreivladbrg]

other contracts will require a struct (because of the segments

haven't we decided that the segments would be passed in the claim function, but not in the create factory function?

#166 (reply in thread)

so, there shouldn't be a difference between createMerkleStreamerLL and createMerkleStreamerLD functions, in term of number of parameters

My initial reaction was to suggest you discard this feature

now i feel like it's the right choice 😅

[PaulRBerg]

Oooh, you're right, @andreivladbrg. But at any rate, I guess that other parameters will be needed in the constructor for these other implementations.

Anyway. I am happy to proceed with the implementation brought about by this PR.

[andreivladbrg]

@PaulRBerg have you checked the other branch?

which version do you say it's better?

[PaulRBerg]

I haven't checked the other branch, no. In fact, I haven't even checked this PR properly. I've just seen the struct and wanted to comment on that.

I will review it later.

[smol-ninja]

Thanks @andreivladbrg and @PaulRBerg for the comments. Please find my response below.

Were you getting the stack too deep error in both MerkleStreamer.constructor and MerkleStreamerFactory.create function?

Only in MerkleStreamerFactory.createMerkleStreamerLL function. My initial approach was to only have a struct for create function but then I decided on the current approach due to the reasons I have mentioned above.

why do we pass a string in the constructor instead of bytes32, wouldn't it be more efficient

Yes, it's slightly more optimized to allow bytes32 instead of string in the constructor (~1000-2000 gas). I chose string because I was worried that integrators might make mistakes while converting the string into bytes as it follows a big-endian format in solidity.

For example, using 0x0000000000000000000000000000000041697264726f702043616d706169676e instead of 0x41697264726f702043616d706169676e00000000000000000000000000000000 "Airdrop Campaign".

It might be a wrong assumption though.

I've created a separate branch where I approached things differently

Thank you for this. I am happy to take a look at it tomorrow.

so, there shouldn't be a difference between createMerkleStreamerLL and createMerkleStreamerLD functions, in term of number of parameters

Well there is LockupLinear.Durations streamDurations in createMerkleStreamerLL. Would that be a part of createMerkleStreamerLD too? 🤔

now i feel like it's the right choice 😅

I might agree too. Do we really need a name at contract level? Also, why I pointed out the gas concern in the original issue.

[smol-ninja]

I had a look at your branch @andreivladbrg. It looks good to me. However, I would like to give remarks on the two major differences between this PR and your branch:

1. Data structure

• Including all the factory's createMerkleStreamerLL parameters vs only MerkleStreamerLL constructor parameters in the CreateLL struct

See code snippet

struct CreateLL {
    address initialAdmin;
    string name;
    ISablierV2LockupLinear lockupLinear;
    IERC20 asset;
    bytes32 merkleRoot;
    uint40 expiration;
    LockupLinear.Durations streamDurations;
    bool cancelable;
    bool transferable;
    string ipfsCID;
    uint256 aggregateAmount;
    uint256 recipientsCount;
}

vs 
struct CreateLL {
    address initialAdmin;
    ISablierV2LockupLinear lockupLinear;
    IERC20 asset;
    string name;
    bytes32 merkleRoot;
    uint40 expiration;
    LockupLinear.Durations streamDurations;
    bool cancelable;
    bool transferable;
}

My initial approach was just like yours. But then I stared to make the following assumptions:

1. There could be integrators integrating MerkleStreamerLL directly.
   In that case, the shared parameters between the two APIs would not be the same because of the different name type. If this is something you agree upon, then this PR's approach looks better otherwise we should go with yours.

2. The addition of extra parameters in the MerkleStreamerLT and MerkleStreamerLD constructors such as startTime, segments, tranches.
   If our decision is to include these in the Merkle root and only allow them in the claim function, then I am happy to go with your approach otherwise it makes more sense to have a shared struct parameter between factory and streamer to avoid future stack too deep error.

2. Testing

• Having ISablierV2LockupLinear vs not having ISablierV2LockupLinear in Defaults.sol

After studying the v2-core and v2-periphery codebase, my observation was that all the utility files, except precompiles, are designed to declare default variables that are not dependent on interfaces. Then we have Base.t.sol which imports all the interfaces, import Defaults.sol and declares its default variables which could not be defined in Defaults.sol.

For that reason, I assumed it could break our design consistency if I import interfaces and declare lockupLinear in Defaults.sol. However I agree that I could do better than using name defaultCreateLLParams in MerkleStreamer.t.sol.

If this is something which you do not agree upon, I am happy to accept your changes.

[andreivladbrg]

thanks for your responses @smol-ninja

I chose string because I was worried that integrators might make mistakes while converting the string into bytes as it follows a big-endian format in solidity.

this , and there is one more thing if we keep the check in the constructor, we won't need to make the same check in every createMerklerStreamer function. so i am happy to leave it there

After rethinking about this, I consider now that it would be better to have a struct that cointains only the parameters passed in the abstract contract:

library MerkleStreamer {
    struct ConstructorParams {
        address initialAdmin;
        string name;
        IERC20 asset;
        bytes32 merkleRoot;
        uint40 expiration;
        bool cancelable;
        bool transferable;
    }
}

So that it can be used in all future contracts that inherit MerkleStreamer. Ofc, this needs to tested to see if it really worki in practic (if we no longer have stack too deep error):

function createMerkleStreamerLL(
    MerkleStreamer.ConstructorParams memory params,
    ISablierV2LockupLinear lockupLinear,
    LockupLinear.Durations streamDurations,
    string ipfsCID,
    uint256 aggregateAmount,
    uint256 recipientsCount
)
    external
    returns (ISablierV2MerkleStreamerLL merkleStreamerLL); 

wdyt, do you like this proposal?

@PaulRBerg tagging you again because Shub has asked:

I might agree too. Do we really need a name at contract level?

[smol-ninja]

Great idea @andreivladbrg. I prefer this approach too. This way, we won't have to deal with separate structs for future streamer contracts. I think it should work because it would nonetheless reduce the no. of stack variables for the factory create function.

I would also like to know your comment on my 2nd point: "not having ISablierV2LockupLinear in Defaults.sol".

Whatever decision @PaulRBerg takes on discarding this feature or not, we should still do this because there might be new variables in the factory create versions of LD and LT that would eventually place us into stack too deep error.

If you agree, I am happy to make these changes and update the PR.

[andreivladbrg]

I would also like to know your comment on my 2nd point: "not having ISablierV2LockupLinear in Defaults.sol

well, if we switch to the proposal i've made, we will no longer need the ISablierV2LockupLinear in Defaults.sol

[smol-ninja]

@andreivladbrg I've updated the PR based on our newest proposal. I have also taken some suggestions from your branch.

Let me know what you think.

[andreivladbrg]

looking good now

might be new variables in the factory create versions of LD and LT

well, what are these new variables? maybe they will even be less, we will remove the LockupLinear.Durations struct, but we will add MAX_SEGMENT_COUNT

[smol-ninja]

maybe they will even be less, we will remove the LockupLinear.Durations struct, but we will add MAX_SEGMENT_COUNT

Yes, you are right.

[PaulRBerg]

I like your approach to modularize the constructor params in (i) shared params and (ii) bespoke params for each shape (LL, LT, etc.)

However:

• I would prefer the name of the struct parameter to better reflect the fact that the parameters are shared. Maybe we could rename it to baseParams, sharedParams, commonParams, or something along these lines. Suggestions welcome.
• Why are ipfsCID, aggregateAmount, and recipientsCount not part of the ConstructorParams struct? They seem to be common to all campaigns, not just the LL campaigns.

[PaulRBerg]

We should also write a fuzz test that fuzzes the name.

Or maybe we can incorporate this fuzzing in the existing testFuzz_CreateMerkleStreamerLL test?

[smol-ninja]

Interesting, but why do you want to fuzz name? We have an integration test for name length and since it is constant, as long as it fits into 32 bytes, there can never be a problem.

[andreivladbrg]

Why are ipfsCID, aggregateAmount, and recipientsCount not part of the ConstructorParams struct? They seem to be common to all campaigns

because these parameters are not passed in the constructor and the struct passed is called ConstructorParams and it's within MerkleStreamer namespace, not Factory:

v2-periphery/src/abstracts/SablierV2MerkleStreamer.sol

Line 61 in 17d8d94

constructor(MerkleStreamer.ConstructorParams memory params) {

do you want to add a new library MerkleStreamerFactory as i did in my separate branch, which you can also review?

initially considered the same thing as being better, but as i said in my comment here, i no longer find it to be the better option

[PaulRBerg]

because these parameters are not passed in the constructor

Got it. Makes sense.

My feedback about the name of the struct parameter still stands, though.

Can we rename it to baseParams or commonParams?

Do you want to add a new library MerkleStreamerFactory

No.

[andreivladbrg]

Can we rename it to baseParams or commonParams?

well, not sure if it would be better, since this struct is not within a MerkleStreamerLL or MerkleStreamerLT namespace

i personally consider the current version to be better

@smol-ninja wdyt?

[smol-ninja]

I agree with @andreivladbrg on renaming ConstructorParams. I don't see any of them more relevant than ConstructorParams other than the fact that they are shorter names. I also don't like ConstructorParams but unfortunately I couldn't think of a better name that can express all the parameters of the MerkleStreamer constructor.

[smol-ninja]

@PaulRBerg do you have any suggestions on this? Andrei has already approved the changes.

[PaulRBerg]

I don't think you have understood what I meant. I wanted to rename the name of the struct parameter, not of the struct itself. I.e. here:

v2-periphery/src/SablierV2MerkleStreamerLL.sol

Line 45 in bbf6366

MerkleStreamer.ConstructorParams memory params,

Rename params to baseParams, commonParams, or something like this (whichever you like most).

[smol-ninja]

I agree with that @PaulRBerg. So I've decided to use baseParams to refer to MerkleStreamer.ConstructorParams struct in 712c24c. Kindly let me know if it looks good now.

[andreivladbrg]

shouldn't baseParams name be only in the createMerkleStreamerLL function and in the MerkleStreamerLL?

why does it need to be named "base" in the abstract contract as well?

[smol-ninja]

Yes! You're right @andreivladbrg. That was a mistake.

[PaulRBerg]

LGTM now

[PaulRBerg]

Though the build fails in CI

[smol-ninja]

That's because we updated package.json in #264 without updating bun lockfile. I've now fixed it.

"@sablier/v2-core": "github:sablier-labs/v2-core#staging"