Conversation
…cFee and add dynamic fee support to MowPlantHarvestBlueprint
… constant and unified overflow check in BlueprintBase
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
… across blueprints
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…g/protocol into feat/dynamic-fee-module
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…g/protocol into feat/dynamic-fee-module
fr1jo
left a comment
fr1jo
left a comment
There was a problem hiding this comment.
some comments (not finalized, may have some more comments).
One thing to note that i didn't realize until reviewing is that part of the calculation is circular:
(1): the amount of value to withdraw to pay for the tip is dependent on the gas used.
(2): the gas used is dependent on the number of deposits to withdraw
since (2) is dependent on (1), this is not easily done on chain (this would require an iterative loop).
We need to resolve this.
First, we should have a constant, which represents the gas used to call the TractorHelpers.tip function. You will need to investigate the amount of gas this function uses. This is simple to do with a script and logging the gas used
Second, we need to find the amount of gas that siloHelpers.withdrawBeansFromSources takes. Given that this changes a lot based on 1) the withdraw type and 2) the amount, if we want to do a constant value, we will have to do some analysis to see how much gas this function takes.
we should have 2 constants, GAS_USED_BEAN, and GAS_USED_LP, which is the gas used to withdraw from Pinto deposits and gas used to withdraw from LP.
Defer to you on how to best determine these two constants. One way to do this is to look at historical tractor events, and derive the gas used for LP withdraws and BEAN withdraws. We can then take the average gas used, and add a large margin to this (50%).
Happy to discuss async if the above isn't clear.
| uint256 marginBps | ||
| ) external view returns (uint256 fee, uint256 ethBeanRate) { | ||
| ethBeanRate = _getEthBeanRate(); | ||
| fee = this.calculateFeeInBean(gasUsed, marginBps); |
There was a problem hiding this comment.
calculateFeeInBean can be public here and can omit the this keyword.
There was a problem hiding this comment.
nit; can we move this file and BlueprintBase into blueprints?
| fee = gasCostCalculator.calculateFeeInBean(feeParams.gasUsed, feeParams.feeMarginBps); | ||
|
|
||
| // Validate fee doesn't overflow when cast to int256 | ||
| require(fee <= uint256(type(int256).max), "BlueprintBase: fee overflow"); |
There was a problem hiding this comment.
do we really need this require statement?
| ); | ||
| LibSiloHelpers.WithdrawalPlan memory emptyPlan; | ||
|
|
||
| siloHelpers.withdrawBeansFromSources( |
There was a problem hiding this comment.
can we use getWithdrawalPlan instead of withdrawBeansFromSources?
| ); | ||
|
|
||
| // Sow the withdrawn beans | ||
| if (referral == address(0)) { |
There was a problem hiding this comment.
given the ordering, you should move the sow block before _processFeesAndTip, because the sow transaction is missed (and thus missed in the 2nd gasleft()) call.
| * @param feeParams Struct containing all parameters for dynamic fee calculation | ||
| * @return fee The calculated fee amount in Bean | ||
| */ | ||
| function _payDynamicFee(DynamicFeeParams memory feeParams) internal returns (uint256 fee) { |
There was a problem hiding this comment.
_payDynamicFee implies an action is occuring, but in this function, we're simply performing the calculation, so i would rename this to something better, like calcDynamicFee.
|
|
||
| // Add dynamic fee if enabled | ||
| if (params.opParams.baseOpParams.useDynamicFee) { | ||
| uint256 gasUsedBeforeFee = startGas - gasleft(); |
There was a problem hiding this comment.
gasLeft() should be called as far down the transaction as possible, as we should minimize the amount of gas thats being calculated from a constant in GasCostCalculator. Ideally, we should call gasLeft() in calculateFeeInBean instead, after we perform the beanstalkPrice.price call, and the chainlink oracle call.
| * @notice Gas buffer for dynamic fee calculation to account for remaining operations | ||
| * @dev This buffer covers the gas cost of fee withdrawal and subsequent tip operations | ||
| */ | ||
| uint256 public constant DYNAMIC_FEE_GAS_BUFFER = 15000; |
There was a problem hiding this comment.
how did we get 15000 gas? this probably needs to be a lot higher.
3 participants
No description provided.