feat: add SablierMerkleInstant

[smol-ninja]

Changelog

• Closes Implement SablierMerkleInstant - A merkle lockup contract to facilitate instant airdrop campaigns #995
• Refactor ISablierMerkleLockup to ISablierMerkleBase
• Refactor MerkleLockupto MerkleBase
• Refactor ISablierMerkleLockupFactory to ISablierMerkleFactory
• rename merkle-lockup directory to merkle-campaign
• rename MerkleLockup_Integration_Test contract to MerkleCampaign_Integration_Test. In a separate PR, I will DRY'ify the shared test.
• fix event assertion in merkleLL and merkleLT fork tests, similar to missing expectEmit in MerkleLL and MerkleLT fork tests v2-periphery#382.
• I have used Merkle Lockup instead of MerkleLockup in natspecs to represent MerkleLL and MerkleLT.

PR Dependencies

• refactor: remove V2 and obsolete references #994

[smol-ninja]

@andreivladbrg feel free to review it. Sorry that the PR has become huge due to refactoring of Lockup contract.

I will be working on #1002 to dry'fy the shared tests across merkle contracts.

[andreivladbrg]

Great work, and sorry for the delayed review.
I haven't looked at tests yet, but I will leave this review for src

We can dry-fy the claim functions by implementing a _claim function in the base contract:

Claim function

    function _claim(uint256 index, address recipient, uint128 amount, bytes32[] calldata merkleProof) internal {
        // Check: the campaign has not expired.
        if (hasExpired()) {
            revert Errors.SablierMerkleBase_CampaignExpired({ blockTimestamp: block.timestamp, expiration: EXPIRATION });
        }

        // Check: the index has not been claimed.
        if (_claimedBitMap.get(index)) {
            revert Errors.SablierMerkleBase_StreamClaimed(index);
        }

        // Generate the Merkle tree leaf by hashing the corresponding parameters. Hashing twice prevents second
        // preimage attacks.
        bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(index, recipient, amount))));

        // Check: the input claim is included in the Merkle tree.
        if (!MerkleProof.verify(merkleProof, MERKLE_ROOT, leaf)) {
            revert Errors.SablierMerkleBase_InvalidProof();
        }

        // Effect: set the `_firstClaimTime` if its zero.
        if (_firstClaimTime == 0) {
            _firstClaimTime = uint40(block.timestamp);
        }

        // Effect: mark the index as claimed.
        _claimedBitMap.set(index);
    }

Or, we can even implement the claim external function in Base contract, and add basically all the logic from SablierMerkleInstant.claim expect for the token transfer.
And at the end of the function, it will call an internal virtual function _claim that is going to be implemented by the child contract. In case of SablierMerkleInstant it will simply transfer the tokens, in case of Merkle Lockup, it would create a stream.

The pros:

• the code is muchh clearner

The cons:

• no returned stream id
• the CEI is not respected in MerkleLT (still safe tho)

Curious about your opinion on this @smol-ninja .

[smol-ninja]

Thanks @andreivladbrg for the review. I like your second approach to implement the claim function in Base contract and override _claim in the child contracts. As a consequence of this, we can also move claim tests into the shared folder in
dry-ify the merkle tests PR, which is not possible with the first approch.

[smol-ninja]

After giving it more thought, I realized that in the second approach, there would neither be a stream ID returned nor would there be one in the Claim event (as you also mentioned). And isn't it useful to have stream ID at these places (for integrators and smart contract calls)?

As for the first approach, we can move these two lines to the base contract:

1. bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(index, recipient, amount))))
2. _claimedBitMap.set(index)

and it wouldn't affect the CEI pattern that much because there is anyway one more Effect (_firstClaimTime = uint40(block.timestamp)) in _checkClaim. However, the current structure looks better to me:

1. You generate leaf
2. You verify using index, leaf and merkleProof (these are very merkle tree specific data) through checkClaim
3. You set the bit
4. take action

I am happy to move the leaf to _checkClaim and wouldn't require us to rename it; OR
I can move both the lines to _checkClaim and rename it to _checkClaimAndSet` (???); OR
take no action (if you also agree)

[andreivladbrg]

And isn't it useful to have stream ID at these places (for integrators and smart contract calls)?

not that much, as it already emitted in the create event

and it wouldn't affect the CEI pattern that much

the first suggestion doesn't affect the CEI at all, i was talking about the second one

_checkClaimAndSet

that's exactly what the first suggestion is about

[andreivladbrg]

one other aspect about the second suggestion, we can include a bytes param, in case other future version of Merkle contract would need extra params, but for now it would only be ignored

    function claim(
        uint256 index,
        address recipient,
        uint128 amount,
        bytes32[] calldata merkleProof,
        bytes memory data // optional data
    )
        external
    {
        // claim logic

        _claim(index, recipient, amount, merkleProof, data);
    }

    function _claim(
        uint256 index,
        address recipient,
        uint128 amount,
        bytes32[] calldata merkleProof,
        bytes memory /* data */ // ignore the data
    )
        internal
    { }

do you find it useful? the idea is to not modify the future API version

[smol-ninja]

not that much, as it already emitted in the create event

But then why do we have returned value in batch lockup functions and even the create functions in lockup contracts. I suppose its so that if a contract (integrator) calls them on behalf of the users, they can use return value to keep track of the streams. Smart contracts don't have access to events so its beneficial to them. Same argument in favour of having stream ID returned in claim.

[andreivladbrg]

Smart contracts don't have access to events so its beneficial to them

yeah, but the point you've raised was about the event, wasn't it?
the returned value problem i've already mentioned in the cons of the proposal :))

i don't want to deny the con, but i highly doubt that the merkle-campaign are meant to be integrated at the contract level, as we've built them to have an e2e product for your UI

also, it is not the end of the world, as the integrators can still can obtain the id:

uint256 streamId = merkleLL.LOCKUP_LINEAR().nextStreamId();
merkleLL.claim(params);

[smol-ninja]

yeah, but the point you've raised was about the event, wasn't it?

Actually both: even and return value. But mostly return value.

also, it is not the end of the world, as the integrators can still can obtain the id:

Haha. Now, I am convinced that we should DRY'ify the claim function. Taking your 2nd approach and remove stream ID from Claim event and return value. Should we create a separate issue for it and hear what @PaulRBerg has to say, in case he disagrees with this?

one other aspect about the second suggestion, we can include a bytes param, in case other future version of Merkle contract would need extra params, but for now it would only be ignored

About this, I am not in favour of adding bytes param when we don't need it. It would be entirely redundant and we don't even know when we will have the need for it.

[andreivladbrg]

The tests look good 👍

Before merging this to staging, let's first create a PR against this one with the dry-fy shared one

[smol-ninja]

Before merging this to staging, let's first create a PR against this one with the dry-fy shared one

The PR is already open for review: #1009

[smol-ninja]

Let me know if all looks good in this PR. If so, could you please approve it?

[andreivladbrg]

including the changes made in #1011 it looks good