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

fix:tests #28

Merged
merged 7 commits into from
Oct 17, 2024
Merged

fix:tests #28

merged 7 commits into from
Oct 17, 2024

Conversation

livingrockrises
Copy link
Contributor

revamp (WIP) test cases and helpers a bit to avoid running into stack too deep issues.

for further work on this repo,
keep these in mind

i. Bundling related parameters into struct - those are used repeatedly.
ii. Minimize number of local variables and identify areas where it can be block scoped.
iii. Offload intermediate variables into memory to avoid direct stack operations
iv. Review order of variable initialisation
v. can work on further breaking down complex functions.

Note that paymaster validation gas limit, callGasLimit, verificationGasLimit estimations are currently skipped. either ways they can not be accurate without using gas estimation package.
unaccountedGasCost should be kept appropriate value after testing, when there is premium to be charge unaccountedGas needs to be higher value (also depending on warm/cold slot of fee collector)

Copy link
Collaborator

@filmakarov filmakarov left a comment

Choose a reason for hiding this comment

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

approving with minor suggestions

test/base/TestBase.sol Outdated Show resolved Hide resolved
PackedUserOperation[] memory ops = new PackedUserOperation[](1);
(PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, priceMarkup);
(PackedUserOperation memory userOp, bytes32 userOpHash) = createUserOp(ALICE, bicoPaymaster, 1_100_000, 100_000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above, name params for more readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was avoiding local vars :/

test/unit/fuzz/TestFuzz_TestSponsorshipPaymaster.t.sol Outdated Show resolved Hide resolved
@livingrockrises livingrockrises merged commit d2c8096 into develop Oct 17, 2024
3 checks passed
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.

2 participants