From c8e30df7e122f468c33aefa5bd2d9093a7f9dce1 Mon Sep 17 00:00:00 2001 From: mpetrun5 Date: Wed, 16 Oct 2024 11:11:33 +0200 Subject: [PATCH 1/4] Add revert gas to origin chain fee calculation --- .github/workflows/test.yml | 4 +-- Makefile | 2 +- .../fee/dynamic/TwapERC20NativeFeeHandler.sol | 6 +++-- .../handlers/fee/dynamic/TwapFeeHandler.sol | 27 ++++++++++++++----- .../fee/dynamic/TwapNativeTokenFeeHandler.sol | 8 ++++-- 5 files changed, 34 insertions(+), 13 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a93b7458..8e4b07ea 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,8 +1,8 @@ name: Test on: - pull_request: - types: [opened, synchronize, reopened] + push: + - mpetrun5/add-revert-gas jobs: test: diff --git a/Makefile b/Makefile index 0c6358fb..6fc807c0 100644 --- a/Makefile +++ b/Makefile @@ -23,7 +23,7 @@ start-forkedMainnet: test-forked: @echo " > \033[32mTesting contracts... \033[0m " - truffle test --stacktrace testUnderForked/* + npx truffle test --stacktrace testUnderForked/* start-geth: @echo " > \033[32mStarting geth... \033[0m " diff --git a/contracts/handlers/fee/dynamic/TwapERC20NativeFeeHandler.sol b/contracts/handlers/fee/dynamic/TwapERC20NativeFeeHandler.sol index 9f0aeaa2..f5915fe2 100644 --- a/contracts/handlers/fee/dynamic/TwapERC20NativeFeeHandler.sol +++ b/contracts/handlers/fee/dynamic/TwapERC20NativeFeeHandler.sol @@ -15,11 +15,13 @@ contract TwapERC20NativeFeeHandler is TwapNativeTokenFeeHandler { @param bridgeAddress Contract address of previously deployed Bridge. @param feeHandlerRouterAddress Contract address of previously deployed FeeHandlerRouter. @param gasUsed Default gas used for proposal execution in the destination. + @param recoverGas Gas used for an optional call fallback. */ constructor( address bridgeAddress, address feeHandlerRouterAddress, - uint32 gasUsed - ) TwapNativeTokenFeeHandler(bridgeAddress, feeHandlerRouterAddress, gasUsed) { + uint32 gasUsed, + uint32 recoverGas + ) TwapNativeTokenFeeHandler(bridgeAddress, feeHandlerRouterAddress, gasUsed, recoverGas) { } } diff --git a/contracts/handlers/fee/dynamic/TwapFeeHandler.sol b/contracts/handlers/fee/dynamic/TwapFeeHandler.sol index b15d31da..7ed418f2 100644 --- a/contracts/handlers/fee/dynamic/TwapFeeHandler.sol +++ b/contracts/handlers/fee/dynamic/TwapFeeHandler.sol @@ -26,6 +26,7 @@ abstract contract TwapFeeHandler is IFeeHandler, AccessControl { ProtocolFeeType public protocolFeeType; uint32 public _gasUsed; + uint32 public _recoverGas; mapping(uint8 => address) public destinationNativeCoinWrap; mapping(uint8 => Fee) public destinationFee; @@ -43,7 +44,8 @@ abstract contract TwapFeeHandler is IFeeHandler, AccessControl { } event FeeOracleAddressSet(TwapOracle feeOracleAddress); - event FeePropertySet(uint32 gasUsed); + event GasUsedSet(uint32 gasUsed); + event RecoverGasSet(uint32 recoverGas); event GasPriceSet(uint8 destinationDomainID, uint256 gasPrice); event WrapTokenAddressSet(uint8 destinationDomainID, address wrapTokenAddress); @@ -136,16 +138,29 @@ abstract contract TwapFeeHandler is IFeeHandler, AccessControl { } /** - @notice Sets the fee properties. + @notice Sets the recover gas property. + @param recoverGas Gas used for an optional call fallback. + */ + function setRecoverGas(uint32 recoverGas) external onlyAdmin { + _setRecoverGas(recoverGas); + } + + function _setRecoverGas(uint32 recoverGas) internal { + _recoverGas = recoverGas; + emit RecoverGasSet(recoverGas); + } + + /** + @notice Sets the gas used property. @param gasUsed Default gas used for proposal execution in the destination. */ - function setFeeProperties(uint32 gasUsed) external onlyAdmin { - _setFeeProperties(gasUsed); + function setGasUsed(uint32 gasUsed) external onlyAdmin { + _setGasUsed(gasUsed); } - function _setFeeProperties(uint32 gasUsed) internal { + function _setGasUsed(uint32 gasUsed) internal { _gasUsed = gasUsed; - emit FeePropertySet(gasUsed); + emit GasUsedSet(gasUsed); } /** diff --git a/contracts/handlers/fee/dynamic/TwapNativeTokenFeeHandler.sol b/contracts/handlers/fee/dynamic/TwapNativeTokenFeeHandler.sol index 0a5fa34c..5893a3bb 100644 --- a/contracts/handlers/fee/dynamic/TwapNativeTokenFeeHandler.sol +++ b/contracts/handlers/fee/dynamic/TwapNativeTokenFeeHandler.sol @@ -15,13 +15,16 @@ contract TwapNativeTokenFeeHandler is TwapFeeHandler { @param bridgeAddress Contract address of previously deployed Bridge. @param feeHandlerRouterAddress Contract address of previously deployed FeeHandlerRouter. @param gasUsed Default gas used for proposal execution in the destination. + @param recoverGas Gas used for an optional call fallback. */ constructor( address bridgeAddress, address feeHandlerRouterAddress, - uint32 gasUsed + uint32 gasUsed, + uint32 recoverGas ) TwapFeeHandler(bridgeAddress, feeHandlerRouterAddress) { - _setFeeProperties(gasUsed); + _setGasUsed(gasUsed); + _setRecoverGas(recoverGas); } /** @@ -38,6 +41,7 @@ contract TwapNativeTokenFeeHandler is TwapFeeHandler { if (depositData.length > (pointer + 64)) { uint256 gas = abi.decode(depositData[pointer:], (uint256)); maxFee += gas; + maxFee += _recoverGas; } uint256 destinationCoinPrice = twapOracle.getPrice(destinationNativeCoinWrap[destinationDomainID]); if (destinationCoinPrice == 0) revert IncorrectPrice(); From d06b3f4003d9730b97ba0aeff9d44856ef42e4bf Mon Sep 17 00:00:00 2001 From: mpetrun5 Date: Wed, 16 Oct 2024 14:49:00 +0200 Subject: [PATCH 2/4] Fix action --- .github/workflows/test.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8e4b07ea..a1614dea 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -2,7 +2,8 @@ name: Test on: push: - - mpetrun5/add-revert-gas + branches: + - mpetrun5/add-revert-gas jobs: test: From f4819c8324f571f819253fcfffc9a950883998f9 Mon Sep 17 00:00:00 2001 From: mpetrun5 Date: Wed, 16 Oct 2024 17:09:16 +0200 Subject: [PATCH 3/4] Update contract deplyoments in tests --- testUnderForked/admin.js | 30 +++++++++++++++---- testUnderForked/calculateFeeERC20EVM.js | 3 +- testUnderForked/collectFeeERC20EVM.js | 3 +- .../calculateFeeERC20EVM.js | 3 +- 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/testUnderForked/admin.js b/testUnderForked/admin.js index a3e05149..17f9ef92 100644 --- a/testUnderForked/admin.js +++ b/testUnderForked/admin.js @@ -87,6 +87,7 @@ contract("TwapFeeHandler - [admin]", async (accounts) => { DynamicFeeHandlerInstance = await DynamicFeeHandlerContract.new( BridgeInstance.address, FeeHandlerRouterInstance.address, + 0, 0 ); ADMIN_ROLE = await DynamicFeeHandlerInstance.DEFAULT_ADMIN_ROLE(); @@ -119,21 +120,40 @@ contract("TwapFeeHandler - [admin]", async (accounts) => { ); }); - it("should set fee properties and emit 'FeePropertySet' event", async () => { + it("should set gas used property and emit 'GasUsedSet' event", async () => { assert.equal(await DynamicFeeHandlerInstance._gasUsed.call(), "0"); - const setFeeOraclePropertiesTx = await DynamicFeeHandlerInstance.setFeeProperties(gasUsed); + const setFeeOraclePropertiesTx = await DynamicFeeHandlerInstance.setGasUsed(gasUsed); assert.equal(await DynamicFeeHandlerInstance._gasUsed.call(), gasUsed); - TruffleAssert.eventEmitted(setFeeOraclePropertiesTx, "FeePropertySet", (event) => { + TruffleAssert.eventEmitted(setFeeOraclePropertiesTx, "GasUsedSet", (event) => { return ( event.gasUsed.toNumber() === gasUsed ); }); }); - it("should require admin role to change fee properties", async () => { + it("should require admin role to set gas used", async () => { await assertOnlyAdmin( - DynamicFeeHandlerInstance.setFeeProperties, + DynamicFeeHandlerInstance.setGasUsed, + gasUsed + ); + }); + + it("should set revert gas property and emit 'RecoverGasSet' event", async () => { + assert.equal(await DynamicFeeHandlerInstance._recoverGas.call(), "0"); + const setRecoverGasTx = await DynamicFeeHandlerInstance.setRecoverGas(gasUsed); + assert.equal(await DynamicFeeHandlerInstance._recoverGas.call(), gasUsed); + + TruffleAssert.eventEmitted(setRecoverGasTx, "RecoverGasSet", (event) => { + return ( + event.recoverGas.toNumber() === gasUsed + ); + }); + }); + + it("should require admin role to set recover gas", async () => { + await assertOnlyAdmin( + DynamicFeeHandlerInstance.setRecoverGas, gasUsed ); }); diff --git a/testUnderForked/calculateFeeERC20EVM.js b/testUnderForked/calculateFeeERC20EVM.js index 42628b69..2d725f72 100644 --- a/testUnderForked/calculateFeeERC20EVM.js +++ b/testUnderForked/calculateFeeERC20EVM.js @@ -102,6 +102,7 @@ contract("TwapFeeHandler - [calculateFee]", async (accounts) => { DynamicFeeHandlerInstance = await DynamicFeeHandlerContract.new( BridgeInstance.address, FeeHandlerRouterInstance.address, + 0, 0 ); FeeHandlerRouterInstance.adminSetResourceHandler( @@ -117,7 +118,7 @@ contract("TwapFeeHandler - [calculateFee]", async (accounts) => { fixedProtocolFee ); await DynamicFeeHandlerInstance.setWrapTokenAddress(destinationDomainID, MATIC_ADDRESS); - await DynamicFeeHandlerInstance.setFeeProperties(gasUsed); + await DynamicFeeHandlerInstance.setGasUsed(gasUsed); depositData = Helpers.createERCDepositData( 100, diff --git a/testUnderForked/collectFeeERC20EVM.js b/testUnderForked/collectFeeERC20EVM.js index 35e971b5..c61dbcd9 100644 --- a/testUnderForked/collectFeeERC20EVM.js +++ b/testUnderForked/collectFeeERC20EVM.js @@ -81,6 +81,7 @@ contract("TwapNativeTokenFeeHandler - [collectFee]", async (accounts) => { DynamicFeeHandlerInstance = await DynamicFeeHandlerContract.new( BridgeInstance.address, FeeHandlerRouterInstance.address, + 0, 0 ); @@ -127,7 +128,7 @@ contract("TwapNativeTokenFeeHandler - [collectFee]", async (accounts) => { fixedProtocolFee ); await DynamicFeeHandlerInstance.setWrapTokenAddress(destinationDomainID, MATIC_ADDRESS); - await DynamicFeeHandlerInstance.setFeeProperties(gasUsed); + await DynamicFeeHandlerInstance.setGasUsed(gasUsed); await BridgeInstance.adminSetResource( ERC20HandlerInstance.address, diff --git a/testUnderForked/optionalContractCall/calculateFeeERC20EVM.js b/testUnderForked/optionalContractCall/calculateFeeERC20EVM.js index 55e1648f..74dc0398 100644 --- a/testUnderForked/optionalContractCall/calculateFeeERC20EVM.js +++ b/testUnderForked/optionalContractCall/calculateFeeERC20EVM.js @@ -107,6 +107,7 @@ contract("TwapFeeHandler - [calculateFee]", async (accounts) => { DynamicFeeHandlerInstance = await DynamicFeeHandlerContract.new( BridgeInstance.address, FeeHandlerRouterInstance.address, + 0, 0 ); FeeHandlerRouterInstance.adminSetResourceHandler( @@ -122,7 +123,7 @@ contract("TwapFeeHandler - [calculateFee]", async (accounts) => { fixedProtocolFee ); await DynamicFeeHandlerInstance.setWrapTokenAddress(destinationDomainID, MATIC_ADDRESS); - await DynamicFeeHandlerInstance.setFeeProperties(gasUsed); + await DynamicFeeHandlerInstance.setGasUsed(gasUsed); }); it("should return higher fee for higher execution amount", async () => { From bbc8bbf37a5b269608875b91bc1ec8b97a1943fb Mon Sep 17 00:00:00 2001 From: nmlinaric Date: Fri, 18 Oct 2024 15:35:01 +0200 Subject: [PATCH 4/4] make old tests pass + test for recover gas tx cost --- .github/workflows/test.yml | 5 +- .../fee/dynamic/TwapNativeTokenFeeHandler.sol | 2 +- testUnderForked/collectFeeERC20EVM.js | 4 +- testUnderForked/collectFeeGenericEVM.js | 4 +- .../calculateFeeERC20EVM.js | 53 +++++++++++++++++++ 5 files changed, 60 insertions(+), 8 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a1614dea..a93b7458 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,9 +1,8 @@ name: Test on: - push: - branches: - - mpetrun5/add-revert-gas + pull_request: + types: [opened, synchronize, reopened] jobs: test: diff --git a/contracts/handlers/fee/dynamic/TwapNativeTokenFeeHandler.sol b/contracts/handlers/fee/dynamic/TwapNativeTokenFeeHandler.sol index 5893a3bb..73d0ebf7 100644 --- a/contracts/handlers/fee/dynamic/TwapNativeTokenFeeHandler.sol +++ b/contracts/handlers/fee/dynamic/TwapNativeTokenFeeHandler.sol @@ -41,7 +41,7 @@ contract TwapNativeTokenFeeHandler is TwapFeeHandler { if (depositData.length > (pointer + 64)) { uint256 gas = abi.decode(depositData[pointer:], (uint256)); maxFee += gas; - maxFee += _recoverGas; + maxFee += uint256(_recoverGas); } uint256 destinationCoinPrice = twapOracle.getPrice(destinationNativeCoinWrap[destinationDomainID]); if (destinationCoinPrice == 0) revert IncorrectPrice(); diff --git a/testUnderForked/collectFeeERC20EVM.js b/testUnderForked/collectFeeERC20EVM.js index c61dbcd9..208bf582 100644 --- a/testUnderForked/collectFeeERC20EVM.js +++ b/testUnderForked/collectFeeERC20EVM.js @@ -222,7 +222,7 @@ contract("TwapNativeTokenFeeHandler - [collectFee]", async (accounts) => { const fee = Ethers.BigNumber.from(expectedFee.toString()).div(2); const errorValues = await Helpers.expectToRevertWithCustomError( - BridgeInstance.deposit( + BridgeInstance.deposit.call( destinationDomainID, resourceID, depositData, @@ -243,7 +243,7 @@ contract("TwapNativeTokenFeeHandler - [collectFee]", async (accounts) => { await TwapOracleInstance.setPrice(MATIC_ADDRESS, 0); await Helpers.expectToRevertWithCustomError( - BridgeInstance.deposit( + BridgeInstance.deposit.call( destinationDomainID, resourceID, depositData, diff --git a/testUnderForked/collectFeeGenericEVM.js b/testUnderForked/collectFeeGenericEVM.js index 65046eb7..e049a9bf 100644 --- a/testUnderForked/collectFeeGenericEVM.js +++ b/testUnderForked/collectFeeGenericEVM.js @@ -217,7 +217,7 @@ contract("TwapGenericFeeHandler - [collectFee]", async (accounts) => { const fee = Ethers.BigNumber.from(expectedFee.toString()).div(2); const errorValues = await Helpers.expectToRevertWithCustomError( - BridgeInstance.deposit( + BridgeInstance.deposit.call( destinationDomainID, resourceID, depositData, @@ -238,7 +238,7 @@ contract("TwapGenericFeeHandler - [collectFee]", async (accounts) => { await TwapOracleInstance.setPrice(MATIC_ADDRESS, 0); await Helpers.expectToRevertWithCustomError( - BridgeInstance.deposit( + BridgeInstance.deposit.call( destinationDomainID, resourceID, depositData, diff --git a/testUnderForked/optionalContractCall/calculateFeeERC20EVM.js b/testUnderForked/optionalContractCall/calculateFeeERC20EVM.js index 74dc0398..11551013 100644 --- a/testUnderForked/optionalContractCall/calculateFeeERC20EVM.js +++ b/testUnderForked/optionalContractCall/calculateFeeERC20EVM.js @@ -49,6 +49,7 @@ contract("TwapFeeHandler - [calculateFee]", async (accounts) => { const higherExecutionGasAmount = 30000000; const lowerExecutionGasAmount = 3000000; const feeData = "0x"; + const feePercentage = 1000; // 10% let UniswapFactoryInstance; let TwapOracleInstance; @@ -176,4 +177,56 @@ contract("TwapFeeHandler - [calculateFee]", async (accounts) => { expect(higherExecutionGasAmountRes.fee.toNumber()).to.be.gt(lowerExecutionGasAmountRes.fee.toNumber()) }); + + it("[percentage protocol fee] should calculate in recover gas for tx cost", async () => { + const mintableERC20Iface = new Ethers.utils.Interface(["function mint(address to, uint256 amount)"]); + const actions = [{ + nativeValue: 0, + callTo: ERC20MintableInstance.address, + approveTo: Ethers.constants.AddressZero, + tokenSend: Ethers.constants.AddressZero, + tokenReceive: Ethers.constants.AddressZero, + data: mintableERC20Iface.encodeFunctionData("mint", [recipientAddress, "20"]), + }] + const message = Helpers.createMessageCallData( + transactionId, + actions, + recipientAddress + ); + + const depositData = Helpers.createOptionalContractCallDepositData( + transferredAmount, + Ethers.constants.AddressZero, + higherExecutionGasAmount, + message + ); + + await DynamicFeeHandlerInstance.setGasPrice( + destinationDomainID, + gasPrice, // Polygon gas price is 200 Gwei + ProtocolFeeType.Percentage, + feePercentage + ); + + const resWithoutRecoverGas = await FeeHandlerRouterInstance.calculateFee.call( + sender, + originDomainID, + destinationDomainID, + resourceID, + depositData, + "0x00" + ); + + await DynamicFeeHandlerInstance.setRecoverGas(gasUsed); + + const resWithRecoverGas = await FeeHandlerRouterInstance.calculateFee.call( + sender, + originDomainID, + destinationDomainID, + resourceID, + depositData, + "0x00" + ); + expect(resWithRecoverGas.fee.toNumber()).to.be.gt(resWithoutRecoverGas.fee.toNumber()); + }); });