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

WIP: RevenueSharer predeploy #115

Draft
wants to merge 29 commits into
base: main
Choose a base branch
from
Draft

WIP: RevenueSharer predeploy #115

wants to merge 29 commits into from

Conversation

geoknee
Copy link
Contributor

@geoknee geoknee commented Apr 8, 2024

No description provided.

specs/protocol/predeploys.md Outdated Show resolved Hide resolved
specs/protocol/predeploys.md Outdated Show resolved Hide resolved
0x4200000000000000000000000000000000000022
```

### Deploying `RevenueSharer`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on RevenueShare vs RevenueSharer? Seems a bit easier to say and gets the same point across

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have super strong opinion. I think having the -er follows existing predeploys such as L2ToL1MessagePasser , L2CrossDomainMessenger and so on. And we do already have some hard-to-say names such as OptimismMintableERC721Factory.

Other options are RevShare and RevSharer. Base are using something they call a FeeDisburser.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a preference to just call it RevenueShare to make it easier to say. the "revenue share predeploy" vs the "revenue sharer predeploy"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slack poll so far indicating a preference for RevenueSharer. I'm going to to continue using this name for now, it should be easy to replace down the line if necessary.

@geoknee geoknee requested a review from blmalone April 9, 2024 15:11
specs/protocol/predeploys.md Show resolved Hide resolved
specs/protocol/predeploys.md Show resolved Hide resolved
specs/protocol/predeploys.md Show resolved Hide resolved
specs/protocol/revenue-sharing.md Outdated Show resolved Hide resolved
specs/protocol/revenue-sharing.md Outdated Show resolved Hide resolved
specs/protocol/revenue-sharing.md Outdated Show resolved Hide resolved
specs/protocol/revenue-sharing.md Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor

tynes commented Apr 9, 2024

Thoughts on being more abstract with language as to not enshrine the optimism collective deeply into the protocol itself? its free software that anybody can use, perhaps we can just call the address that funds collect to the "collective address"

@geoknee
Copy link
Contributor Author

geoknee commented Apr 10, 2024

Thoughts on being more abstract with language as to not enshrine the optimism collective deeply into the protocol itself? its free software that anybody can use, perhaps we can just call the address that funds collect to the "collective address"

I agree I went with "Beneficiary" here 572b90d and listed Optimism Collective as an example.

@@ -81,7 +81,7 @@ The three types of fees are collected in 3 distinct L2 fee-vault deployments for
fee payments are not registered as internal EVM calls, and thus distinguished better this way.

These are hardcoded addresses, pointing at pre-deployed proxy contracts.
The proxies are backed by vault contract deployments, based on `FeeVault`, to route vault funds to L1 securely.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we should be too specific here on the behavior of the FeeVault. This is free software, it can be configured however

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a suggested wording? Can we just replace "are" with "may"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps something like "based on FeeVault, to collect fees"

simple and not opinionated on where the fees go

## `RevenueSharer` predeploy
Revenue sharing is achieved through an L2 [predeploy](./predeploys.md) contract `RevenueSharer` with address `0x4200000000000000000000000000000000000024`.

### Invariants
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vaults must be configured to withdraw to this predeploy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is relevant to your other comment.

The model I initially had in mind was to hardcode the RevenueShare(r) as the output of the FeeVaults. To my mind, this represents a greater level of enshrinement of revenue sharing, which I believe is our overarching goal. But it does have some downsides:

  1. It could be offputting or confusing to any chain operator who isn't sure about doing revenue sharing in the first place. As you say, from an open source perspective it's nice to include this set of users.
  2. It reduces overall configurability over how fees can flow around. Although we can ensure that the RevenueShare(r) can still be configured to send all funds to e.g. the chain operator or any other wallet, this approach does tie all three FeeVaults together (not possible to withdraw from one without withdrawing from the others).

We can make a different choice and make the the output of the FeeVaults freely configurable. This represents a somewhat lesser enshrinement of revenue sharing. The downside could be that:

  1. We risk not achieving our overall goal. Chains can easily bypass the RevenueShare(r) predeploy, leaving it dangling there doing nothing,
  2. There is more opportunity to deploy a "bad" configuration which could lead to incorrect behaviour. Of course this may be addressable through documentation and/or runtime checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From slack thread: would be nice to have a simple way to opt out of revenue sharing via a flag in the deploy configuration. When opting out, the existing/legacy behaviour can prevail.

| $s$ | Revenue share due to Optimism Collective | $\max(0.15r,0.025p)$

## `RevenueSharer` predeploy
Revenue sharing is achieved through an L2 [predeploy](./predeploys.md) contract `RevenueSharer` with address `0x4200000000000000000000000000000000000024`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps giving a simple interface + description of execute could be useful

Revenue sharing is achieved through an L2 [predeploy](./predeploys.md) contract `RevenueSharer` with address `0x4200000000000000000000000000000000000024`.

### Invariants
* The `RevenueSharer` contract should not accumulate any ETH or Tokens itself.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a "Security Considerations" section at the bottom with responses to questions around the security, ie "what if a user sends ether or a token to the contract directly?" Will this mess with its accounting?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geoknee let me know if you want input on this section 👍🏻

| $r$ | Sequencer Revenue | $d + b + q$
| $p$ | Sequencer Profit | $r - e$
| $s$ | Revenue share due to Optimism Collective | $\max(0.15r,0.025p)$

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add definitions for BENEFICIARY and REMAINDER_BENEFICIARY (or whatever terms we settle on).

The gas paying token address is network specific configuration, therefore it MUST be set in storage and not as an immutable. This ensures that the same contract bytecode can be used by multiple OP Stack chains.

I spotted this line on #111, so I guess we should follow that pattern here (REMAINDER_BENEFICIARY is chain specific).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants