Merge branch '115-evmchain-improvements-and-bug-fixes' into 'dev'

Resolve "EvmChain Improvements and bug fixes"

Closes #115

See merge request ergo/rosen-bridge/rosen-chains!137

.changeset/five-horses-knock.md

@@ -0,0 +1,5 @@
+---
+'@rosen-chains/evm': major
+---
+
+add gasLimitCap to EvmConfigs (the cap is used in tx generation and fee verification when required gas is too high)

.changeset/modern-readers-explain.md

@@ -0,0 +1,5 @@
+---
+'@rosen-chains/evm-rpc': patch
+---
+
+fix gas estimation

packages/chains/evm/lib/EvmChain.ts

@@ -159,9 +159,14 @@ abstract class EvmChain extends AbstractChain<Transaction> {
           chainId: this.CHAIN_ID,
         });
       }
-      trx.gasLimit =
-        (await this.network.getGasRequired(trx)) *
-        this.configs.gasLimitMultiplier;
+      let estimatedRequiredGas = await this.network.getGasRequired(trx);
+      if (estimatedRequiredGas > this.configs.gasLimitCap) {
+        this.logger.warn(
+          `Estimated required gas is more than gas limit cap config and cap is used for the tx [${estimatedRequiredGas} > ${this.configs.gasLimitCap}]`
+        );
+        estimatedRequiredGas = this.configs.gasLimitCap;
+      }
+      trx.gasLimit = estimatedRequiredGas * this.configs.gasLimitMultiplier;
       totalGas += trx.gasLimit;
 
       evmTrxs.push(
@@ -375,8 +380,14 @@ abstract class EvmChain extends AbstractChain<Transaction> {
     }
 
     // check gas limit
-    const gasRequired =
-      (await this.network.getGasRequired(tx)) * this.configs.gasLimitMultiplier;
+    let estimatedRequiredGas = await this.network.getGasRequired(tx);
+    if (estimatedRequiredGas > this.configs.gasLimitCap) {
+      this.logger.debug(
+        `Estimated required gas is more than gas limit cap config and cap is used for verification [${estimatedRequiredGas} > ${this.configs.gasLimitCap}]`
+      );
+      estimatedRequiredGas = this.configs.gasLimitCap;
+    }
+    const gasRequired = estimatedRequiredGas * this.configs.gasLimitMultiplier;
     const gasLimitSlippage =
       (gasRequired * BigInt(this.configs.gasLimitSlippage)) / 100n;
     const gasDifference =

packages/chains/evm/lib/types.ts

@@ -5,6 +5,7 @@ export interface EvmConfigs extends ChainConfigs {
   gasPriceSlippage: bigint;
   gasLimitSlippage: bigint;
   gasLimitMultiplier: bigint;
+  gasLimitCap: bigint;
 }
 
 export type TssSignFunction = (txHash: Uint8Array) => Promise<{

packages/chains/evm/tests/EvmChain.spec.ts

@@ -64,6 +64,7 @@ describe('EvmChain', () => {
      * - no extra data should be found in the transactions' data
      * - transactions must be of type 2 and has no blobs
      * - nonces must be in sequential order starting from next available nonce
+     * - gas limit should be as expected
      */
     it('should generate payment transactions successfully for multiple orders', async () => {
       const orders = TestData.multipleOrders;
@@ -87,9 +88,10 @@ describe('EvmChain', () => {
 
       // mock hasLockAddressEnoughAssets, getMaxFeePerGas,
       // getGasRequired, getAddressNextNonce, getMaxPriorityFeePerGas
+      const requiredGas = 100000n;
       testUtils.mockHasLockAddressEnoughAssets(evmChain, true);
       testUtils.mockGetMaxFeePerGas(network, 10n);
-      testUtils.mockGetGasRequired(network, 100000n);
+      testUtils.mockGetGasRequired(network, requiredGas);
       testUtils.mockGetAddressNextAvailableNonce(network, nonce);
       testUtils.mockGetMaxPriorityFeePerGas(network, 10n);
 
@@ -133,6 +135,9 @@ describe('EvmChain', () => {
 
         // check nonce
         expect(tx.nonce).toEqual(nonce + index);
+
+        // check gas limit
+        expect(tx.gasLimit).toEqual(requiredGas * 3n); // requiredGas * gasLimitMultiplier
       }
     });
 
@@ -155,6 +160,7 @@ describe('EvmChain', () => {
      * - no extra data should be found in the transaction data
      * - transaction must be of type 2 and has no blobs
      * - nonce must be the same as the next available nonce
+     * - gas limit should be as expected
      */
     it('should generate payment transaction successfully for single order', async () => {
       const order = TestData.nativePaymentOrder;
@@ -164,9 +170,10 @@ describe('EvmChain', () => {
 
       // mock hasLockAddressEnoughAssets, getMaxFeePerGas,
       // getGasRequired, getAddressNextNonce, getMaxPriorityFeePerGas
+      const requiredGas = 21000n;
       testUtils.mockHasLockAddressEnoughAssets(evmChain, true);
       testUtils.mockGetMaxFeePerGas(network, 10n);
-      testUtils.mockGetGasRequired(network, 21000n);
+      testUtils.mockGetGasRequired(network, requiredGas);
       testUtils.mockGetAddressNextAvailableNonce(network, nonce);
       testUtils.mockGetMaxPriorityFeePerGas(network, 10n);
 
@@ -204,6 +211,83 @@ describe('EvmChain', () => {
 
       // check nonce
       expect(tx.nonce).toEqual(nonce);
+
+      // check gas limit
+      expect(tx.gasLimit).toEqual(requiredGas * 3n); // requiredGas * gasLimitMultiplier
+    });
+
+    /**
+     * @target EvmChain.generateMultipleTransactions should generate payment
+     * transaction with capped gas limit when required is too high
+     * @dependencies
+     * @scenario
+     * - mock hasLockAddressEnoughAssets, getMaxFeePerGas
+     * - mock getGasRequired, getAddressNextNonce
+     * - mock getMaxPriorityFeePerGas
+     * - call the function
+     * - check returned value
+     * @expected
+     * - PaymentTransaction txType, eventId and network should be as
+     *   expected
+     * - extracted order of generated transaction should be the same as input
+     *   order
+     * - eventId should be properly in the transaction data
+     * - no extra data should be found in the transaction data
+     * - transaction must be of type 2 and has no blobs
+     * - nonce must be the same as the next available nonce
+     * - gas limit should be the capped gas limit
+     */
+    it('should generate payment transaction with capped gas limit when required is too high', async () => {
+      const order = TestData.nativePaymentOrder;
+      const eventId = 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb';
+      const txType = TransactionType.payment;
+      const nonce = 54;
+
+      // mock hasLockAddressEnoughAssets, getMaxFeePerGas,
+      // getGasRequired, getAddressNextNonce, getMaxPriorityFeePerGas
+      testUtils.mockHasLockAddressEnoughAssets(evmChain, true);
+      testUtils.mockGetMaxFeePerGas(network, 10n);
+      testUtils.mockGetGasRequired(network, 200000n);
+      testUtils.mockGetAddressNextAvailableNonce(network, nonce);
+      testUtils.mockGetMaxPriorityFeePerGas(network, 10n);
+
+      // run test
+      const evmTx = await evmChain.generateMultipleTransactions(
+        eventId,
+        txType,
+        order,
+        [],
+        []
+      );
+
+      // check returned value
+      expect(evmTx[0].txType).toEqual(txType);
+      expect(evmTx[0].eventId).toEqual(eventId);
+      expect(evmTx[0].network).toEqual(evmChain.CHAIN);
+
+      // extracted order of generated transaction should be the same as input order
+      const extractedOrder = evmChain.extractTransactionOrder(evmTx[0]);
+      expect(extractedOrder).toEqual(order);
+
+      const tx = Serializer.deserialize(evmTx[0].txBytes);
+
+      // check eventId encoded at the end of the data
+      expect(tx.data.substring(2, 34)).toEqual(eventId);
+
+      // check there is no more data
+      expect(tx.data.length).toEqual(34);
+
+      // check transaction type
+      expect(tx.type).toEqual(2);
+
+      // check blobs zero
+      expect(tx.maxFeePerBlobGas).toEqual(null);
+
+      // check nonce
+      expect(tx.nonce).toEqual(nonce);
+
+      // check gas limit
+      expect(tx.gasLimit).toEqual(100000n * 3n); // gasLimitCap * gasLimitMultiplier
     });
 
     /**
@@ -817,6 +901,49 @@ describe('EvmChain', () => {
       expect(result).toEqual(false);
     });
 
+    /**
+     * @target EvmChain.verifyTransactionFee should return false when gasLimit is over
+     * the cap for erc-20 transfer
+     * @dependencies
+     * @scenario
+     * - mock mockGetGasRequired
+     * - mock mockGetMaxFeePerGas, mockGetMaxPriorityFeePerGas
+     * - mock PaymentTransaction
+     * - check returned value
+     * @expected
+     * - it should return false
+     */
+    it('should return false when gasLimit is over the cap for erc-20 transfer', async () => {
+      // mock a config that has more fee and wrong required gas
+      // comparing to the mocked transaction
+      testUtils.mockGetGasRequired(network, 90000n);
+      testUtils.mockGetMaxFeePerGas(network, 20n);
+      testUtils.mockGetMaxPriorityFeePerGas(network, 7n);
+
+      // mock PaymentTransaction
+      const eventId = 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb';
+      const txType = TransactionType.payment;
+      const tx = Transaction.from(TestData.transaction1Json);
+      tx.gasLimit = 120000n * evmChain.configs.gasLimitMultiplier;
+      tx.maxFeePerGas = 22n;
+      tx.maxPriorityFeePerGas = 8n;
+      tx.value = 0n;
+
+      const paymentTx = new PaymentTransaction(
+        evmChain.CHAIN,
+        tx.unsignedHash,
+        eventId,
+        Serializer.serialize(tx),
+        txType
+      );
+
+      // run test
+      const result = await evmChain.verifyTransactionFee(paymentTx);
+
+      // check returned value
+      expect(result).toEqual(false);
+    });
+
     /**
      * @target EvmChain.verifyTransactionFee should return false when gasLimit is wrong
      * for native-token transfer

packages/chains/evm/tests/TestUtils.ts

@@ -37,6 +37,7 @@ export const configs: EvmConfigs = {
   gasPriceSlippage: 15n,
   gasLimitSlippage: 25n,
   gasLimitMultiplier: 3n,
+  gasLimitCap: 100000n,
 };
 
 export const mockedSignFn = () =>

packages/networks/evm-rpc/lib/EvmRpcNetwork.ts

@@ -29,6 +29,7 @@ class EvmRpcNetwork extends AbstractEvmNetwork {
   readonly chain: string;
   protected readonly provider: JsonRpcProvider;
   protected readonly dbAction: AddressTxAction;
+  protected readonly lockAddress: string;
 
   constructor(
     chain: string,
@@ -44,6 +45,7 @@ class EvmRpcNetwork extends AbstractEvmNetwork {
       ? new JsonRpcProvider(`${url}/${authToken}`)
       : new JsonRpcProvider(`${url}`);
     this.dbAction = new AddressTxAction(lockAddress, dataSource, logger);
+    this.lockAddress = lockAddress;
   }
 
   /**
@@ -297,7 +299,10 @@ class EvmRpcNetwork extends AbstractEvmNetwork {
    */
   getGasRequired = async (transaction: Transaction): Promise<bigint> => {
     try {
-      const gas = await this.provider.estimateGas(transaction);
+      const gas = await this.provider.estimateGas({
+        ...transaction.toJSON(),
+        from: this.lockAddress,
+      });
       this.logger.debug(
         `requested 'estimateGas' of ${
           this.chain