fix: gas price estimation and native transfer (#316)

WIP: gas estimations for the CLI refactor  --- ## PR-Codex overview This PR focuses on improving gas cost optimization and enhancing transaction logic in Celo SDK and CLI. ### Detailed summary - Improved logic to fetch `maxFeePerGas` - Updated `transfer:celo` to use native `sendTransaction` - Added mocks for gas price and maxPriorityFeePerGas - Enhanced gas cost optimization in various functions > The following files were skipped due to too many changes: `docs/sdk/connect/classes/connection.Connection.md` > ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}`  --------- Co-authored-by: Aaron DeRuvo <[email protected]>
celo-org · Aug 8, 2024 · d245703 · d245703
1 parent 7715590
commit d245703
Show file tree

Hide file tree

Showing 11 changed files with 144 additions and 31 deletions.
diff --git a/.changeset/plenty-books-shop.md b/.changeset/plenty-books-shop.md
@@ -0,0 +1,5 @@
+---
+'@celo/connect': patch
+---
+
+Fix the logic to fetch maxFeePerGas
diff --git a/.changeset/swift-pens-switch.md b/.changeset/swift-pens-switch.md
@@ -0,0 +1,5 @@
+---
+'@celo/celocli': patch
+---
+
+Change transfer:celo to use native sendTransaction to save up on gas costs as well as better decoding on ledger
diff --git a/docs/sdk/connect/classes/connection.Connection.md b/docs/sdk/connect/classes/connection.Connection.md
@@ -52,6 +52,7 @@ handleRevert sets handleRevert on the web3.eth instance passed in
 - [getBlockHeader](connection.Connection.md#getblockheader)
 - [getBlockNumber](connection.Connection.md#getblocknumber)
 - [getLocalAccounts](connection.Connection.md#getlocalaccounts)
+- [getMaxPriorityFeePerGas](connection.Connection.md#getmaxpriorityfeepergas)
 - [getNodeAccounts](connection.Connection.md#getnodeaccounts)
 - [getTransaction](connection.Connection.md#gettransaction)
 - [getTransactionCount](connection.Connection.md#gettransactioncount)
@@ -392,7 +393,7 @@ ___
 
 #### Defined in
 
-[packages/sdk/connect/src/connection.ts:478](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L478)
+[packages/sdk/connect/src/connection.ts:485](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L485)
 
 ___
 
@@ -413,7 +414,7 @@ ___
 
 #### Defined in
 
-[packages/sdk/connect/src/connection.ts:453](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L453)
+[packages/sdk/connect/src/connection.ts:460](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L460)
 
 ___
 
@@ -433,7 +434,7 @@ ___
 
 #### Defined in
 
-[packages/sdk/connect/src/connection.ts:466](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L466)
+[packages/sdk/connect/src/connection.ts:473](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L473)
 
 ___
 
@@ -447,7 +448,7 @@ ___
 
 #### Defined in
 
-[packages/sdk/connect/src/connection.ts:444](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L444)
+[packages/sdk/connect/src/connection.ts:451](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L451)
 
 ___
 
@@ -465,6 +466,26 @@ ___
 
 ___
 
+### getMaxPriorityFeePerGas
+
+▸ **getMaxPriorityFeePerGas**(`feeCurrency?`): `Promise`\<`string`\>
+
+#### Parameters
+
+| Name | Type |
+| :------ | :------ |
+| `feeCurrency?` | `string` |
+
+#### Returns
+
+`Promise`\<`string`\>
+
+#### Defined in
+
+[packages/sdk/connect/src/connection.ts:444](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L444)
+
+___
+
 ### getNodeAccounts
 
 ▸ **getNodeAccounts**(): `Promise`\<\`0x$\{string}\`[]\>
@@ -495,7 +516,7 @@ ___
 
 #### Defined in
 
-[packages/sdk/connect/src/connection.ts:487](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L487)
+[packages/sdk/connect/src/connection.ts:494](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L494)
 
 ___
 
@@ -535,7 +556,7 @@ ___
 
 #### Defined in
 
-[packages/sdk/connect/src/connection.ts:495](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L495)
+[packages/sdk/connect/src/connection.ts:502](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L502)
 
 ___
 
@@ -828,4 +849,4 @@ ___
 
 #### Defined in
 
-[packages/sdk/connect/src/connection.ts:520](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L520)
+[packages/sdk/connect/src/connection.ts:527](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L527)
diff --git a/docs/sdk/connect/modules/connection.md b/docs/sdk/connect/modules/connection.md
@@ -34,4 +34,4 @@ value is string \| number \| bigint \| BN
 
 #### Defined in
 
-[packages/sdk/connect/src/connection.ts:538](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L538)
+[packages/sdk/connect/src/connection.ts:545](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L545)
diff --git a/packages/cli/src/commands/releasecelo/transfer-dollars.test.ts b/packages/cli/src/commands/releasecelo/transfer-dollars.test.ts
@@ -25,10 +25,20 @@ testWithGanache('releasegold:transfer-dollars cmd', (web3: Web3) => {
     )
     kit = newKitFromWeb3(web3)
     accounts = await web3.eth.getAccounts()
+    jest.spyOn(kit.connection, 'gasPrice').mockImplementation(async () => {
+      return '3000'
+    })
+    jest.spyOn(kit.connection, 'getMaxPriorityFeePerGas').mockImplementation(async () => {
+      return '4000'
+    })
     await testLocally(Register, ['--from', accounts[0]])
     await testLocally(CreateAccount, ['--contract', contractAddress])
   })
 
+  afterEach(() => {
+    jest.clearAllMocks()
+  })
+
   test('can transfer dollars out of the ReleaseGold contract', async () => {
     const balanceBefore = await kit.getTotalBalance(accounts[0])
     const cUSDToTransfer = '500000000000000000000'

diff --git a/packages/cli/src/commands/transfer/celo.test.ts b/packages/cli/src/commands/transfer/celo.test.ts
@@ -1,15 +1,38 @@
 import { COMPLIANT_ERROR_RESPONSE, SANCTIONED_ADDRESSES } from '@celo/compliance'
+import { HttpRpcCaller } from '@celo/connect'
 import { ContractKit, StableToken, newKitFromWeb3 } from '@celo/contractkit'
+import { testWithGanache } from '@celo/dev-utils/lib/ganache-test'
 import Web3 from 'web3'
 import { testLocallyWithWeb3Node } from '../../test-utils/cliUtils'
 import TransferCelo from './celo'
-import { testWithGanache } from '@celo/dev-utils/lib/ganache-test'
 
 process.env.NO_SYNCCHECK = 'true'
 
 // Lots of commands, sometimes times out
 jest.setTimeout(15000)
 
+const mockRpc = () =>
+  jest.spyOn(HttpRpcCaller.prototype, 'call').mockImplementation(async (method, _args) => {
+    if (method === 'eth_maxPriorityFeePerGas') {
+      return {
+        result: '20000',
+        id: 1,
+        jsonrpc: '2.0',
+      }
+    }
+    if (method === 'eth_gasPrice') {
+      return {
+        result: '30000',
+        id: 1,
+        jsonrpc: '2.0',
+      }
+    }
+    return {
+      result: 0,
+      id: Math.random(),
+      jsonrpc: '2.0',
+    }
+  })
 testWithGanache('transfer:celo cmd', (web3: Web3) => {
   let accounts: string[] = []
   let kit: ContractKit
@@ -24,6 +47,7 @@ testWithGanache('transfer:celo cmd', (web3: Web3) => {
     const receiverBalanceBefore = await kit.getTotalBalance(accounts[1])
     const amountToTransfer = '500000000000000000000'
     // Send cUSD to RG contract
+    let mock = mockRpc()
     await testLocallyWithWeb3Node(
       TransferCelo,
       [
@@ -38,11 +62,14 @@ testWithGanache('transfer:celo cmd', (web3: Web3) => {
       ],
       web3
     )
+    mock.mockRestore()
     // RG cUSD balance should match the amount sent
     const receiverBalance = await kit.getTotalBalance(accounts[1])
     expect(receiverBalance.CELO!.toFixed()).toEqual(
       receiverBalanceBefore.CELO!.plus(amountToTransfer).toFixed()
     )
+
+    mock = mockRpc()
     // Attempt to send cUSD back
     await testLocallyWithWeb3Node(
       TransferCelo,
@@ -58,6 +85,7 @@ testWithGanache('transfer:celo cmd', (web3: Web3) => {
       ],
       web3
     )
+    mock.mockRestore()
     const balanceAfter = await kit.getTotalBalance(accounts[0])
     expect(balanceBefore.CELO).toEqual(balanceAfter.CELO)
   })
@@ -104,6 +132,7 @@ testWithGanache('transfer:celo cmd', (web3: Web3) => {
     const balanceBefore = await kit.getTotalBalance(accounts[0])
     const receiverBalanceBefore = await kit.getTotalBalance(accounts[1])
     const amountToTransfer = '1'
+    let mock = mockRpc()
     await expect(
       testLocallyWithWeb3Node(
         TransferCelo,
@@ -120,7 +149,7 @@ testWithGanache('transfer:celo cmd', (web3: Web3) => {
         web3
       )
     ).resolves.toBeUndefined()
-
+    mock.mockRestore()
     const balanceAfter = await kit.getTotalBalance(accounts[0])
     const receiverBalanceAfter = await kit.getTotalBalance(accounts[1])
     expect(receiverBalanceAfter.CELO!.toFixed()).toEqual(

diff --git a/packages/cli/src/commands/transfer/celo.ts b/packages/cli/src/commands/transfer/celo.ts
@@ -38,11 +38,26 @@ export default class TransferCelo extends BaseCommand {
       .hasEnoughCelo(from, value)
       .runChecks()
 
+    const params = await kit.connection.setFeeMarketGas(
+      kit.connection.defaultFeeCurrency ? { feeCurrency: kit.connection.defaultFeeCurrency } : {}
+    )
+
     await (res.flags.comment
       ? displaySendTx(
           'transferWithComment',
           celoToken.transferWithComment(to, value.toFixed(), res.flags.comment)
         )
-      : displaySendTx('transfer', celoToken.transfer(to, value.toFixed())))
+      : displaySendTx(
+          'transfer',
+          // @ts-expect-error
+          {
+            // NOTE: this used to be celoToken.transfer
+            // but this way ledger considers this a native transfer and show the to and value properly
+            // instead of a contract call
+            send: (_params) =>
+              kit.sendTransaction({ to, value: value.toFixed(), from, ..._params }),
+          },
+          params
+        ))
   }
 }
diff --git a/packages/cli/src/transfer-stable-base.ts b/packages/cli/src/transfer-stable-base.ts
@@ -62,7 +62,7 @@ export abstract class TransferStableBase extends BaseCommand {
     // NOTE 2: if --gasCurrency is set by the user, then
     //     `kit.connection.defaultFeeCurrency` is set in base.ts via
     //     `kit.setFeeCurrency()`
-    const params = kit.connection.defaultFeeCurrency
+    const baseParams = kit.connection.defaultFeeCurrency
       ? { feeCurrency: kit.connection.defaultFeeCurrency }
       : {}
 
@@ -84,7 +84,7 @@ export abstract class TransferStableBase extends BaseCommand {
         }`,
         async () => {
           ;[gas, gasPrice, gasBalance, valueBalance] = await Promise.all([
-            tx.txo.estimateGas(params),
+            tx.txo.estimateGas(baseParams),
             kit.connection.gasPrice(kit.connection.defaultFeeCurrency),
             kit.connection.defaultFeeCurrency
               ? // @ts-expect-error abi typing is not 100% correct but works
@@ -107,6 +107,7 @@ export abstract class TransferStableBase extends BaseCommand {
       )
       .runChecks()
 
+    const params = await kit.connection.setFeeMarketGas(baseParams)
     await displaySendTx(res.flags.comment ? 'transferWithComment' : 'transfer', tx, params)
   }
 }
diff --git a/packages/sdk/connect/src/connection.test.ts b/packages/sdk/connect/src/connection.test.ts
@@ -53,19 +53,38 @@ describe('Connection', () => {
               jsonrpc: '2.0',
             }
           }
+          if (method === 'eth_getBlockByNumber') {
+            return {
+              result: { gasLimit: 0, baseFeePerGas: 42 },
+              id: 24,
+              jsonrpc: '2.0',
+            }
+          }
           return {
             result: 0,
-            id: 24,
+            id: 25,
             jsonrpc: '2.0',
           }
         })
       })
-      it('asked the rpc what they should be', async () => {
-        await connection.setFeeMarketGas({ feeCurrency: '0x000000' })
-        expect(connection.rpcCaller.call).toHaveBeenCalledWith('eth_gasPrice', ['0x000000'])
+      it('asked the rpc what they should be w/ feeCurrency', async () => {
+        const result = await connection.setFeeMarketGas({ feeCurrency: '0x000001' })
         expect(connection.rpcCaller.call).toHaveBeenCalledWith('eth_maxPriorityFeePerGas', [
-          '0x000000',
+          '0x000001',
+        ])
+        expect(connection.rpcCaller.call).toHaveBeenCalledWith('eth_gasPrice', ['0x000001'])
+        expect(result.maxFeePerGas).toEqual('300000')
+        expect(result.maxPriorityFeePerGas).toEqual('200000')
+      })
+      it('asked the rpc what they should be w/o feeCurrency', async () => {
+        const result = await connection.setFeeMarketGas({})
+        expect(connection.rpcCaller.call).toHaveBeenCalledWith('eth_maxPriorityFeePerGas', [])
+        expect(connection.rpcCaller.call).toHaveBeenCalledWith('eth_getBlockByNumber', [
+          'latest',
+          true,
         ])
+        expect(result.maxFeePerGas).toEqual(`0x${BigInt('200042').toString(16)}`)
+        expect(result.maxPriorityFeePerGas).toEqual('200000')
       })
     })
   })

diff --git a/packages/sdk/connect/src/connection.ts b/packages/sdk/connect/src/connection.ts
@@ -334,25 +334,25 @@ export class Connection {
   }
   // if neither gas price nor feeMarket fields are present set them.
   setFeeMarketGas = async (tx: CeloTx): Promise<CeloTx> => {
-    // default to the current values
-    const calls = [Promise.resolve(tx.maxFeePerGas), Promise.resolve(tx.maxPriorityFeePerGas)]
+    if (isEmpty(tx.maxPriorityFeePerGas)) {
+      tx.maxPriorityFeePerGas = await this.getMaxPriorityFeePerGas(tx.feeCurrency)
+    }
 
     if (isEmpty(tx.maxFeePerGas)) {
-      calls[0] = this.gasPrice(tx.feeCurrency)
-    }
-    if (isEmpty(tx.maxPriorityFeePerGas)) {
-      calls[1] = this.rpcCaller
-        .call('eth_maxPriorityFeePerGas', [tx.feeCurrency])
-        .then((rpcResponse) => {
-          return rpcResponse.result
-        })
+      if (isEmpty(tx.feeCurrency)) {
+        const baseFee = await this.getBlock('latest').then((block: any) => block.baseFeePerGas)
+        tx.maxFeePerGas = `0x${(
+          BigInt(baseFee as string) + BigInt(tx.maxPriorityFeePerGas as string)
+        ).toString(16)}`
+      } else {
+        // NOTE: fallback to original estimation
+        tx.maxFeePerGas = await this.gasPrice(tx.feeCurrency)
+      }
     }
-    const [maxFeePerGas, maxPriorityFeePerGas] = await Promise.all(calls)
+
     return {
       ...tx,
       gasPrice: undefined,
-      maxFeePerGas,
-      maxPriorityFeePerGas,
     }
   }
 
@@ -441,6 +441,13 @@ export class Connection {
     return gasPriceInHex
   }
 
+  getMaxPriorityFeePerGas = async (feeCurrency?: Address): Promise<string> => {
+    const parameter = feeCurrency ? [feeCurrency] : []
+    return this.rpcCaller.call('eth_maxPriorityFeePerGas', parameter).then((rpcResponse) => {
+      return rpcResponse.result
+    })
+  }
+
   getBlockNumber = async (): Promise<number> => {
     const response = await this.rpcCaller.call('eth_blockNumber', [])
 

diff --git a/packages/sdk/contractkit/src/wrappers/Validators.test.ts b/packages/sdk/contractkit/src/wrappers/Validators.test.ts
@@ -113,6 +113,7 @@ testWithAnvilL1('Validators Wrapper', (web3) => {
 
     // Set commission update delay to 3 blocks for backwards compatibility
     await setCommissionUpdateDelay(web3, validators.address, 3)
+    await mineBlocks(1, web3)
 
     await validators.setNextCommissionUpdate('0.2').sendAndWaitForReceipt(txOpts)
     await mineBlocks(3, web3)
Original file line number	Diff line number	Diff line change
Expand Up		@@ -34,4 +34,4 @@ value is string \\| number \\| bigint \\| BN

		#### Defined in

		[packages/sdk/connect/src/connection.ts:538](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L538)
		[packages/sdk/connect/src/connection.ts:545](https://github.com/celo-org/developer-tooling/blob/master/packages/sdk/connect/src/connection.ts#L545)