Comptroller design: a single contract to store all logic related to fees

[andreivladbrg]

I'm opening this discussion as a follow-up to the decision to implement a single contract (Comptroller) to manage all fee logic across our protocols — airdrops, flow, and lockup.
See: sablier-labs/lockup#1219

(@maxdesalle tagging to get notified about this — as it will be relevant for you since all fees across campaigns, lockup, and flow will be centralized in one place. That means only one collectFees per chain. No need to dig into the technical details below)

Current Airdrops Design: fee charged in claim

Reference: sablier-labs/airdrops#66

TLDR: We store an oracle and a minimum fee in USD, then convert it to wei at claim time — both in the factory and deployed campaigns. Key points:

• Factory: Fee can be changed freely (e.g. $0 → $3 → $5 → $3)
  • Custom fees are stored in the factory
• Campaigns: Fee can only decrease post-deployment
  • It can be either the default or the custom fee set during deployment

Note: with the Comptroller, we will have a greater flexibility, which is a plus.

Lockup and Flow: fee charged in withdraw

In these protocols, fees can be dynamic throughout the stream lifecycle. For example, a stream might start with a $0 fee, and the min fee could be increased later — unlike airdrops, where the fee can't be change if is zero at deployment time.

To match the airdrops approach, the initial idea was to support custom fees for stream senders. But there's a problem:

If a stream is non-cancelable, the recipient is guaranteed all the funds deposited. This makes the sender role less relevant. A user could set a sender address which has a lower (or zero) fee, so it's bypassing the default withdraw fee.

Note: This isn’t an issue in airdrops because the msg.sender is used to get the fee at deployment. In streams, stream.sender might differ from msg.sender.

Possible Solutions

1. Simplest: accept the possible losses (similar to batch withdrawals)
2. Don’t support custom fees for stream senders — only support this business model in airdrops.
3. Set msg.sender as stream.sender (might be consequences in MerkleLockup campaigns) not a good solution, see below
4. Store both sender and streamCreator in the stream data

lmk if you come up with better solutions

Based on what we decide here, in the Comptroller we would need to implement these functions:

• calculateMinFeeWeiForCampaignCreator(address campaignCreator) - used in claim
• calculateMinFeeWeiForSender(address sender) - used in withdraw

Other Question

Given our plan is to charge: (i) $1 on withdraw (ii) $3 fee on claim

Should the Comptroller have:

• Two variables: minFeeUSDClaim and minFeeUSDWithdraw, or
• One variable: minFeeUSD, set as $1 with a $3 passed in our UI for claims?

Since getting the Merkle proof isn't easy for users, a single minFeeUSD might be enough.

@sablier-labs/evm @razgraf would appreciate your input. This is priority 0, as the audit is approaching.

[smol-ninja]

Thanks for starting the discussion Andrei.

There is one more question:

How do we handle a user's request for custom fee in both airdrop and lockup?

In that case, we will have to whitelist his msg.sender in airdrops and his campaign's initialAdmin in Lockup. A better solution would be to whitelist initialAdmin in both campaign and Lockup (or in comptroller if we implement that), so that it can be applied globally.

Though it can lead to exploitation of fees but its very unlikely. Especially in case of campaign, where there will always be some funds to clawback. So, I am beginning to question whether custom fee based on msg.sender in airdrops is a good approach.

Set msg.sender as stream.sender

It has consequences in BatchLockup as well. So not a good idea.

Don’t support custom fees for stream senders

I think this makes the most sense meanwhile. We can keep the fee fixed at $1 or $0.5 in Lockup and charge premium in the UI whenever we want.

Since getting the Merkle proof isn't easy for users

Since IPFS is public goods and given there is AI, it would now be easy to construct merkle proof and bypass the fee. I don't think we should have just one variable minFeeUSD. Also, if fee becomes a problem in Lockup, we might also set it to 0 in Lockup but have $3 in Airdrops. So, I think we should have minFeeUSDClaim and minFeeUSDWithdraw.

Comptroller

What do you think about calling it SablierFeeController instead of Comptroller? I suppose it does not have any other logic than fee, right?

with the Comptroller, we will have a greater flexibility, which is a plus

Will it change the fee logic used in Airdrops / Campaigns?

[andreivladbrg]

How do we handle a user's request for custom fee in both airdrop and lockup?

we can have a setCustomFeeCampaignsAndStreams(uint256 campaignFee, uint256 streamFee) function, which fixes this easily

In that case, we will have to whitelist his msg.sender in airdrops and his campaign's initialAdmin in Lockup. A better solution would be to whitelist initialAdmin in both campaign and Lockup (or in comptroller if we implement that), so that it can be applied globally.

i'm sorry, i didn't understand this

It has consequences in BatchLockup as well. So not a good idea.

right, not a good solution

I think this makes the most sense meanwhile. We can keep the fee fixed at $1 or $0.5 in Lockup and charge premium in the UI whenever we want

wait, with the custom fee, the idea is mainly to allow no fee, or a lower fee, for customers who pay — not the other way around.
thus, if we don't have a custom fee, the streams would require the recipient to pay a fee

i didn't mention this in the OP, even though one non-cancelable stream is created with a different stream. the recipient would still need to withdraw via a different UI or Etherscan to bypass the fee taken in the app.
although technically still possible to bypass it, it requires multiple steps, which should be taken. and, it applies only to non-cancelable streams, which might not be created very often. we need an answer here

Since IPFS is public goods and given there is AI

fair enough, we can have both. also, better to differentiate

What do you think about calling it SablierFeeController instead of Comptroller? I suppose it does not have any other logic than fee, right?

i personally like Comptroller, but i don't have a strong opinion.

Will it change the fee logic used in Airdrops / Campaigns?

the flow remains the same, but it would query the comptroller for the min fee in wei instead of implementing the logic.
and ofc, the comptroller address will be stored in the factory

[smol-ninja]

we can have a setCustomFeeCampaignsAndStreams(uint256 campaignFee, uint256 streamFee) function, which fixes this easily

Yes. But the function will need address of both msg.sender in campaign and sender (or whatever) for stream. So, these will be like two different addresses for the same user. I was talking about one user one address whitelist.

i'm sorry, i didn't understand this.

With the current approach, we would have to whitelist two addresses for each user (unless initialAdmin == msg.sender of campaign). Does this make sense now?

wait, with the custom fee, the idea is mainly to allow no fee, or a lower fee, for customers who pay

Ooh right.

[andreivladbrg]

Yes. But the function will need address of both msg.sender in campaign and sender (or whatever) for stream. So, these will be like two different addresses for the same user. I was talking about one user one address whitelist.
With the current approach, we would have to whitelist two addresses for each user (unless initialAdmin == msg.sender of campaign). Does this make sense now?

since we still want to avoid increasing the fee after a campaign is deployed, we will still need minFeeUSD in the Merkle:

MerkleBase

uint256 public minFeeUSD; 
constructor(address campaignCreator) {
     minFeeUSD = FACTORY.comptroller().getMinFeeUSDForCampaignCreator(campaignCreator);
}
function claim() public {
     // pass the minFeeUSD stored
     uint256 minFeeInWei = FACTORY.comptroller().calculateMinFeeWei(minFeeUSD);
}

but in:

Lockup and Flow

function withdraw(uint256 streamId) {
      // pass the sender
     uint256 minFeeInWei = comptroller.calculateMinFeeWeiForSender(_streams[streamId].sender);
}

it makes the most sense to me to go with this approach

[andreivladbrg]

To add an interface, it would most likely looks like this:

Note

It's important to have as many combinations as possible, so that we can update the comptroller in the factory, lockup, and flow contracts without deploying a new version of these contracts

State variables

• _minFeeUSDClaim
• _minFeeUSDWithdraw
• _campaignCreatorsCustomFeesUSD
• _sendersCustomFeesUSD

Getters

• getMinFeeUSDClaim()
• getMinFeeUSDWithdraw()
• getCustomFeeUSDForCampaignCreator(address who)
• getCustomFeeUSDForSender(address who)

Calculation functions

• calculateMinFeeWeiClaim() - default
• calculateMinFeeWeiClaim(uint256 campaignMinFeeUSD)
• calculateMinFeeWeiWithdraw()
• calculateCustomFeeWeiCampaignCreator(address who)
• calculateCustomFeeWeiSender(address who)

Setters

• setMinFeeUSDClaim(uint256 minFeeUSDClaim)
• setMinFeeUSDWithdraw(uint256 minFeeUSDWithdraw)
• setMinFeeUSDClaimAndWithdraw(uint256 minFeeUSDClaim, uint256 minFeeUSDWithdraw)
• setCustomFeeUSDForCampaignCreator(address who, uint256 minFeeUSD)
• setCustomFeeUSDForSender(address who, uint256 minFeeUSD)
• setCustomFeeUSDForCampaignCreatorAndSender(address who, uint256 minFeeUSDCampaignCreator, uint256 minFeeUSDSender)
• disableCustomFeeUSDForCampaignCreator(address who)
• disableCustomFeeUSDForSender(address who)
• disableCustomFeeUSDForCampaignCreatorAndSender(address sender, address campaignCreator)

lmk if you agree

[andreivladbrg]

• Instead of having all variables in one file, I would recommend to place variables specific to Lockup into a separate abstract contract and same with Flow and Airdrops. _minFeeUSDWithdraw can be different for Lockup and Flow. Or something like Lockup.minFeeUSD through library. I will let you decide whats the best approach here but I think it would be more helpful this way.

i am not sure how libraries would work here - they don't have storage - we don't want constants/immutables for the minFeeUSD.

and if we declare them in the lockup or flow, we would need setters, and we’d go back to the root problem with size that we faced in lockup

Instead of adding all combinations, which can make it look very crowded, how about we add a batch function here?

have thought about this — i don’t think it’s a good idea, as these are admin-only functions, which will be called via safe. IMO it’s better to have actual functions here

[smol-ninja]

which will be called via safe

OK then we can batch them always. Why do we need all combinations then?

i am not sure how libraries would work here

Not libraries, but structs. Basically, we need variables per contract otherwise we wont be able to set different fees.

[smol-ninja]

FeeStruct public lockupFee;
FeeStruct public FlowFee;
FeeStruct public AirdropFee

Like this where FeeStruct contains all required variables.

[andreivladbrg]

oh i see, you are talking about storage layout in comptroller grouped in namespaces, and not about public libraries VestingMath and Helpers

yeah, it makes sense — does this look good:

struct CustomFeeUSD {
    bool enabled;
    uint256 fee;
}

struct FeeLockup {
    uint256 minFeeUSD; // default
    mapping(address sender => CustomFeeUSD) customFeeForSender; // custom fee for sender
}

[smol-ninja]

Yeah this is what I am talking about.

[andreivladbrg]

looks like there aren’t so many non-cancelable streams:

[andreivladbrg]

@razgraf has suggested on a call today — what if we whitelist tokens instead of addresses to facilitate custom fees?

this makes sense with new tokens used for airdrops, but IMO with streams and big tokens it doesn't work that well

(sorry for the spam)

[smol-ninja]

Yeah I agree with your rationale.

sorry for the spam

Its not spamming, its discussing :))