initial version of gas analysis script #37

nicholaspai · 2022-02-16T18:42:19Z

can be run via yarn test:gas

…art-contracts-v2 into npai/gas-analysis

nicholaspai · 2022-02-16T18:46:09Z

test/gas-analytics/HubPool.RefundExecution.ts

+}
+
+// Construct tree with REFUND_CHAIN_COUNT leaves, each containing REFUND_TOKEN_COUNT sends
+async function constructSimpleTree() {


Before adding more code to this script and more test cases, I'm curious what you guys think about this being on the right track for gas analysis? Currently it only allows you create REFUND_CHAIN_COUNT count of leaves, each with REFUND_TOKEN_COUNT rebalances triggered by the HubPool.executeRelayerRefund method

chrismaree · 2022-02-16T18:55:05Z

test/gas-analytics/HubPool.RefundExecution.ts

+import {
+  toBNWei,
+  toBN,
+  SignerWithAddress,
+  seedWallet,
+  Contract,
+  ethers,
+  getContractFactory,
+  BigNumber,
+  randomAddress,
+  expect,
+} from "../utils";


I think it's cleaner to do two imports for this kind of thing, rather than taking 12 lines.

If you do import * as utils from "../utils" then you need to call utils.expect and utils.Contract for types, which seems annoying. Were you thinking of something different?

chrismaree · 2022-02-16T18:56:31Z

test/gas-analytics/HubPool.RefundExecution.ts

+
+describe("Gas Analytics: HubPool Relayer Refund Execution", function () {
+  before(async function () {
+    if (!process.env.GAS_TEST_ENABLED) this.skip();


janky but works well! good

chrismaree · 2022-02-16T18:58:30Z

test/gas-analytics/HubPool.RefundExecution.ts

+
+    // Now that we've verified that the transaction succeeded, let's compute average gas costs.
+    const receipt = await txn.wait();
+    console.log(receipt.cumulativeGasUsed);


perhaps add some additional strings to this? something like cumlativeGasUsed: or something.

Yeah just starting from here, I'll log it

mrice32

This is awesome! Love the way you're approaching the package.json scripts.

Have we figured out a strategy for estimating gas costs in different environments (arbitrum, optimism, etc)? Note: this would only be relevant for SpokePool tests, but was just curious if you had thought about that.

mrice32 · 2022-02-16T20:59:28Z

package.json

    "lint-fix": "yarn prettier --write",
    "prettier": "prettier .",
-    "test": "REPORT_GAS=true yarn hardhat test"
+    "test": "REPORT_GAS=true yarn hardhat test",


What does REPORT_GAS=true do? If it's the hardhat module, should we turn this off for normal test runs (to reduce runtime) and have a separate call for hardhat gas reporting when you want it?

Yeah good call

mrice32 · 2022-02-16T21:35:59Z

test/gas-analytics/HubPool.RefundExecution.ts

+
+    // Deploy test tokens for each chain ID
+    for (let i = 0; i < REFUND_CHAIN_COUNT; i++) {
+      destinationChainIds.push(i);


For multiple tests, I don't think this array ever gets reset, so you would end up with [1 ... n, 1 ... n, 1 ...n, etc] after a few runs.

ahh great catch

mrice32 · 2022-02-16T21:37:06Z

test/gas-analytics/HubPool.RefundExecution.ts

+    const leafIndexToExecute = 0;
+
+    await hubPool.connect(dataWorker).initiateRelayerRefund(
+      [3117], // bundleEvaluationBlockNumbers used by bots to construct bundles. Length must equal the number of leafs.


What's 3117?

a random block number, I'll set this to something in consts to be more clear

mrice32 · 2022-02-16T21:39:04Z

test/gas-analytics/HubPool.RefundExecution.ts

+      expect(await l1Tokens[leafIndexToExecute][i].balanceOf(mockAdapter.address)).to.equal(SEND_AMOUNT);
+    }
+
+    // Now that we've verified that the transaction succeeded, let's compute average gas costs.


nit: is this supposed to be an average?

this ones. not an avrerage, the next test case will execute all leaves i'll fix the comment

mrice32 · 2022-02-16T21:39:20Z

test/gas-analytics/HubPool.RefundExecution.ts

+    const { leafs, tree } = await constructSimpleTree();
+    const leafIndexToExecute = 0;
+
+    await hubPool.connect(dataWorker).initiateRelayerRefund(


Should we be measuring the gas cost of this call as well?

…art-contracts-v2 into npai/gas-analysis

nicholaspai · 2022-02-17T00:25:05Z

Here's the latest run of yarn hardhat test:gas-analytics:

  Gas Analytics: HubPool Relayer Refund Execution
    Tree with 10 Leaves, each containing refunds for 10 different tokens
initiateRelayerRefund-cumulativeGasUsed: 195704
executeRelayerRefund-cumulativeGasUsed: 981935
      ✓ Executing 1 leaf
initiateRelayerRefund-cumulativeGasUsed: 200627
(average) executeRelayerRefund-cumulativeGasUsed: 949458
      ✓ Executing all leaves

mrice32

LGTM! One minor comment

mrice32 · 2022-02-17T19:18:11Z

test/gas-analytics/HubPool.RefundExecution.ts

+
+      // Now that we've verified that the transaction succeeded, let's compute average gas costs.
+      const receipts = await Promise.all(txns.map((_txn) => _txn.wait()));
+      const cumulativeGasUsed = receipts.map((_receipt) => _receipt.cumulativeGasUsed).reduce((x, y) => x.add(y));


Wherever we're computing gas, I think we should use gasUsed, not cumulativeGasUsed. The latter is the gas used in this transaction and all preceding transactions in the same block. Not sure this will be a problem in hardhat since it usually maps each transaction to its own block, but since that's not guaranteed we probably just want to use gasUsed to be safe.

Source: https://ethereum.stackexchange.com/a/3354/47801

…art-contracts-v2 into npai/gas-analysis

Authored by Matt & Nick. Signed-off-by: Matt Rice <matthewcrice32@gmail.com> Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com> Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai added 12 commits February 14, 2022 10:47

Reduce size of uint states

830bcdf

fix tests

cfc18cb

Update hardhat.config.ts

d8b0552

Update package.json

b95bfbb

add arbitrum spokepool skeleton

ebee27e

WIP

f91967d

wip

c71e51d

Update package.json

4fd0999

Merge branch 'master' of https://github.com/across-protocol/across-sm…

f708ab1

…art-contracts-v2 into npai/gas-analysis

refactor

5fef28a

Merge branch 'master' of https://github.com/across-protocol/across-sm…

362cde8

…art-contracts-v2 into npai/gas-analysis

remove scripts

4bbb49b

nicholaspai commented Feb 16, 2022

View reviewed changes

nicholaspai marked this pull request as ready for review February 16, 2022 18:52

chrismaree reviewed Feb 16, 2022

View reviewed changes

mrice32 approved these changes Feb 16, 2022

View reviewed changes

nicholaspai added 3 commits February 16, 2022 16:17

Merge branch 'master' of https://github.com/across-protocol/across-sm…

920096d

…art-contracts-v2 into npai/gas-analysis

Update package.json

541ca27

Update HubPool.RefundExecution.ts

58a09c9

nicholaspai requested review from chrismaree and mrice32 February 17, 2022 00:25

mrice32 approved these changes Feb 17, 2022

View reviewed changes

nicholaspai added 3 commits February 23, 2022 14:20

Merge branch 'master' of https://github.com/across-protocol/across-sm…

aadbbb0

…art-contracts-v2 into npai/gas-analysis

Update HubPool.RefundExecution.ts

f2184aa

Update HubPool.RefundExecution.ts

9757efa

nicholaspai merged commit c6445c6 into master Feb 23, 2022

nicholaspai deleted the npai/gas-analysis branch February 23, 2022 21:34

initial version of gas analysis script #37

initial version of gas analysis script #37

Uh oh!

Conversation

nicholaspai commented Feb 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaspai commented Feb 17, 2022

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

mrice32 Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mrice32 Feb 17, 2022 •

edited

Loading