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

🗞️ Allow expected reverts in verifyAndExecutePackets and fix forge coverage in TestHelper #615

Closed
wants to merge 12 commits into from

Conversation

DanL0
Copy link
Contributor

@DanL0 DanL0 commented May 22, 2024

Depends on:

@DanL0 DanL0 requested a review from St0rmBr3w May 24, 2024 07:56
@St0rmBr3w
Copy link
Contributor

Should we add a revert argument for checking the composed delivery?

@janjakubnanista janjakubnanista added the enhancement New feature or request label May 29, 2024
@TRileySchwarz
Copy link
Contributor

This is also missing fixing the tests, or perhaps this is just PoC for now

@DanL0
Copy link
Contributor Author

DanL0 commented Jun 11, 2024

seems vm.expectRevert Foundry doesn't behave like it's documented in Vm.sol, related issue: foundry-rs/foundry#7629

Reported to Foundry team

@DanL0 DanL0 marked this pull request as ready for review June 11, 2024 15:55
@DanL0 DanL0 requested a review from TRileySchwarz June 11, 2024 15:59
@janjakubnanista janjakubnanista changed the title allow expected reverts in verifyAndExecutePackets and rename 🧹 Allow expected reverts in verifyAndExecutePackets and rename Jun 13, 2024
@janjakubnanista janjakubnanista changed the title 🧹 Allow expected reverts in verifyAndExecutePackets and rename 🗞️ Allow expected reverts in verifyAndExecutePackets and rename Jun 13, 2024
Copy link

socket-security bot commented Jun 25, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@DanL0 DanL0 changed the title 🗞️ Allow expected reverts in verifyAndExecutePackets and rename 🗞️ Allow expected reverts in verifyAndExecutePackets and fix forge coverage in TestHelper Jul 3, 2024
uint256 _packetAmount,
address _composer,
bytes memory _expectedReceiveRevertData,
bytes memory _expectedComposeRevertData,
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont know if I really agree on imbedding the expected revert data into the function like this, is there a functional reason why we dont just rely on the bubbled up revert and catch it with the vm.expectRevert inside of the test files implementation of this contract?

Otherwise it's unconventional compared to how you traditionally would read a testfile using vm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably do that if we expose more granular functions from the TestHelper, eg. split verification and execution of packets. Right now vm.expectRevert only allows to catch revert in next function call or something and it doesnt catch reverts properly

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with @DanL0 , you can't just expectRevert() to be a catch-all for verifyPackets()

@TRileySchwarz
Copy link
Contributor

We should split this PR into seperate smaller ones:

One for refactoring the endpoint setup to be more verbose. Excluding the "executeAndVerify" portion

@DanL0
Copy link
Contributor Author

DanL0 commented Jul 29, 2024

We should split this PR into seperate smaller ones:

One for refactoring the endpoint setup to be more verbose. Excluding the "executeAndVerify" portion

#791

@DanL0 DanL0 closed this Jul 29, 2024
@janjakubnanista janjakubnanista deleted the test-revert-logic branch August 9, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

forge coverage fails to run on OApp
5 participants