Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Part 2 of 2] feat: internal feedback: remove gasSnapshot and use native snapshot #67

Merged
merged 4 commits into from
Jan 25, 2025

Conversation

ChefMist
Copy link
Collaborator

This PR remove gasSnapshot and use native snapshot

Built as separate PR so its easier to review

@ChefMist ChefMist mentioned this pull request Jan 23, 2025
8 tasks
@ChefMist ChefMist changed the title [Part 2 of 2] feat: internal feedback [Part 2 of 2] feat: internal feedback: remove gasSnapshot and use native snapshot Jan 24, 2025
migrator.multicall(data);
snapEnd();
vm.snapshotGasLastCall(string(abi.encodePacked(_getContractName(), "#testMigrateFromV2IncludingInit")));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why would we need _getContractName here since by default the snapshot file already named as test file name

Copy link
Contributor

@chef-omelette chef-omelette Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and somehow the snapshot is not generated

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, yeh _getContractName() no longer needed since each gas snapshot stored under file name, thanks!

Copy link
Contributor

@chef-omelette chef-omelette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CleanShot 2025-01-24 at 17 23 56@2x
I ran the code locally and found that the number of snapshot files should be 20 instead of only 10

@chef-omelette
Copy link
Contributor

should we add husky pre-commit hook for snapshots auto generation and git commit?

@ChefMist ChefMist mentioned this pull request Jan 25, 2025
@ChefMist
Copy link
Collaborator Author

husky pre-commit hook for snapshots auto generation and git commit

created this issue to track - https://github.com/pancakeswap/pancake-v4-periphery/issues/69 i think there's pro & cons, lets discuss internally first 👍

@ChefMist
Copy link
Collaborator Author

@ChefMist ChefMist merged commit b41a48d into feat/internal-feeback Jan 25, 2025
1 of 2 checks passed
@ChefMist ChefMist deleted the feat/internal-feeback-v2 branch January 25, 2025 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants