feat(ethexe/contracts): add fee for upload code, add method for withdrawing exec balance#5280
Conversation
…rawing exec balance
Changed Files
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the router contract by introducing a fee mechanism for code validation to cover operational costs. It also adds administrative capabilities for balance management and updates the infrastructure to track network gas prices, ensuring that validation requests are properly incentivized and funded. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request implements a dynamic gas pricing mechanism for code validation within the ethexe router, requiring a fee based on the network's latest gas price, and adds an owner-only function to withdraw WrappedVara. Key feedback points out that the withdrawal logic risks depleting reward funds, and the gas price update mechanism is incomplete as it lacks the documented periodic updates during batch commitments. Furthermore, the Rust client contains a potential integer overflow in fee calculations and uses discouraged error handling patterns, while the contract's strict fee equality and hardcoded deployment gas limits are noted as fragile.
| function withdrawExecutableBalance() external onlyOwner { | ||
| IERC20 _wrappedVara = IERC20(wrappedVara()); | ||
|
|
||
| uint256 value = _wrappedVara.balanceOf(address(this)); | ||
| require(value > 0, ZeroValueTransfer()); | ||
|
|
||
| bool success = _wrappedVara.transfer(msg.sender, value); | ||
| require(success, WithdrawExecutableBalanceFailed()); | ||
| } |
There was a problem hiding this comment.
The withdrawExecutableBalance function withdraws the entire WrappedVara balance of the Router contract. However, the Router also holds WrappedVara intended for rewards distribution (see _commitRewards at line 512). Withdrawing the full balance will cause rewards commitments to fail due to insufficient funds. Consider tracking the 'executable balance' separately or allowing withdrawal of only a specific amount.
There was a problem hiding this comment.
we currently do not use _commitRewards at production
| // keccak256(abi.encode(uint256(keccak256("router.storage.Transient")) - 1)) & ~bytes32(uint256(0xff)) | ||
| bytes32 private constant TRANSIENT_STORAGE = 0xf02b465737fa6045c2ff53fb2df43c66916ac2166fa303264668fb2f6a1d8c00; | ||
|
|
||
| uint256 public constant CODE_COMMITMENT_GAS = 13_551; |
There was a problem hiding this comment.
from benchmarks PR
|
@claude review |
|
Claude finished @grishasobol's task in 1m 17s —— View job Code Review
This PR adds a mandatory fee for
|
…-upload-code-fee-and-withdraw-erc20
| // Private implementation details | ||
| impl EthereumDeployer { | ||
| /// Fixes "out of gas" error for unknown reason, maybe need to report to alloy? | ||
| const LOOKUP_GENESIS_HASH_GAS: u64 = 100_000; |
There was a problem hiding this comment.
Yes, make sense, I don't find similar issues alloy repo right now.
But, I guess, it happens, because we use the nightly build of foundry (maybe I am wrong)
…-upload-code-fee-and-withdraw-erc20
…-upload-code-fee-and-withdraw-erc20
breathx
left a comment
There was a problem hiding this comment.
I don’t see any need to store or update the gas price, since we won’t be converting it directly, either now or in the future.
Moreover, the withdrawal of exec balances is high risk operation since it could brake further committing logic so I'd keep it in upcoming commitments reward.
Fee for code uploading should be charged in WVARA with some constant value that could be updated on migrations.
No description provided.