feat: internal feedback #66
Conversation
ChefMist
changed the title
| @@ -15,6 +15,15 @@ fs_permissions = [ | |||
| evm_version = 'cancun' | |||
| bytecode_hash = "none" | |||
|
|
|||
| # more info about compiler_profiles and restrictions here https://github.com/foundry-rs/foundry/pull/8668 | |||
There was a problem hiding this comment.
Saw u deleted it in the end. Curious would we even have a bigger optimizer_runs if we keep it ?
There was a problem hiding this comment.
my bad -- added back in https://github.com/pancakeswap/pancake-v4-periphery/pull/66/commits/9df3f60086f382fa95ca78cb5e889e1766477aeb
the bytecode size test for CLPositionManager was wrong, i used manager which was CLPoolManager. after updating to CLPositionManager, it became 28kb (larger than 24kb limit)
so added back s41 to compile 9000 runs for CLPositionManager and the rest having 1_000_000 runs
| snapSize("BinMigratorBytecodeSize", address(migrator)); | ||
|
|
||
| // forge coverage will run with '--ir-minimum' which set optimizer run to min | ||
| // thus we do not want to revert for forge coverage case |
There was a problem hiding this comment.
Only forge test will run with FOUNDRY_PROFILE, is it ?
There was a problem hiding this comment.
yeh and only in CI -- ref: https://github.com/pancakeswap/pancake-v4-periphery/blob/main/.github/workflows/test.yml#L31
| function test_initcodeHash() public { | ||
| vm.snapshotValue( | ||
| "BinPositionManager initcode hash (without constructor params, as uint256)", | ||
| uint256(keccak256(type(BinPositionManager).creationCode)) |
There was a problem hiding this comment.
can i ask if this initCodeHash for create3 ?
There was a problem hiding this comment.
will remove these test -- ur right, seems like for our create3 which is creationCode + param, it will be hard to match probably.. maybe will investigate seperately if we want to achieve this (if required)
…ive snapshot (#67) * s52: remove gasSnapshot and use native snapshot * feat: remove _getContractName and use testName directly * forge test after merge
ChefMist
changed the title
|
closing this PR. splitting into multiple PR for easier to review |
This PR does the following based on internal feedback
s41: tried to use
compilation restrictionso we can increase optimizer_runs, in the end we can just increase without that. so have bumped up optimizer_runs (which reduces gas)s43: add comment to code based on natspec format
s46: add code hash (similar to v4-core) for posm and migrator which user will interact with
s48: use directive with global eg.
using Planner for Plan global;so we don't have to typeusing Planner for Plans50: use
internal constantto make it obvious (and follow other files convention)Pre-requisite
Current built failing as GasSnapshot not found (already removed in v4-core), so this PR will pass once the below 3 items are done
v4-coredependency to main