Getting rid of the core <> periphery terminology

[PaulRBerg]

Since the addition of MerkleInstant contract, I wonder if the core <> periphery still makes sense?

The MerkleInstant contract holds the airdrop funds, which are never deposited in any core contract.

What do you think, @sablier-labs/solidity?

[andreivladbrg]

Just to confirm, is this what you are suggesting?

From:

Click to see tree

src/
├── core
│   ├── LockupNFTDescriptor.sol
│   ├── SablierLockupDynamic.sol
│   ├── SablierLockupLinear.sol
│   ├── SablierLockupTranched.sol
│   ├── abstracts
│   │   ├── Adminable.sol
│   │   ├── NoDelegateCall.sol
│   │   └── SablierLockup.sol
│   ├── interfaces
│   │   ├── IAdminable.sol
│   │   ├── ILockupNFTDescriptor.sol
│   │   ├── ISablierLockup.sol
│   │   ├── ISablierLockupDynamic.sol
│   │   ├── ISablierLockupLinear.sol
│   │   ├── ISablierLockupRecipient.sol
│   │   └── ISablierLockupTranched.sol
│   ├── libraries
│   │   ├── Errors.sol
│   │   ├── Helpers.sol
│   │   ├── NFTSVG.sol
│   │   └── SVGElements.sol
│   └── types
│       └── DataTypes.sol
└── periphery
    ├── SablierBatchLockup.sol
    ├── SablierMerkleFactory.sol
    ├── SablierMerkleInstant.sol
    ├── SablierMerkleLL.sol
    ├── SablierMerkleLT.sol
    ├── abstracts
    │   └── SablierMerkleBase.sol
    ├── interfaces
    │   ├── ISablierBatchLockup.sol
    │   ├── ISablierMerkleBase.sol
    │   ├── ISablierMerkleFactory.sol
    │   ├── ISablierMerkleInstant.sol
    │   ├── ISablierMerkleLL.sol
    │   └── ISablierMerkleLT.sol
    ├── libraries
    │   └── Errors.sol
    └── types
        └── DataTypes.sol

To:

Click to see tree

src/
├── LockupNFTDescriptor.sol
├── SablierLockupDynamic.sol
├── SablierLockupLinear.sol
├── SablierLockupTranched.sol
├── SablierBatchLockup.sol
├── SablierMerkleFactory.sol
├── SablierMerkleInstant.sol
├── SablierMerkleLL.sol
├── SablierMerkleLT.sol
├── abstracts
│   ├── Adminable.sol
│   ├── NoDelegateCall.sol
│   ├── SablierLockup.sol
│   └── SablierMerkleBase.sol
├── interfaces
│   ├── IAdminable.sol
│   ├── ILockupNFTDescriptor.sol
│   ├── ISablierLockup.sol
│   ├── ISablierLockupDynamic.sol
│   ├── ISablierLockupLinear.sol
│   ├── ISablierLockupRecipient.sol
│   ├── ISablierLockupTranched.sol
│   ├── ISablierBatchLockup.sol
│   ├── ISablierMerkleBase.sol
│   ├── ISablierMerkleFactory.sol
│   ├── ISablierMerkleInstant.sol
│   ├── ISablierMerkleLL.sol
│   └── ISablierMerkleLT.sol
├── libraries
│   ├── Errors.sol
│   ├── Helpers.sol
│   ├── NFTSVG.sol
│   └── SVGElements.sol
└── types
    └── DataTypes.sol

Will there be a common DataTypes.sol and Errors.sol files for both?

[PaulRBerg]

Yes and yes.

[smol-ninja]

Interesting. If you are specifically targeting MerkleInstant contract then in its defence, I can argue that its similar to MerkleLL and MerkleLT. They also hold airdrop funds until user "claims" them.

To @andreivladbrg, an argument against combining core <> periphery into one is that the merkle lockup contracts do not form the lockup protocol. The current periphery is made of two parts:

1. Batch contract: batches create functions of lockup protocol. Since, its tightly integrated with the Lockup's create APIs, I wonder why it should not be a part of the core protocol itself. The core protocol also contains batch functions such as withdrawMultiple, withdrawMaxAndTransfer, and now will include the batch function, so why give batch create a special attention.

2. Airstreams: They are not a part of the Lockup protocol. They have their own system of contracts which can be used to facilitate airdrops either through lockups or instant or some other upcoming system.

Having said that, "Airstreams" protocol deserve its own repo. But if we are interested into keeping them in Lockup repo, a better structure would be the following:

1. Include BatchLockup in the core directory
2. Rename the periphery to airstreams

[PaulRBerg]

whether this breaking change is truly useful

Yes. Otherwise, we wouldn't have implemented package tethering.

My point was that tethering the package is, in itself, a breaking change. Separating the airdrop contracts wouldn’t alter this from a qualitative perspective.

Do you see this now?

rather whether streaming and airstream contracts should be in the same repo or not (since most of them use the streaming contract).

Yes, I understood that. What I meant to say is that it'll be fine to have a separate airdrop repo for the contracts on each chain.

two repos per blockchain.

Yes.

would the total development work vs. the improvement gained be worth it in the short/long term

Yes as per the arguments given by Shub and I throughout this discussion.

an actor will need to “lock up” some funds

Yes. Words have multiple meanings. We should use and promote the meaning that makes most sense.

[andreivladbrg]

My point was that tethering the package is, in itself, a breaking change. Separating the airdrop contracts wouldn’t alter this from a qualitative perspective.
Do you see this now?

i see, but i have mentioned this in my comment from the beginning: "assuming that this change is going to be post 1.3.0" - post the current package tethering version, which implies we would have two breaking changes consecutively (about this, the question about "improvement gained be worth it in the short/long term" is raised)

Yes as per the arguments given by Shub and I throughout this discussion.

yes, i assume you’re referring to these two points:

Yes, but this only applies to 66% of them. In the future, we might also consider the possibility of developing more airdrop contracts that do not rely on lockups (such as https://github.com/sablier-labs/company-discussions/discussions/85). So having a separate repo for Airstreams keep that option open for us.
Instead of organizing them as a monorepo, it would then be restructured as "product oriented repos": lockup, flow and airstream

and my response was:

nevertheless, i am tentatively against this for now, until we reach a number of campaigns that do not use a streaming contract greater than the ones that use them

which wasn’t answered

[PaulRBerg]

Oh, sorry I missed the part about post-1.3.0 — the thing is, however, that this change is indeed to be included in v1.3.0

i am tentatively against this for now, until we reach a number of campaigns that do not use a streaming contract greater than the ones that use them

The fact that some contracts are no longer interacting with Lockup is good enough of a reason to create the separate repo.

Categories are useful even when there are no clear, sharp distinction between them.

So, to go back to the problem in the OP — we have a problem of category formation.

The periphery is no longer a periphery because some contracts are completely independent of the core contracts.

By moving the airdrop contracts to a separate repo, we would get rid of this category problem.

It's fine to put the MerkleLL & co. contracts in the separate repo because they are airdrop contracts. It's just a more natural place for them — the fact that they are airdrop contracts is a more defining characteristic than the fact that they interact with the core contracts.

To give a further example: we also use OpenZeppelin's contracts, but we don't open PRs to move our code there.

[andreivladbrg]

Oh, sorry I missed the part about post-1.3.0 — the thing is, however, that this change is indeed to be included in v1.3.0

Great, then we’re having a different discussion.

I’m happy to say I’ve changed my mind and agree with this. Thanks for re-explaining

[PaulRBerg]

Cheers

[smol-ninja]

So, what do we call our new baby repo? Airstreams? Merkle-Drop? Any ideas?

[PaulRBerg]

airdrops

[PaulRBerg]

Or maybe merkle?

Just like the lockup repo is named after the name of the contracts (Lockup).

[smol-ninja]

merkle is popularly known to represents merkle tree. So the name can be confused with merkle libraries. airdrops is a very generic yet good name. However, the names can be confused with "Sablier doing an airdrop". I guess your argument against airstreams is that this will no longer facilitate only streams. So I agree with you on that.

[PaulRBerg]

the names can be confused with "Sablier doing an airdrop"

Not bad for marketing

[smol-ninja]

Issues

1. Create a separate repo for Airdrops #1072
2. Place SablierBatchLockup and core contracts into src directory #1073