WIP: Enshrined Revenue Sharing implementation #10320
Conversation
![semgrep-app[bot]](https://avatars.githubusercontent.com/in/60555?s=60&v=4){kind=small}
|
Semgrep found 15
Prefer Semgrep found 13
Javadoc-style comments are not allowed. Use Semgrep found 11
No Semgrep found 2
When working with web applications that involve rendering user-generated content, it's important to properly escape any HTML content to prevent Cross-Site Scripting (XSS) attacks. In Go, the Semgrep found 1 Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. Ignore this finding from dangerous-exec-command. |
|
You should rebase on latest |
| "RevenueSharer: FeeVault must withdraw to RevenueSharer contract" | ||
| ); | ||
| uint256 initial_balance = address(this).balance; | ||
| FeeVault(_feeVault).withdraw(); // TODO do we need a reentrancy guard around this? |
There was a problem hiding this comment.
can you explain this todo more?
There was a problem hiding this comment.
The control flow is kinda jank for this system, def not your fault, its trying to build something on top that isn't really suited well for this usecase. It would be good to have a comment saying "withdraw calls back to this contract, sending its full balance of ether" or something like that so its more obvious the control flow
| * the minimum withdrawal amount. | ||
| */ | ||
| function feeVaultWithdrawal(address payable _feeVault) internal returns (uint256) { | ||
| require( |
There was a problem hiding this comment.
It could be worth also calling to check that the withdrawal network is L2, otherwise the state machine can be fooled
For sure. I also added WIP to the title, this is far from merging. |
with some TODO comments
and related changes to deployconfig etc failing on "proxy not initialized"
geoknee
force-pushed
the
gk/revenue-sharing2
branch
from
94fe988
to
f2530b5
Compare
| _beneficiary: cfg.revenueShareRecipient(), | ||
| _l1Wallet: cfg.revenueShareRemainderRecipient() | ||
| }); | ||
| } |
There was a problem hiding this comment.
you should be sure to call initialize on the implementation too, with null values
| uint256 remainder = r - s; | ||
|
|
||
| // Send Beneficiary their revenue share on L2 | ||
| if (!SafeCall.send(BENEFICIARY, gasleft(), s)) { |
There was a problem hiding this comment.
i would like us to add a version of SafeCall that only takes 2 arguments so we don't need to pass gasleft. If you don't want to do it as part of this PR thats fine
|
This is great so far! |
| /** | ||
| * @dev Withdraws funds from FeeVaults, sends Optimism their revenue share, and withdraws remaining funds to L1. | ||
| */ | ||
| function execute() external virtual { |
There was a problem hiding this comment.
If you added a mermaid diagram to the specs that showed the flow of calls, that would be amazing
| @@ -110,6 +112,8 @@ contract DeployConfig is Script { | |||
| sequencerFeeVaultRecipient = stdJson.readAddress(_json, "$.sequencerFeeVaultRecipient"); | |||
| sequencerFeeVaultMinimumWithdrawalAmount = stdJson.readUint(_json, "$.sequencerFeeVaultMinimumWithdrawalAmount"); | |||
| sequencerFeeVaultWithdrawalNetwork = stdJson.readUint(_json, "$.sequencerFeeVaultWithdrawalNetwork"); | |||
There was a problem hiding this comment.
If you could add a useRevenueShare = _envOr(_json, ".useRevenueShare", true); that would be great then in the Genesis generation you can know to configure the fee vaults appropriately when its set to true. Will need to add a setter setUseRevenueShare to be able to not break legacy tests, you can see how this is done with plasma, you need to call it in the tests setUp before calling super.setUp
| @@ -110,6 +112,8 @@ contract DeployConfig is Script { | |||
| sequencerFeeVaultRecipient = stdJson.readAddress(_json, "$.sequencerFeeVaultRecipient"); | |||
| sequencerFeeVaultMinimumWithdrawalAmount = stdJson.readUint(_json, "$.sequencerFeeVaultMinimumWithdrawalAmount"); | |||
| sequencerFeeVaultWithdrawalNetwork = stdJson.readUint(_json, "$.sequencerFeeVaultWithdrawalNetwork"); | |||
| revenueShareRecipient = payable(stdJson.readAddress(_json, "$.revenueShareRecipient")); | |||
| revenueShareRemainderRecipient = payable(stdJson.readAddress(_json, "$.revenueShareRemainderRecipient")); | |||
There was a problem hiding this comment.
can you use _envOr with null fallback values? This prevents breaking config and also makes it so that the null values are set when the revenue share is turned off
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Towards https://github.com/ethereum-optimism/client-pod/issues/739