Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ETH fee in withdraw function #1076

Closed
2 tasks done
PaulRBerg opened this issue Nov 5, 2024 · 18 comments
Closed
2 tasks done

ETH fee in withdraw function #1076

PaulRBerg opened this issue Nov 5, 2024 · 18 comments
Assignees
Labels
effort: high Large or difficult task. priority: 1 This is important. It should be dealt with shortly. type: feature New feature or request. work: complex Probe-sense-respond. The relationship between cause and effect can only be perceived in retrospect.

Comments

@PaulRBerg
Copy link
Member

PaulRBerg commented Nov 5, 2024

Context

As discussed here, we want to be able to monetize the Lockup streams by charging an ETH fee, since this is much easier than charging in USDC or the stream's token.

Spec

  • Make all functions payable so that the UI can set a non-zero msg.value.
  • Allow anyone to call withdrawFees function, and always transfer the ETH fees to the contract admin.

Note: compared to the ETH fee in MerkleLockup, the fee in this proposal is optional. The goal is to keep the protocol free but monetize at the UI level — the Sablier UI would introduce a small ETH fee in the transactions signed by users.

@andreivladbrg
Copy link
Member

Since we are talking about an ETH fee, why not charge it in both create and withdraw ? @PaulRBerg

@andreivladbrg andreivladbrg added type: feature New feature or request. effort: high Large or difficult task. work: complex Probe-sense-respond. The relationship between cause and effect can only be perceived in retrospect. priority: 1 This is important. It should be dealt with shortly. labels Nov 5, 2024
@PaulRBerg
Copy link
Member Author

PaulRBerg commented Nov 5, 2024

Because we've decided to charge only recipients.

Also, charging at withdrawing time ensures that we only charge users after the funds have been streamed. Charging at create time may double charge users who create, cancel, and then recreate a stream.

@smol-ninja

This comment was marked as resolved.

@PaulRBerg

This comment was marked as resolved.

@smol-ninja

This comment was marked as resolved.

@andreivladbrg
Copy link
Member

I started working on this, and regarding step 2:

"If it is, add it to a storage variable that accumulates the ETH fee"

it is not required to have a storage variable for this, as there is a native this.balance opcode .

@smol-ninja
Copy link
Member

Can someone make a direct transfer of ETH to the contract? If no, then looks good. If yes, then tracking would be useful for accounting purposes.

@PaulRBerg
Copy link
Member Author

There are several different ways in which ETH can be airdropped in the contract, e.g. SELFDESTRUCT. See this StackExchange Q&A.

So yes, it'd be helpful for accounting purposes to introduce the storage variable. But I wonder if the gas overhead is worth it? We could treat the non-fee-related ETH deposits (which are super unlikely anyway) as just another kind of revenue.

@smol-ninja
Copy link
Member

We didn't add any storage variable to track ETH fee in merkle lockup (didn't realise at that time). So a decision here would affect the logic there as well.

@andreivladbrg
Copy link
Member

So yes, it'd be helpful for accounting purposes to introduce the storage variable

Personally, I don’t find that helpful or worth it enough.
Can’t we emit the fee taken in the event and sum the amounts through the subgraph to calculate the all-time revenue generated through the withdraw function? @razgraf

@PaulRBerg
Copy link
Member Author

PaulRBerg commented Nov 15, 2024

@andreivladbrg I actually agree with you — what follows the part you quoted was supporting your position. The gas overhead isn't worth it.

Good idea to log the ETH fee.

@razgraf
Copy link
Member

razgraf commented Nov 15, 2024

+1 for the fee in an event

@PaulRBerg
Copy link
Member Author

Due to an issue with the batch function, we are unable to emit the fee in an event because calculating would require a huge refactor, which is simply not worth it. We can figure out how much we made in ETH through other means, e.g., Etherscan or subgraphs.

@razgraf
Copy link
Member

razgraf commented Nov 20, 2024

If there's no event, I'm not sure the subgraphs will be able to index it. It might be that we can track msg.value from the transaction attached to the Withdraw event, but it's a workaround.

For context, the main idea behind having those fees as part of the subgraphs is the potential for CSV Exports for accounting purposes (both traditional Fintech and CEXs include fees as part of account statements).

@PaulRBerg
Copy link
Member Author

There must be a way to obtain these msg.value amounts somehow from a transaction hash. Perhaps from the RPC? (this may be needed to show users how much they paid in fees)

We will figure out a way to obtain the CSV exports, though we might not need them. We can take a snapshot of the ETH fee at the end of each month.

@razgraf
Copy link
Member

razgraf commented Nov 20, 2024

There definitely are ways, I was just discussing the subgraph case, since that would be the easiest to implement (since there would be no external calls or external systems required).

@PaulRBerg
Copy link
Member Author

Understood. Unfortunately, the refactor cost is huge, and given the pressure to launch the fees ASAP, I suggest launching without including the fees in the event.

@PaulRBerg
Copy link
Member Author

FYI @sablier-labs/solidity I have updated the spec in the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: high Large or difficult task. priority: 1 This is important. It should be dealt with shortly. type: feature New feature or request. work: complex Probe-sense-respond. The relationship between cause and effect can only be perceived in retrospect.
Projects
None yet
Development

No branches or pull requests

4 participants