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.

Sign up for GitHub

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

Jump to bottom

[4337 Module] Increase test coverage to 99+% #87

Closed
rmeissner opened this issue Oct 5, 2023 · 3 comments · Fixed by #108
Closed

[4337 Module] Increase test coverage to 99+% #87

rmeissner opened this issue Oct 5, 2023 · 3 comments · Fixed by #108
Assignees
@nlordell

Comments

@rmeissner
Copy link
Member

rmeissner commented Oct 5, 2023 •
edited
Loading

  • Update hardhat version
  • All branches need to be covered
  • (optional) Add separate tests using latest bytecode for entrypoint on official address
@rmeissner rmeissner self-assigned this Oct 5, 2023
@rmeissner rmeissner assigned nlordell and unassigned rmeissner Oct 16, 2023
@mmv08 mmv08 mentioned this issue Oct 19, 2023
Closed
This was referenced Oct 19, 2023
Closed
Merged
@nlordell
Copy link
Collaborator

Updating hardhat version being worked on in #100
Reduced code to cover in #104

nlordell added a commit that referenced this issue Oct 23, 2023
@nlordell
Increase Coverage By Removing Code (#104)
8a887dd 
In implementing #87, I realised that a lot of the missing code coverage
by lines were in the `UserOperationLib` source file. Since this is only
used by test contracts (in particular the test entry point and the mock
4337 Safe), I moved the code around/cleaned up dead code to increase the
coverage of the "main" contracts. In particular:

* The `UserOperation` struct definition moved to the 4337 interface file
(felt natural as the struct is part of the 4337 account interface)
* Moved the `UserOperationLib` library to the `test/` subdirectory, the
only place it was being used
* I also got rid of some dead code, library functions that weren't being
used anywhere
* Deduplicated the `Account` interface from the test entry point
* Fixed some Solidity warning lints (that weren't causing CI errors -
should we change CI to fail on these warnings as well?)
@nlordell nlordell mentioned this issue Oct 24, 2023
Merged
nlordell added a commit that referenced this issue Oct 24, 2023
@nlordell
Increase branch Coverage (#108)
1826c56 
Partially fixes #87

This PR adds unit tests to cover the remaining branches from the
`SimpleEIP4337Module` contract. With this, we should have 100% coverage
for the core contracts in the repo (although, not all paths in the test
contracts are covered, which should be fine).

In terms of test organisation, I added a `EIP4337Module.spec.ts` for
unit tests directly targeting the module functions (`validateUserOp` and
`execUserOp*`).
@nlordell
Copy link
Collaborator

Looks like GitHub auto-closed this issue with the #108 PR ("partially fixes $ISSUE" is interpreted as "fixes $ISSUE") 🙈.

@nlordell nlordell reopened this Oct 25, 2023
@nlordell nlordell mentioned this issue Oct 25, 2023
Merged
@nlordell
Copy link
Collaborator

Closing as the required tasks are implemented. The good to have isn't quite done, but there is a draft PR open for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nlordell nlordell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Increase branch Coverage safe-global/safe-modules
2 participants
@rmeissner @nlordell