add gas measurement to tests and refactor some test structures #337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alex0207s
commented
Jan 2, 2025
| @@ -1,8 +1,6 @@ | |||
| [submodule "lib/openzeppelin-contracts"] | |||
| path = lib/openzeppelin-contracts | |||
| url = https://github.com/OpenZeppelin/openzeppelin-contracts | |||
| branch = v5.0.2 | |||
There was a problem hiding this comment.
The branch entry is an optional setting. However, its inclusion causes forge update to fail when updating dependencies.
Reference: foundry-rs/foundry#3720 (comment)
alex0207s
commented
Jan 2, 2025
| } | ||
| } | ||
|
|
||
| contract EIP712TestContract is EIP712 { | ||
| contract EIP712Harness is EIP712 { |
There was a problem hiding this comment.
A best pratice for tseting internal fuction: https://book.getfoundry.sh/tutorials/best-practices?highlight=har#test-harnesses
alex0207s
commented
Jan 2, 2025
| @@ -14,7 +14,6 @@ import { IUniswapSwapRouter02 } from "test/utils/IUniswapSwapRouter02.sol"; | |||
| import { MockStrategy } from "test/mocks/MockStrategy.sol"; | |||
| import { GenericSwap } from "contracts/GenericSwap.sol"; | |||
| import { AllowanceTarget } from "contracts/AllowanceTarget.sol"; | |||
| import { TokenCollector } from "contracts/abstracts/TokenCollector.sol"; | |||
alex0207s
commented
Jan 2, 2025
| @@ -24,18 +23,6 @@ import { ISmartOrderStrategy } from "contracts/interfaces/ISmartOrderStrategy.so | |||
| contract GenericSwapTest is Test, Tokens, BalanceUtil, Permit2Helper, SigHelper { | |||
| using BalanceSnapshot for Snapshot; | |||
|
|
|||
| event Swap( | |||
There was a problem hiding this comment.
Use the Swap event from the IGenericSwap interface instead of declaring it in the test.
alex0207s
commented
Jan 2, 2025
| @@ -132,7 +136,7 @@ contract FillTest is LimitOrderSwapTest { | |||
| Snapshot memory fcMakerToken = BalanceSnapshot.take({ owner: feeCollector, token: defaultOrder.makerToken }); | |||
|
|
|||
| uint256 takingAmount = defaultOrder.takerTokenAmount / 2; | |||
| uint256 makingAmount = defaultOrder.takerTokenAmount / 2; | |||
| uint256 makingAmount = defaultOrder.makerTokenAmount / 2; | |||
There was a problem hiding this comment.
It should be makerTokenAmount instead of takerTokenAmount.
alex0207s
commented
Jan 2, 2025
| @@ -1,7 +1,6 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity 0.8.26; | |||
|
|
|||
| import { ILimitOrderSwap } from "contracts/interfaces/ILimitOrderSwap.sol"; | |||
alex0207s
commented
Jan 2, 2025
| @@ -10,8 +10,6 @@ import { ISmartOrderStrategy } from "contracts/interfaces/ISmartOrderStrategy.so | |||
| import { IUniswapSwapRouter02 } from "test/utils/IUniswapSwapRouter02.sol"; | |||
| import { Constant } from "contracts/libraries/Constant.sol"; | |||
| import { BalanceSnapshot, Snapshot } from "test/utils/BalanceSnapshot.sol"; | |||
| import { UniswapV2Library } from "test/utils/UniswapV2Library.sol"; | |||
| import { UniswapV3 } from "test/utils/UniswapV3.sol"; | |||
alex0207s
commented
Jan 2, 2025
| @@ -13,9 +13,9 @@ import { ISmartOrderStrategy } from "contracts/interfaces/ISmartOrderStrategy.so | |||
| import { ILimitOrderSwap } from "contracts/interfaces/ILimitOrderSwap.sol"; | |||
| import { TokenCollector } from "contracts/abstracts/TokenCollector.sol"; | |||
| import { Constant } from "contracts/libraries/Constant.sol"; | |||
| import { RFQOffer, getRFQOfferHash } from "contracts/libraries/RFQOffer.sol"; | |||
alex0207s
commented
Jan 2, 2025
| import { RFQTx } from "contracts/libraries/RFQTx.sol"; | ||
| import { LimitOrder, getLimitOrderHash } from "contracts/libraries/LimitOrder.sol"; |
alex0207s
commented
Jan 5, 2025
|
|
||
| uint256 balance = token.balanceOf(address(this)); | ||
| assertEq(balance, amount); | ||
| } | ||
|
|
||
| /* Token Permit */ | ||
|
|
||
| TokenPermit DEFAULT_TOKEN_PERMIT = |
There was a problem hiding this comment.
move to the setup function
NIC619
reviewed
Jan 7, 2025
test/AllowanceTarget.t.sol
Outdated
| allowanceTarget.spendFromUserTo(user, address(mockERC20), recipient, amount); | ||
| vm.stopPrank(); | ||
| vm.snapshotGasLastCall("AllowanceTarget", "testSpendFromUserTo: spendFromUserTo()"); |
There was a problem hiding this comment.
nit: move the called function to the front would make it easier to compare in snapshot file
Suggested change
| vm.snapshotGasLastCall("AllowanceTarget", "testSpendFromUserTo: spendFromUserTo()"); | |
| vm.snapshotGasLastCall("AllowanceTarget", "spendFromUserTo(): testSpendFromUserTo"); |
There was a problem hiding this comment.
Which format is better: targetFunction() | Test Function: testFunction() or targetFunction(): testFunction ?
There was a problem hiding this comment.
I think second one will do.
NIC619
approved these changes
Jan 13, 2025
alex0207s
force-pushed
the
test/add-gas-snapshot
branch
from
56723b9 to
9b4afdb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update the
forge-stdlibrary to v1.9.4, which supports the snapshotGasLastCall cheatcode.Add the
snapshotGasLastCallcheatcode to tests for measuring gas usage.Refactor test structures to enhance consistency. (e.g. replace
vm.prank()withvm.startPrank()andvm.stopPrank()).Fix a bug in a test function where
takerTokenAmountandmakerTokenAmountwere misused.