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

maint(ct): Fix test broken by Foundry update #1687

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

RPate97
Copy link
Collaborator

@RPate97 RPate97 commented Apr 18, 2024

Purpose

This fixes a minor issue in one of our tests that was introduced by this PR in Foundry. This issue does not effect our service for any users.

Copy link
Contributor

mergify bot commented Apr 18, 2024

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

@@ -209,6 +209,6 @@ contract ManagedService_Test is Test, ManagedService {

function test_RevertIfOwnerIsAddressZero() external {
vm.expectRevert("ManagedService: admin cannot be address(0)");
new ManagedService{ salt: keccak256("1") }(address(0));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The update to Foundry to use the create2 factory in the simulation caused this test to break since the error if the create2 factory is used differs from the error we expect on line 211. I resolved this issue just by deploying via the normal create opcode.

For reference, this is the result if the create2 factory is used:
Screenshot 2024-04-18 at 3 55 21 PM

@RPate97 RPate97 requested a review from sam-goldman April 18, 2024 23:03
Copy link
Collaborator

@sam-goldman sam-goldman left a comment

Choose a reason for hiding this comment

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

Looks good. I think this PR resolves this ticket

@RPate97 RPate97 merged commit 426b71e into develop Apr 19, 2024
8 checks passed
@RPate97 RPate97 deleted the pate/fix-managed-service-test branch April 19, 2024 21:48
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