MultiSwapForge Contracts for GasEstimation #42

salmanferrum · 2024-01-30T13:55:29Z

Overview

This pull request introduces changes to the contract architecture to accommodate gas estimation functionality and separates the production contracts from gas estimation contracts. The main contracts involved are MultiSwapForgeContract, ForgeFundManager, FiberRouter, and FundManager.

Contracts Overview

Production Contracts:
- FiberRouter & FundManager: These contracts facilitate real-time transactions in the production environment.
Gas Estimation Contracts:
- MultiSwapForgeContract: Simulates withdrawals and reverts with gas estimation, featuring estimateWithdrawSigned and estimateWithdrawSignedAndSwapOneInch functions.
- ForgeFundManager: A gas estimation replica of FundManager, featuring only withdrawSigned and withdrawSignedAndSwapOneInch functions.

Production Contracts Workflow

Real-time transactions leverage the production contracts (FiberRouter and FundManager).

Gas Estimation Contracts Workflow

Gas estimation for withdrawals follows a specific flow to simulate transactions without interacting with the production contracts:
1. FiberBackend calls estimateGasForWithdrawSigned in MultiSwapForgeContract.
2. MultiSwapForgeContract's estimateGasForWithdrawSigned function invokes the withdrawSigned function of ForgeFundManager.
3. After simulating the transaction, the function call reverts the transaction to simulate the gas consumption.
4. estimateGas.estimateGasForWithdrawSigned() function returns the estimated gas for simulating the withdrawSigned function.

These changes enhance the gas estimation capabilities of the system and segregate the gas estimation logic from the production transactions.

Changes

Introduction of gas estimation contracts (MultiSwapForgeContract and ForgeFundManager).
Modification of workflows to ensure production contracts are utilized for real transactions, while gas estimation contracts are dedicated to estimating gas usage.
Changed the Upgradeable-Contracts folder name to multiswap-contracts.
Added the forge directory in scripts directory to accomodate the multiswapForge / forgeFundManager contracts deployments.

Additional Notes

A clear distinction is maintained between production and gas estimation contracts to prevent inadvertent interactions.
This architectural update prepares the system for efficient gas estimation handling during withdrawal simulations.

… develop

naiemk · 2024-02-04T14:08:44Z

contracts/multiswap-contracts/FiberRouter.sol

@@ -107,7 +106,7 @@ contract FiberRouter is Ownable, TokenReceivable {
     @notice Sets the fund manager contract.
     @param _pool The fund manager
     */
-    function setPool(address _pool) external onlyOwner {
+    function setPool(address _pool) public virtual onlyOwner {


why public virtual?

Changing the FiberRouter contract’s function access from external to public and introducing the virtual keyword is for inheritance. This is visible in MultiswapForge.sol, where functions like setPool are overridden. Without making the parent class functions public virtual, the compiler was giving errors while overriding in MultiswapForge, which is why I had to make this change in FiberRouter.

naiemk · 2024-02-04T14:08:59Z

contracts/multiswap-contracts/FiberRouter.sol

@@ -119,7 +118,7 @@ contract FiberRouter is Ownable, TokenReceivable {
     @notice Sets the gas wallet address.
     @param _gasWallet The wallet which pays for the funds on withdrawal
     */
-    function setGasWallet(address payable _gasWallet) external onlyOwner {
+    function setGasWallet(address payable _gasWallet) external virtual onlyOwner {


naiemk · 2024-02-04T14:09:28Z

contracts/multiswap-contracts/FiberRouter.sol

@@ -132,7 +131,8 @@ contract FiberRouter is Ownable, TokenReceivable {
     @param _newRouterAddress The new Router Address of oneInch
     */
    function setOneInchAggregatorRouter(address _newRouterAddress)
-        external
+        public


naiemk · 2024-02-04T14:25:18Z

contracts/multiswap-contracts/FiberRouter.sol

@@ -412,7 +413,7 @@ function swapAndCrossOneInch(
        bytes32 salt,
        uint256 expiry,
        bytes memory multiSignature
-    ) external nonReentrant {
+    ) public virtual nonReentrant {


Why public? also the same for all other methods

naiemk · 2024-02-04T14:26:34Z

contracts/multiswap-contracts/ForgeFundManager.sol

+import "foundry-contracts/contracts/common/FerrumDeployer.sol";
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+
+contract ForgeFundManager is SigCheckable, WithAdmin, TokenReceivable {


ForgeFundManager is FundManager This is the only thing needed. Similar to the router no need to rewrite everything.

Sure let me make this adjustment

naiemk · 2024-02-04T14:28:46Z

contracts/multiswap-contracts/MultiswapForge.sol

+        address targetAddress,
+        bytes32 withdrawalData
+    ) external override payable nonReentrant {
+        revert("Function is reverted");


Maybe Not supported error mesage

naiemk · 2024-02-04T14:30:03Z

contracts/multiswap-contracts/MultiswapForge.sol

+    }
+
+    // Inherit withdrawSigned function
+    function withdrawSigned(


Rename to withdrawSignedForGasEstimateion and overwrite the withdrawSign to revert.

Then create a new function called estimateGasForWithdrawSigned, where it will use address(this).call(...) to call the above function. This function will not fail, because it is using .call so you on the backend or frontend, you can use ethres.estimateGas(tx) to estimate the gas correctly.

Let me take a good look into this approach and test it again.

@naiemk I have pushed the latest commit with the changes mentioned in this PR comments,

Moreover, the change specifically for estimation functions are implemented in a way that the original withdrawSigned function overriden and reverted in Forge contract, however, I have renamed the withdrawSigned to withdrawSignedForGasEstimateion and called the super.withdrawSigned to simulate the withdraw flow with a revert at the end. Then, I have created estimateGasForWithdrawSigned that calls the withdrawSignedForGasEstimateion using address(this).call(). On backend (deployAndTest.js), I have called contract.estimateGas.estimateGasForWithdrawSigned and now it is returning correct gasFee.

naiemk · 2024-02-04T14:30:37Z

contracts/multiswap-contracts/MultiswapForge.sol

+        );
+
+        // Calculate the gas used during the execution
+        uint256 gasUsed = initialGas - gasleft();


Unused. no need

contracts/multiswap-contracts/MultiswapForge.sol

naiemk · 2024-02-04T14:31:36Z

contracts/multiswap-contracts/MultiswapForge.sol

+        // Calculate the gas used during the execution
+        uint256 gasUsed = initialGas - gasleft();
+
+        // Revert with a message indicating gas usage


Why messge indicating gas use? No easy way to get this message in the client so this is useless

salmanferrum · 2024-02-09T15:01:20Z

@naiemk changes have been made as per the suggestion.

Changes introduced to the latest commit are mentioned below:

Removed all unnecessary functions from ForgeFundManager & MultiSwapForge except withdrawSigned & withdrawSignedSwapAndCrossOneInch.
Changed the visibility modifier of all FiberRouter & FundManager functions back to external (removing virtual) except for FiberRouter'swithdrawSigned & withdrawSignedSwapAndCrossOneInch
addSigner() function needed to be changed to public from external in FundManager since we are calling it from forgeFundManager super.addSigner() so the visibility needs to be public.
Constructor of ForgeFundManager will set the Signer with a test signer address that will be used only for getting the gas fee from forge. The private key of test signer will be added in the commented form.

naiemk · 2024-02-09T18:55:33Z

contracts/multiswap-contracts/MultiswapForge.sol

+        bytes32 salt,
+        uint256 expiry,
+        bytes memory multiSignature
+    ) external {


This function does not revert. Can be used to extract funds.

naiemk · 2024-02-09T18:56:07Z

contracts/multiswap-contracts/MultiswapForge.sol

+        revert("Not Supported");
+    }
+    // This function is only used specifically for GasEstimation & Simulation of withdrawSigned
+    function withdrawSignedForGasEstimation(


Function must revert.

naiemk · 2024-02-09T18:57:39Z

contracts/multiswap-contracts/ForgeManager.sol

+     * @note PrivateKey of the Test Signer to be added here for Devs Reference
+     */
+    constructor() {
+        super.addSigner(0x);


What's the point of adding 0x as signer. Add a real signer.

Certainly! I have requested a test wallet, and once it is received, I will include the test signer's address and private key (to be added in comments) and then proceed with the commit.

naiemk · 2024-02-09T18:58:02Z

contracts/multiswap-contracts/FundManager.sol

@@ -80,7 +79,7 @@ contract FundManager is SigCheckable, WithAdmin, TokenReceivable {
        router = _router;
    }

-    function addSigner(address _signer) external onlyOwner {
+    function addSigner(address _signer) public onlyOwner {


Alright, I can create a dedicated setSigner function within the ForgeFundManager contract, performing the same task of adding the test signer to the signers array. This approach allows the function to be conveniently invoked through the constructor.

function setSigner(address _signer) public onlyOwner { require(_signer != address(0), "Bad signer"); signers[_signer] = true; }

P.S: We cannot directly inherit an external function from a parent contract into a child contract unless the visibility of that function is modified to at least public

nm my bad. this will be needed

@naiemk new changes pushed:

0cab7ba

Please review the final changes. Thanks

naiemk · 2024-02-09T19:12:43Z

contracts/multiswap-contracts/MultiswapForge.sol

+        bytes4 selector = bytes4(
+            keccak256(
+                "withdrawSignedAndSwapOneInchForGasEstimation(address,uint256,uint256,address,address,bytes,bytes32,uint256,bytes)"
+            )


Can use this.withdrawSignedAndSwapOneInchForGasEstimation.selector

Implemented.

salmanferrum added 8 commits January 23, 2024 19:07

gas estimation & forge contract

fe96bf5

forge contract implementation

27dcd64

merge conflict resolved

7ddcf5d

removed unnecessary comments

910b96c

forge contracts updates & minor changes

3292c32

Merge branch 'feature/MultiSwapForge' into develop

ab0bc33

Merge branch 'develop' of https://github.com/ferrumnet/MultiSwap into…

bd22b78

… develop

Inheritted FR in Forge

9a988a8

naiemk requested changes Feb 4, 2024

View reviewed changes

salmanferrum added 2 commits February 7, 2024 01:33

Forge Contracts Improvements

609c5cd

forge optomisation

450ed3d

naiemk requested changes Feb 9, 2024

View reviewed changes

Test Signer Added

0cab7ba

taha-abbasi requested a review from naiemk February 12, 2024 17:26

naiemk approved these changes Feb 12, 2024

View reviewed changes

zikriya merged commit 14e0a1a into ferrumnet:develop Feb 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MultiSwapForge Contracts for GasEstimation #42

MultiSwapForge Contracts for GasEstimation #42

salmanferrum commented Jan 30, 2024 •

edited

Loading

naiemk Feb 4, 2024

salmanferrum Feb 5, 2024

naiemk Feb 4, 2024

naiemk Feb 4, 2024

naiemk Feb 4, 2024

naiemk Feb 4, 2024

salmanferrum Feb 5, 2024

naiemk Feb 4, 2024

salmanferrum Feb 5, 2024

naiemk Feb 4, 2024

salmanferrum Feb 5, 2024

salmanferrum Feb 6, 2024

naiemk Feb 4, 2024

naiemk Feb 4, 2024

salmanferrum commented Feb 9, 2024

naiemk Feb 9, 2024

naiemk Feb 9, 2024

naiemk Feb 9, 2024

salmanferrum Feb 9, 2024

naiemk Feb 9, 2024

salmanferrum Feb 9, 2024

naiemk Feb 10, 2024

salmanferrum Feb 12, 2024

naiemk Feb 9, 2024

salmanferrum Feb 9, 2024

MultiSwapForge Contracts for GasEstimation #42

MultiSwapForge Contracts for GasEstimation #42

Conversation

salmanferrum commented Jan 30, 2024 • edited Loading

Overview

Contracts Overview

Production Contracts Workflow

Gas Estimation Contracts Workflow

Changes

Additional Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salmanferrum commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salmanferrum commented Jan 30, 2024 •

edited

Loading