feat: add revenue sharing specs by 0xiamflux · Pull Request #47 · defi-wonderland/specs

0xiamflux · 2025-08-04T16:18:48Z

Adds predeploy spec for Jovian HF.

Closes OPT-973

linear · 2025-08-04T16:18:50Z

agusduha · 2025-08-04T20:52:03Z

+
+A new predeploy contract, `FeeSplitter`, is introduced to manage the distribution of all fees collected on L2.
+Instead of having immutable recipients, the `RECIPIENT` address in each of the four `FeeVault` contracts will
+be set to the `FeeSplitter` contract on new chains, while existing chains will be able to opt-in to use this new predeploy.


All chains should be able to opt-in

Joxess · 2025-08-07T00:02:06Z

+#### `disburseFees`
+
+Initiates the routing flow by withdrawing the fees that each of the fee vaults has collected and sends the shares
+to the appropriate addresses according to the configured percentage.
+
+```solidity
+function disburseFees() external
+```
+
+- MUST emit `FeesDisbursed` event upon successful execution.
+- MUST emit `NoFeesCollected` event if there are no funds available at the time of the call.
+- MUST revert if not enough time has passed since the last successful execution.
+- MUST send the appropriate amounts to the recipients.
+- The balance of the contract MUST be 0 after a successful execution.


Here should be an explanation on the behavior of this function when there are vaults that have a balance below their set setMinWithdrawalAmount, and what NoFeesCollected event means in this context (no fees collected because of minWithdrawalAmount reached or another condition)

That's a pretty good point.

Addressed here.

Joxess · 2025-08-07T00:09:27Z

+
+#### `NoFeesCollected`
+
+Emitted when `disburseFees` is called but there are no funds available in the fee vaults at the time of execution.


I would suggest adding "no funds available to withdraw" to avoid confusion on the fact that the vaults have some funds most of the time (unless the specific fee mechanism is arbitrarily set, as is the case with the operatorFee in most of the chains)

Addressed here.

agusduha · 2025-08-12T13:16:14Z

+- Recipient A
+- Recipient B


We need to rename these names everywhere. We should define something more specific to the usecase, for example: Revenue share recipient and revenue rest recipient

Or something similar

agusduha · 2025-08-12T13:19:01Z

+The function MUST emit the `NoFeesCollected` event if the contract doesn't have any funds after the vaults have been withdrawn,
+which can happen in scenarios where the vaults have a minimum withdrawal amount of 0.
+
+On chains where any fee vault is not to be used and is not expected to maintain a balance in the contract,


I think this sentence is not clear enough

I think it can be removed, since if there is no fees it won't revert but it just won't withdraw anything.

agusduha · 2025-08-12T13:19:45Z

+Currently this is the case for `OperatorFeeVault` on some chains, so setting its minimum withdrawal amount to
+0 is needed on those chains.


Why is this needed? If the min amount is not met the vault is ignored, right?

agusduha · 2025-08-12T13:21:45Z

+
+- MUST emit `FeesDisbursed` event upon successful execution.
+- MUST emit `NoFeesCollected` event if there are no funds available in the contract after the vaults have been withdrawn.
+- MUST revert if any vault has a balance below its minimum withdrawal amount.


This is wrong, we should not revert if there is some other vault with enough balance

agusduha

We should try to adapt the specs to the PoC namings and invariants

Joxess

These specs are quite good. You might want to review what we have in the PoC to maintain consistency between function types, variable declarations, naming, etc to ensure alignment

Joxess · 2025-08-15T22:09:28Z

+
+| Name        | Address                                    | Introduced | Deprecated | Proxied |
+| ----------- | ------------------------------------------ | ---------- | ---------- | ------- |
+| FeeSplitter | 0x420000000000000000000000000000000000001C | Jovian     | No         | Yes     |


Check with the PoC on what the correct address is

Joxess · 2025-08-15T22:40:02Z

+- MUST revert if any vault has a recipient different from this contract.
+- MUST revert if any vault has a withdrawal network different from `WithdrawalNetwork.L2`.
+- MUST withdraw the vault's fees balance if the vault's balance is equal to or greater than the minimum withdrawal amount set.
+- MUST set the `lastDisbursementTime` to the current block timestamp.


Maybe it is helpful to mention that this is updated only when disburseFees collects ETH

Joxess · 2025-08-15T22:44:24Z

+## Security Considerations
+
+- Given that vault recipients can now be updated, it's important to ensure that this can only be done by the appropriate address, namely `ProxyAdmin.owner()`.
+- Upgrading the vaults and making them compatible with the `FeeSplitter` incurs a process where you have to deploy the new implementations and properly configure the vaults, which introduces complexity and potential for errors. It is important to develop a solution, such as a contract to manage the entire upgrade process, simplifying the UX and reducing the risk of errors.


Suggested change

- Upgrading the vaults and making them compatible with the `FeeSplitter` incurs a process where you have to deploy the new implementations and properly configure the vaults, which introduces complexity and potential for errors. It is important to develop a solution, such as a contract to manage the entire upgrade process, simplifying the UX and reducing the risk of errors.

- Upgrading the vaults and making them compatible with the `FeeSplitter` incurs a process that requires deploying the new implementations and properly configuring the vaults, which introduces complexity and potential for errors. It is important to develop a solution, such as a contract to manage the entire upgrade process, simplifying the UX and reducing the risk of errors.

tynes · 2025-08-19T17:15:44Z

Do you mind updating this with a mermaid diagram of the control flow so that its easier to visualize when explaining to people?

funkornaut001 · 2025-08-20T16:22:21Z

+On deployment, the constructor will:
+
+- Read and grab the current values for `recipient()`, `withdrawalNetwork()`, and `minWithdrawalAmount()` from each live vault (`SequencerFeeVault`, `L1FeeVault`, `BaseFeeVault`, `OperatorFeeVault`). Use a `try/catch` block to handle the case where the call fails because the vault is a legacy one, and query for the `RECIPIENT()` and `MIN_WITHDRAWAL_AMOUNT()` immutable values instead.
+- Deploy a new implementation for each vault, passing the grabbed values per vault as constructor immutables, and also passing `L2` as the `WITHDRAWAL_NETWORK` constructor immutable.


Shouldn't we keep whatever this value is on the current fee vault? So if it says L1 deploy the new implementation with L1 as withdrawal network.

funkornaut001 · 2025-08-20T16:23:08Z

+
+- Read and grab the current values for `recipient()`, `withdrawalNetwork()`, and `minWithdrawalAmount()` from each live vault (`SequencerFeeVault`, `L1FeeVault`, `BaseFeeVault`, `OperatorFeeVault`). Use a `try/catch` block to handle the case where the call fails because the vault is a legacy one, and query for the `RECIPIENT()` and `MIN_WITHDRAWAL_AMOUNT()` immutable values instead.
+- Deploy a new implementation for each vault, passing the grabbed values per vault as constructor immutables, and also passing `L2` as the `WITHDRAWAL_NETWORK` constructor immutable.
+- Emit a single event containing the four deployed implementation addresses


Should we emit the old values or old implementation address for any reason?

refactor: FeeVault's `withdraw`

0xChin · 2025-09-17T19:33:59Z

+
+- All fee vaults MUST be correctly configured (`WithdrawalNetwork = L2` and `RECIPIENT = FeeSplitter`) or `disburseFees` MUST revert.
+- `SharesCalculator` output MUST be valid: `sum(amounts) == totalCollected`, each `recipient != address(0)`, and each `withdrawalNetwork` is supported; otherwise `disburseFees` MUST revert.
+- Disbursement MUST be atomic: if any individual payout fails (L2 transfer or L1 withdrawal), `disburseFees` MUST revert the entire transaction.


The splitter only does L2 transfers, L1 withdrawals are handled by the L1Withdrawer

0xChin · 2025-09-17T19:34:40Z

+- All fee vaults MUST be correctly configured (`WithdrawalNetwork = L2` and `RECIPIENT = FeeSplitter`) or `disburseFees` MUST revert.
+- `SharesCalculator` output MUST be valid: `sum(amounts) == totalCollected`, each `recipient != address(0)`, and each `withdrawalNetwork` is supported; otherwise `disburseFees` MUST revert.
+- Disbursement MUST be atomic: if any individual payout fails (L2 transfer or L1 withdrawal), `disburseFees` MUST revert the entire transaction.
+- If no funds are collected for the call, `disburseFees` MUST return early and MUST NOT consume the disbursement interval.


Suggested change

- If no funds are collected for the call, `disburseFees` MUST return early and MUST NOT consume the disbursement interval.

- If no funds are collected for the call, `disburseFees` MUST revert and MUST NOT consume the disbursement interval.

Also I would remove the "must not consume disbursement interval" given that revering means that any state change is reverted but up to u

0xChin · 2025-09-17T19:36:11Z

+### Invariants
+
+- All fee vaults MUST be correctly configured (`WithdrawalNetwork = L2` and `RECIPIENT = FeeSplitter`) or `disburseFees` MUST revert.
+- `SharesCalculator` output MUST be valid: `sum(amounts) == totalCollected`, each `recipient != address(0)`, and each `withdrawalNetwork` is supported; otherwise `disburseFees` MUST revert.


Share info only consists of recipient and amount, withdrawal network does no longer exist in the ShareInfo struct

0xChin · 2025-09-17T19:39:14Z

+- `SharesCalculator` output MUST be valid: `sum(amounts) == totalCollected`, each `recipient != address(0)`, and each `withdrawalNetwork` is supported; otherwise `disburseFees` MUST revert.
+- Disbursement MUST be atomic: if any individual payout fails (L2 transfer or L1 withdrawal), `disburseFees` MUST revert the entire transaction.
+- If no funds are collected for the call, `disburseFees` MUST return early and MUST NOT consume the disbursement interval.
+- Cross‑function reentrancy MUST be prevented: `disburseFees` is non‑reentrant and `receive` MUST revert while a disbursement is in progress.


disburseFees is reentrant, and receive MUST NOT revert while a disbursement is in progress. What we actually do regarding receive + reentrancy:

receive reverts if:

It's not in a fees disbursement context, meaning that receive() is only opened when disbursing fees, so when calling FeeVault.withdraw() it doesn't revert

Caller is not one of the four fee vaults

Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com> Signed-off-by: IamFlux <175354924+0xiamflux@users.noreply.github.com>

feat: add predeploys.md

99a8f26

0xiamflux requested review from agusduha and funkornaut001 August 4, 2025 16:18

0xiamflux self-assigned this Aug 4, 2025

0xiamflux added 2 commits August 4, 2025 12:22

doc: specify WithdrawalNetwork for the L1 and L2 enum

75d2072

doc: break down long lines

6a3bcc8

agusduha reviewed Aug 4, 2025

View reviewed changes

0xiamflux changed the base branch from main to sc-feat/revenue-sharing August 4, 2025 21:28

0xiamflux added 7 commits August 4, 2025 20:05

doc: updates the functions section for fee vaults

5f20b81

doc: add events section for fee vaults

9192833

doc: Update overview section

c8c42d9

doc: expand on vaults invariants and overview

2fa0c12

doc: expand FeeSplitter section

1b96677

doc: add table for new predeploy

f310370

chore: lint

e3a44ca

Joxess reviewed Aug 7, 2025

View reviewed changes

0xiamflux added 5 commits August 7, 2025 10:50

doc: expand on NoFeesCollected event

66f095d

doc: expand on disburseFees behavior

b6000d3

doc: clarify unused vaults behavior, expand withdraw conditions

5718207

doc: expand disburseFees invariants

1e28c0c

doc: remove redundant fee vault documentation

bcb492d

agusduha changed the title ~~feat: add predeploys.md~~ feat: add revenue sharing specs Aug 12, 2025

agusduha reviewed Aug 12, 2025

View reviewed changes

agusduha requested changes Aug 12, 2025

View reviewed changes

agusduha requested a review from 0xDiscotech August 12, 2025 13:25

fix: agus pr comments

59d00af

0xDiscotech added 2 commits August 15, 2025 12:33

chore: remove virtual on receive

9e132ba

refactor: fees disbursed event

5a1e6b2

Joxess reviewed Aug 15, 2025

View reviewed changes

tynes mentioned this pull request Aug 19, 2025

design-doc: revenue sharing ethereum-optimism/design-docs#309

Merged

0xDiscotech added 4 commits August 20, 2025 11:26

feat: first initializer iteration and some fee splitter updates

fb0603c

fix: dont account operator vault for net revenue

402a15a

feat: update initializer logic

66c5807

feat: add pros and cons and polish both specs

526dc35

funkornaut001 reviewed Aug 20, 2025

View reviewed changes

0xDiscotech and others added 4 commits September 1, 2025 15:23

feat: add first fee splitter version

d883b00

refactor: add FeeVault.withdraw refactor

f02eff5

chore: add totalProcessed invariant

a8038b5

Merge pull request #56 from defi-wonderland/refactor/review

66ca654

refactor: FeeVault's `withdraw`

0xChin reviewed Sep 17, 2025

View reviewed changes

feat: add L1Withdrawer and SuperchainRevSharesCalculator specs (#58)

f74237d

0xDiscotech reviewed Sep 18, 2025

View reviewed changes

Comment thread specs/protocol/jovian/fees-depositor.md

0xDiscotech reviewed Sep 18, 2025

View reviewed changes

Comment thread specs/protocol/jovian/predeploys.md Outdated

0xDiscotech reviewed Sep 18, 2025

View reviewed changes

Comment thread specs/protocol/jovian/predeploys.md Outdated

0xDiscotech reviewed Sep 18, 2025

View reviewed changes

Comment thread specs/protocol/jovian/predeploys.md

0xiamflux and others added 4 commits September 18, 2025 18:36

Update specs/protocol/jovian/predeploys.md

79d5737

Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com> Signed-off-by: IamFlux <175354924+0xiamflux@users.noreply.github.com>

feat: add fees flow diagram to fees-depositor.md

5ffbb3a

fix: remove wrong line

f87e10a

refactor: add ShareInfo struct for context

eafd5b0

0xDiscotech merged commit 0ca1934 into sc-feat/revenue-sharing Sep 19, 2025
1 check passed

0xDiscotech deleted the feat/fee-splitter branch September 19, 2025 15:51


		#### `NoFeesCollected`

		Emitted when `disburseFees` is called but there are no funds available in the fee vaults at the time of execution.

		Currently this is the case for `OperatorFeeVault` on some chains, so setting its minimum withdrawal amount to
		0 is needed on those chains.

	- Upgrading the vaults and making them compatible with the `FeeSplitter` incurs a process where you have to deploy the new implementations and properly configure the vaults, which introduces complexity and potential for errors. It is important to develop a solution, such as a contract to manage the entire upgrade process, simplifying the UX and reducing the risk of errors.
	- Upgrading the vaults and making them compatible with the `FeeSplitter` incurs a process that requires deploying the new implementations and properly configuring the vaults, which introduces complexity and potential for errors. It is important to develop a solution, such as a contract to manage the entire upgrade process, simplifying the UX and reducing the risk of errors.

	- If no funds are collected for the call, `disburseFees` MUST return early and MUST NOT consume the disbursement interval.
	- If no funds are collected for the call, `disburseFees` MUST revert and MUST NOT consume the disbursement interval.

Conversation

0xiamflux commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear Bot commented Aug 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xiamflux Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agusduha left a comment

Choose a reason for hiding this comment

Uh oh!

Joxess left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tynes commented Aug 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

0xiamflux commented Aug 4, 2025 •

edited

Loading

0xiamflux Aug 7, 2025 •

edited

Loading