test: add comprehensive MultiSigSigner tests (ithacaxyz#382)#36
Conversation
* chore: add test script without upgrade tests, because they are slow * chore: remove husky because running tests before every commit is slow, sometimes you want to merge commits without contracts building, and forge fmt doesn't work because the files are not added to staging * chore: add Benchmark updates to CI * fix: CI * chore: update package.json * fix: ci * chore: update gas snapshots * chore: add formatting to CI * chore: use standardized commit logic in version bump CI
* feat: add multicall to simulator * chore: add gas test * chore: review fixes * chore: fmt contracts and update gas snapshots
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Dargon789, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's testing infrastructure and introduces advanced simulation features. It includes a new, extensive test suite for the Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant number of tests, including comprehensive tests for MultiSigSigner, upgrade scenarios, and multicall simulations, which greatly enhances the project's test coverage and robustness. Additionally, it includes numerous formatting improvements across the codebase for better readability. My review identified a bug in Simulator.sol where gas consumption was being misreported in a specific failure scenario.
| for (uint256 i = 0; i < results.length - 1; i++) { | ||
| // If any call failed, we return gasUsed = 0, multicall3Gas, and the error data from that call | ||
| if (!results[i].success) { | ||
| return (0, 0, results[i].returnData); |
There was a problem hiding this comment.
In the case of a pre-call failure within _callMulticall3, the function returns 0 for multicall3Gas. However, multicall3Gas has already been calculated and represents the gas spent on the aggregate3 call itself. Returning 0 is misleading and loses important gas accounting information. The function should return the actual multicall3Gas value even when a pre-call fails. The comment on line 180 also suggests that multicall3Gas should be returned.
return (0, multicall3Gas, results[i].returnData);
|
@Mergifyio refresh |
✅ Pull request refreshed |
4 participants
No description provided.