feat: Add gas analysis scripts for SpokePool methods #61
Conversation
nicholaspai
changed the title
mrice32
left a comment
mrice32
left a comment
There was a problem hiding this comment.
This is really awesome stuff!! Some initial comments. Will look at the rest of the tests next!
| const totalDepositAmount = DEPOSIT_AMOUNT.mul(DEPOSIT_COUNT); | ||
| await seedWallet(depositor, [erc20], weth, totalDepositAmount); |
There was a problem hiding this comment.
We should seed the wallet with more than it will deposit in any test. I think the common case is that users will send < their total balance. Otherwise, the gas costs look better than they would normally be.
There was a problem hiding this comment.
why sending exactly their full balance reduce gas costs..?
There was a problem hiding this comment.
Because the storage slot would be deleted?
| await erc20.connect(depositor).approve(spokePool.address, totalDepositAmount); | ||
| await weth.connect(depositor).approve(spokePool.address, totalDepositAmount); |
There was a problem hiding this comment.
Same with approvals. We should make these larger than we will use in the test.
| if (!process.env.GAS_TEST_ENABLED) this.skip(); | ||
| }); | ||
|
|
||
| beforeEach(async function () { |
There was a problem hiding this comment.
In the beforeEach, can we run at least one deposit for each ERC20? Basically, this "warms" the contract so we get an accurate steady state view on what the gas costs will be. For instance, deposit id 0 -> 1 is 4x as expensive as deposit id 1 -> 2.
| const multicallData = Array(DEPOSIT_COUNT).fill( | ||
| spokePool.interface.encodeFunctionData("deposit", constructDepositParams(erc20.address, currentSpokePoolTime)) |
There was a problem hiding this comment.
We're unlikely to see multicall on the deposit side, so I don't think this should be the expected case. Nonetheless, I think it would be cool to see how much cheaper it would be when we aggregate them.
A future across version could go "full-relay" where we allow depositors to just approve and a relayer does batch deposits on the sending side to save users effort and gas at the cost of a slight delay.
There was a problem hiding this comment.
The full-relay is a good idea, i'll keep this test in for now then
| }); | ||
|
|
||
| beforeEach(async function () { | ||
| [depositor] = await ethers.getSigners(); |
There was a problem hiding this comment.
nit: should this be relayer?
There was a problem hiding this comment.
eh doesn't really matter since I use the same address for the depositor and relayer params in the fillRelay call
There was a problem hiding this comment.
actualy i'll make a new recipient address as I wonder if ERC20 is funky if you're calling transfer to your own address
| const totalRelayAmount = RELAY_AMOUNT.mul(RELAY_COUNT); | ||
| await seedWallet(depositor, [erc20], weth, totalRelayAmount); | ||
|
|
||
| // Approve spokepool to spend tokens | ||
| await erc20.connect(depositor).approve(spokePool.address, totalRelayAmount); | ||
| await weth.connect(depositor).approve(spokePool.address, totalRelayAmount); |
There was a problem hiding this comment.
Similar to the above, we should probably seed and approve with more money than we intend to use.
| beforeEach(async function () { | ||
| [depositor] = await ethers.getSigners(); | ||
| ({ spokePool, weth, erc20 } = await spokePoolFixture()); | ||
|
|
There was a problem hiding this comment.
Can we seed the recipients with tokens too? I think recipients will usually not be an empty wallet, so I think a safer assumption is that they're pre-seeded. If we want to see both cases, we could make some of the cases below seed the recipient and some not.
| if (!process.env.GAS_TEST_ENABLED) this.skip(); | ||
| }); | ||
|
|
||
| beforeEach(async function () { |
There was a problem hiding this comment.
This might not be as impactful in this test, but I think we should probably do something similar to the above and "warm" the contract by running relays for all these ERC20s first. In this case, we might also want to run some deposits too to ensure the contract doesn't have a 0 balance of all these ERC20s (since I would expect that that would not be the common case in prod).
Signed-off-by: nicholaspai <npai.nyc@gmail.com>
chrismaree
left a comment
chrismaree
left a comment
There was a problem hiding this comment.
these are really awesome. can you pose some takeways/show how much things cost?
Sample run: Gas Analytics: HubPool Root Bundle Execution
Pool Rebalance tree with 10 leaves, each containing refunds for 10 different tokens
proposeRootBundle-gasUsed: 110250
✔ Simple proposal
executeRootBundle-gasUsed: 716159
✔ Executing 1 leaf (203ms)
(average) executeRootBundle-gasUsed: 686773
✔ Executing all leaves (1593ms)
(average) executeRootBundle-gasUsed: 499493
✔ Executing all leaves using multicall (1567ms)
Gas Analytics: SpokePool Deposits
ERC20 Deposits
deposit-gasUsed: 68156
✔ 1 Deposit
(average) deposit-gasUsed: 68156
✔ 10 deposits (242ms)
(average) deposit-gasUsed: 23665
✔ 10 deposits using multicall (107ms)
WETH Deposits
deposit-gasUsed: 60333
✔ 1 ETH Deposit
Gas Analytics: SpokePool Relays
ERC20 Relays
fillRelay-gasUsed: 82579
✔ 1 Relay
(average) fillRelay-gasUsed: 82589
✔ 10 Relays (182ms)
(average) fillRelay-gasUsed: 46493
✔ 10 relays using multicall (108ms)
WETH Relays
fillRelay-gasUsed: 97568
✔ 1 Relay
Gas Analytics: SpokePool Relayer Refund Root Execution
(ERC20) Tree with 10 leaves, each containing 10 refunds
relayRootBundle-gasUsed: 76040
✔ Relay proposal
executeRelayerRefundRoot-gasUsed: 145028
✔ Executing 1 leaf (58ms)
(average) executeRelayerRefundRoot-gasUsed: 129290
✔ Executing all leaves (542ms)
(average) executeRelayerRefundRoot-gasUsed: 102361
✔ Executing all leaves using multicall (431ms)
(WETH): Relayer Refund tree with 10 leaves, each containing 10 refunds
executeRelayerRefundRoot-gasUsed: 163618
✔ Executing 1 leaf (57ms)
Gas Analytics: SpokePool Slow Relay Root Execution
(ERC20) Tree with 10 leaves
relayRootBundle-gasUsed: 76028
✔ Relay proposal
executeSlowRelayRoot-gasUsed: 81874
✔ Executing 1 leaf
(average) executeSlowRelayRoot-gasUsed: 98683
✔ Executing all leaves (217ms)
(average) executeSlowRelayRoot-gasUsed: 74210
✔ Executing all leaves using multicall (120ms)
(WETH) Tree with 10 leaves
executeSlowRelayRoot-gasUsed: 91614
✔ Executing 1 leaf |
No description provided.