From b571e29161fc48f30f46f561c6c930c1611eb2fd Mon Sep 17 00:00:00 2001 From: vikramarun <33469661+vikramarun@users.noreply.github.com> Date: Mon, 15 Jan 2024 19:22:19 -0500 Subject: [PATCH 1/3] chore: updated payment helper logic --- src/interfaces/IPaymentHelper.sol | 2 +- src/payments/PaymentHelper.sol | 88 ++++++++++++++++++++----------- 2 files changed, 59 insertions(+), 31 deletions(-) diff --git a/src/interfaces/IPaymentHelper.sol b/src/interfaces/IPaymentHelper.sol index df07181e8..0a7dcd909 100644 --- a/src/interfaces/IPaymentHelper.sol +++ b/src/interfaces/IPaymentHelper.sol @@ -27,7 +27,7 @@ interface IPaymentHelper { /// @param defaultNativePrice is the native price on the specified chain /// @param defaultGasPrice is the gas price on the specified chain /// @param dstGasPerByte is the gas per size of data on the specified chain - /// @param ackGasCost is the gas cost for processing acknowledgements on src chain + /// @param ackGasCost is the gas cost for sending and processing messages /// @param timelockCost is the extra cost for processing timelocked payloads /// @param emergencyCost is the extra cost for processing emergency payloads struct PaymentHelperConfig { diff --git a/src/payments/PaymentHelper.sol b/src/payments/PaymentHelper.sol index 86dc43a77..caf1abb9e 100644 --- a/src/payments/PaymentHelper.sol +++ b/src/payments/PaymentHelper.sol @@ -66,14 +66,14 @@ contract PaymentHelper is IPaymentHelper { mapping(uint64 chainId => AggregatorV3Interface) public gasPriceOracle; mapping(uint64 chainId => uint256 gasForSwap) public swapGasUsed; mapping(uint64 chainId => uint256 gasForUpdate) public updateGasUsed; - mapping(uint64 chainId => uint256 gasForOps) public depositGasUsed; - mapping(uint64 chainId => uint256 gasForOps) public withdrawGasUsed; + mapping(uint64 chainId => uint256 gasForDeposit) public depositGasUsed; + mapping(uint64 chainId => uint256 gasForWithdraw) public withdrawGasUsed; mapping(uint64 chainId => uint256 defaultNativePrice) public nativePrice; mapping(uint64 chainId => uint256 defaultGasPrice) public gasPrice; mapping(uint64 chainId => uint256 gasPerByte) public gasPerByte; - mapping(uint64 chainId => uint256 gasForOps) public ackGasCost; - mapping(uint64 chainId => uint256 gasForOps) public timelockCost; - mapping(uint64 chainId => uint256 gasForOps) public emergencyCost; + mapping(uint64 chainId => uint256 gasForAck) public ackGasCost; + mapping(uint64 chainId => uint256 gasForTimelock) public timelockCost; + mapping(uint64 chainId => uint256 gasForEmergency) public emergencyCost; /// @dev register transmuter params bytes public extraDataForTransmuter; @@ -204,8 +204,7 @@ contract PaymentHelper is IPaymentHelper { for (uint256 j; j < arrLen; ++j) { if (!req_.superformsData[i].retain4626s[j]) ++ackLen; } - /// @dev step 4: estimation processing cost of acknowledgement - /// @notice optimistically estimating. (Ideal case scenario: no failed deposits / withdrawals) + /// @dev step 4: estimation processing cost of acknowledgement on source srcAmount += _estimateAckProcessingCost(v.superformIdsLen); /// @dev step 5: estimate dst swap cost if it exists @@ -225,9 +224,20 @@ contract PaymentHelper is IPaymentHelper { } } - /// @dev step 7: estimate execution costs in dst (withdraw / deposit) - /// note: execution cost includes acknowledgement messaging cost - v.totalDstGas += xChain ? _estimateDstExecutionCost(isDeposit_, req_.dstChainIds[i], v.superformIdsLen) : 0; + /// @dev step 7: estimate execution costs in destination including sending acknowledgement to source + /// @dev ensure that acknowledgement costs from dst to src are not double counted + bool anyRetain4626False = false; + for (uint256 j = 0; j < req_.superformsData[i].retain4626s.length; ++j) { + if (!req_.superformsData[i].retain4626s[j]) { + anyRetain4626False = true; + break; + } + } + if (anyRetain4626False && xChain) { + v.totalDstGas += _estimateDstExecutionCost(isDeposit_, false, req_.dstChainIds[i], v.superformIdsLen); + } else { + v.totalDstGas += xChain ? _estimateDstExecutionCost(isDeposit_, true, req_.dstChainIds[i], v.superformIdsLen) : 0; + } /// @dev step 8: convert all dst gas estimates to src chain estimate (withdraw / deposit) dstAmount += _convertToNativeFee(req_.dstChainIds[i], v.totalDstGas); @@ -263,11 +273,12 @@ contract PaymentHelper is IPaymentHelper { if (isDeposit_) { /// @dev step 2: estimate the liqAmount liqAmount += _estimateLiqAmount(req_.superformsData[i].liqRequest.castLiqRequestToArray()); + if (xChain) { /// @dev step 3: estimate update cost (only for deposit) totalDstGas += _estimateUpdateCost(req_.dstChainIds[i], 1); - /// @dev step 4: estimation execution cost of acknowledgement + /// @dev step 4: estimation execution cost of acknowledgement on source if (!req_.superformsData[i].retain4626) { srcAmount += _estimateAckProcessingCost(1); } @@ -289,9 +300,8 @@ contract PaymentHelper is IPaymentHelper { } } - /// @dev step 7: estimate execution costs in dst - /// note: execution cost includes acknowledgement messaging cost - totalDstGas += xChain ? _estimateDstExecutionCost(isDeposit_, req_.dstChainIds[i], 1) : 0; + /// @dev step 7: estimate execution costs in destination including sending acknowledgement to source + totalDstGas += xChain ? _estimateDstExecutionCost(isDeposit_, req_.superformsData[i].retain4626, req_.dstChainIds[i], 1) : 0; /// @dev step 8: convert all dst gas estimates to src chain estimate dstAmount += _convertToNativeFee(req_.dstChainIds[i], totalDstGas); @@ -321,7 +331,10 @@ contract PaymentHelper is IPaymentHelper { srcAmount += ambFees; if (isDeposit_) { - /// @dev step 2: estimate update cost (only for deposit) + /// @dev step 2: estimate the liqAmount + liqAmount += _estimateLiqAmount(req_.superformsData.liqRequests); + + /// @dev step 3: estimate update cost (only for deposit) totalDstGas += _estimateUpdateCost(req_.dstChainId, superformIdsLen); uint256 arrLen = req_.superformsData.retain4626s.length; @@ -331,12 +344,9 @@ contract PaymentHelper is IPaymentHelper { if (!req_.superformsData.retain4626s[i]) ++ackLen; } - /// @dev step 3: estimation execution cost of acknowledgement + /// @dev step 4: estimation execution cost of acknowledgement on source srcAmount += _estimateAckProcessingCost(ackLen); - /// @dev step 4: estimate the liqAmount - liqAmount += _estimateLiqAmount(req_.superformsData.liqRequests); - /// @dev step 5: estimate if swap costs are involved totalDstGas += _estimateSwapFees(req_.dstChainId, req_.superformsData.hasDstSwaps); } else { @@ -354,8 +364,20 @@ contract PaymentHelper is IPaymentHelper { } } - /// @dev step 7: estimate execution costs in destination - totalDstGas += _estimateDstExecutionCost(isDeposit_, req_.dstChainId, superformIdsLen); + /// @dev step 7: estimate execution costs in destination including sending acknowledgement to source + /// @dev ensure that acknowledgement costs from dst to src are not double counted + bool anyRetain4626False = false; + for (uint256 i; i < req_.superformsData.retain4626s.length; ++i) { + if (!req_.superformsData.retain4626s[i]) { + anyRetain4626False = true; + break; + } + } + if (anyRetain4626False) { + totalDstGas += _estimateDstExecutionCost(isDeposit_, false, req_.dstChainId, superformIdsLen); + } else { + totalDstGas += _estimateDstExecutionCost(isDeposit_, true, req_.dstChainId, superformIdsLen); + } /// @dev step 8: convert all destination gas estimates to source chain estimate dstAmount += _convertToNativeFee(req_.dstChainId, totalDstGas); @@ -382,17 +404,17 @@ contract PaymentHelper is IPaymentHelper { srcAmount += ambFees; if (isDeposit_) { - /// @dev step 2: estimate update cost (only for deposit) + /// @dev step 2: estimate the liqAmount + liqAmount += _estimateLiqAmount(req_.superformData.liqRequest.castLiqRequestToArray()); + + /// @dev step 3: estimate update cost (only for deposit) totalDstGas += _estimateUpdateCost(req_.dstChainId, 1); - /// @dev step 3: estimation execution cost of acknowledgement + /// @dev step 4: estimation execution cost of acknowledgement on source if (!req_.superformData.retain4626) { srcAmount += _estimateAckProcessingCost(1); } - /// @dev step 4: estimate the liqAmount - liqAmount += _estimateLiqAmount(req_.superformData.liqRequest.castLiqRequestToArray()); - /// @dev step 5: estimate if swap costs are involved totalDstGas += _estimateSwapFees(req_.dstChainId, req_.superformData.hasDstSwap.castBoolToArray()); } else { @@ -408,8 +430,8 @@ contract PaymentHelper is IPaymentHelper { } } - /// @dev step 7: estimate execution costs in destination - totalDstGas += _estimateDstExecutionCost(isDeposit_, req_.dstChainId, 1); + /// @dev step 7: estimate execution costs in destination including sending acknowledgement to source + totalDstGas += _estimateDstExecutionCost(isDeposit_, req_.superformData.retain4626, req_.dstChainId, 1); /// @dev step 8: convert all destination gas estimates to source chain estimate dstAmount += _convertToNativeFee(req_.dstChainId, totalDstGas); @@ -839,9 +861,11 @@ contract PaymentHelper is IPaymentHelper { return vaultsCount_ * updateGasUsed[dstChainId_]; } - /// @dev helps estimate the dst chain processing gas limit + /// @dev helps estimate the dst chain processing cost including the dst->src message cost + /// @dev assumes that withdrawals optimisically succeed function _estimateDstExecutionCost( bool isDeposit_, + bool retain4626_, uint64 dstChainId_, uint256 vaultsCount_ ) @@ -850,8 +874,12 @@ contract PaymentHelper is IPaymentHelper { returns (uint256 gasUsed) { uint256 executionGasPerVault = isDeposit_ ? depositGasUsed[dstChainId_] : withdrawGasUsed[dstChainId_]; + gasUsed = executionGasPerVault * vaultsCount_; - return executionGasPerVault * vaultsCount_; + /// @dev add ackGasCost only if it's a deposit and retain4626 is false + if (isDeposit_ && !retain4626_) { + gasUsed += ackGasCost[dstChainId_]; + } } /// @dev helps estimate the src chain processing fee From ee7616f9a52f0fbd4e8a4a5c8ddf315e336d989f Mon Sep 17 00:00:00 2001 From: vikramarun <33469661+vikramarun@users.noreply.github.com> Date: Tue, 16 Jan 2024 00:11:36 -0500 Subject: [PATCH 2/3] chore: cleanup --- src/interfaces/IPaymentHelper.sol | 2 +- src/payments/PaymentHelper.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/interfaces/IPaymentHelper.sol b/src/interfaces/IPaymentHelper.sol index 0a7dcd909..4423e9c53 100644 --- a/src/interfaces/IPaymentHelper.sol +++ b/src/interfaces/IPaymentHelper.sol @@ -27,7 +27,7 @@ interface IPaymentHelper { /// @param defaultNativePrice is the native price on the specified chain /// @param defaultGasPrice is the gas price on the specified chain /// @param dstGasPerByte is the gas per size of data on the specified chain - /// @param ackGasCost is the gas cost for sending and processing messages + /// @param ackGasCost is the gas cost for sending and processing from dst->src /// @param timelockCost is the extra cost for processing timelocked payloads /// @param emergencyCost is the extra cost for processing emergency payloads struct PaymentHelperConfig { diff --git a/src/payments/PaymentHelper.sol b/src/payments/PaymentHelper.sol index caf1abb9e..957b24e1c 100644 --- a/src/payments/PaymentHelper.sol +++ b/src/payments/PaymentHelper.sol @@ -227,7 +227,7 @@ contract PaymentHelper is IPaymentHelper { /// @dev step 7: estimate execution costs in destination including sending acknowledgement to source /// @dev ensure that acknowledgement costs from dst to src are not double counted bool anyRetain4626False = false; - for (uint256 j = 0; j < req_.superformsData[i].retain4626s.length; ++j) { + for (uint256 j; j < req_.superformsData[i].retain4626s.length; ++j) { if (!req_.superformsData[i].retain4626s[j]) { anyRetain4626False = true; break; From 4e88e55a3637f7fd2a2a35475ed54e1dfb97ea29 Mon Sep 17 00:00:00 2001 From: sujithsomraaj Date: Wed, 17 Jan 2024 15:34:09 +0530 Subject: [PATCH 3/3] fix: pr comments for SUP-5192 --- src/payments/PaymentHelper.sol | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/payments/PaymentHelper.sol b/src/payments/PaymentHelper.sol index 957b24e1c..d97ff3d26 100644 --- a/src/payments/PaymentHelper.sol +++ b/src/payments/PaymentHelper.sol @@ -199,9 +199,8 @@ contract PaymentHelper is IPaymentHelper { /// @dev step 3: estimate update cost (only for deposit) v.totalDstGas += _estimateUpdateCost(req_.dstChainIds[i], v.superformIdsLen); - uint256 arrLen = req_.superformsData[i].retain4626s.length; uint256 ackLen; - for (uint256 j; j < arrLen; ++j) { + for (uint256 j; j < v.superformIdsLen; ++j) { if (!req_.superformsData[i].retain4626s[j]) ++ackLen; } /// @dev step 4: estimation processing cost of acknowledgement on source @@ -226,17 +225,18 @@ contract PaymentHelper is IPaymentHelper { /// @dev step 7: estimate execution costs in destination including sending acknowledgement to source /// @dev ensure that acknowledgement costs from dst to src are not double counted - bool anyRetain4626False = false; - for (uint256 j; j < req_.superformsData[i].retain4626s.length; ++j) { + bool hasRetain4626; + for (uint256 j; j < v.superformIdsLen; ++j) { if (!req_.superformsData[i].retain4626s[j]) { - anyRetain4626False = true; + hasRetain4626 = true; break; } } - if (anyRetain4626False && xChain) { + if (hasRetain4626 && xChain) { v.totalDstGas += _estimateDstExecutionCost(isDeposit_, false, req_.dstChainIds[i], v.superformIdsLen); } else { - v.totalDstGas += xChain ? _estimateDstExecutionCost(isDeposit_, true, req_.dstChainIds[i], v.superformIdsLen) : 0; + v.totalDstGas += + xChain ? _estimateDstExecutionCost(isDeposit_, true, req_.dstChainIds[i], v.superformIdsLen) : 0; } /// @dev step 8: convert all dst gas estimates to src chain estimate (withdraw / deposit) @@ -301,7 +301,9 @@ contract PaymentHelper is IPaymentHelper { } /// @dev step 7: estimate execution costs in destination including sending acknowledgement to source - totalDstGas += xChain ? _estimateDstExecutionCost(isDeposit_, req_.superformsData[i].retain4626, req_.dstChainIds[i], 1) : 0; + totalDstGas += xChain + ? _estimateDstExecutionCost(isDeposit_, req_.superformsData[i].retain4626, req_.dstChainIds[i], 1) + : 0; /// @dev step 8: convert all dst gas estimates to src chain estimate dstAmount += _convertToNativeFee(req_.dstChainIds[i], totalDstGas); @@ -337,10 +339,8 @@ contract PaymentHelper is IPaymentHelper { /// @dev step 3: estimate update cost (only for deposit) totalDstGas += _estimateUpdateCost(req_.dstChainId, superformIdsLen); - uint256 arrLen = req_.superformsData.retain4626s.length; uint256 ackLen; - - for (uint256 i; i < arrLen; ++i) { + for (uint256 i; i < superformIdsLen; ++i) { if (!req_.superformsData.retain4626s[i]) ++ackLen; } @@ -366,14 +366,14 @@ contract PaymentHelper is IPaymentHelper { /// @dev step 7: estimate execution costs in destination including sending acknowledgement to source /// @dev ensure that acknowledgement costs from dst to src are not double counted - bool anyRetain4626False = false; - for (uint256 i; i < req_.superformsData.retain4626s.length; ++i) { + bool hasRetain4626; + for (uint256 i; i < superformIdsLen; ++i) { if (!req_.superformsData.retain4626s[i]) { - anyRetain4626False = true; + hasRetain4626 = true; break; } } - if (anyRetain4626False) { + if (hasRetain4626) { totalDstGas += _estimateDstExecutionCost(isDeposit_, false, req_.dstChainId, superformIdsLen); } else { totalDstGas += _estimateDstExecutionCost(isDeposit_, true, req_.dstChainId, superformIdsLen); @@ -862,7 +862,7 @@ contract PaymentHelper is IPaymentHelper { } /// @dev helps estimate the dst chain processing cost including the dst->src message cost - /// @dev assumes that withdrawals optimisically succeed + /// @dev assumes that withdrawals optimisically succeed function _estimateDstExecutionCost( bool isDeposit_, bool retain4626_,