Skip to content

Conversation

@nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented Jan 19, 2024

This PR was created by running forge install foundry-rs/forge-std which created a local lib/ directory to store
useful forge contracts for testing, like forge-std. I also created a new directory test/foundry to store foundry tests from here on out.

I've been running the following test script with forge test --fork-url https://mainnet.infura.io/v3/<KEY> -vvvv (the four "v" flags are for for maximum logging)

Fixes ACX-1827

nicholaspai and others added 17 commits June 13, 2023 20:27
Signed-off-by: nicholaspai <npai.nyc@gmail.com>
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: nicholaspai <npai.nyc@gmail.com>
This reverts commit af8716b.
@nicholaspai nicholaspai requested a review from mrice32 January 19, 2024 17:35
@linear
Copy link

linear bot commented Jan 19, 2024

@nicholaspai nicholaspai marked this pull request as ready for review January 19, 2024 20:15
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +136 to +138
vm.prank(pauseAdmin);
hubPoolProxy.setPaused(true);
assertTrue(hubPool.paused());
Copy link
Contributor

Choose a reason for hiding this comment

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

Two nits:

  1. Do you want to test calling it with the default admin as well?
  2. Should we reset it to unpaused after the test to ensure other tests aren't impacted by the state change?

Copy link
Member Author

Choose a reason for hiding this comment

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

assertEq(address(hubPool).balance, balBefore);
}

/// forge-config: default.fuzz.runs = 300
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a cool use of fuzzing!

FYI, solidity compilation will fail if function selectors collide: https://ethereum.stackexchange.com/a/46188/47801. So the HubPool, by definition can't have colliding selectors.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh ok, so we really only need to compare the Proxy functions with the HubPool functions then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep!

Copy link
Contributor

Choose a reason for hiding this comment

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

But collisions are a superset of that case, so collisions are fine to test for -- just wanted to mention it.

vm.stopPrank();
}

function testFallback() public {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great test! +1 for covering this case.

@nicholaspai nicholaspai requested a review from mrice32 January 21, 2024 16:35
…re robust fuzz testing of fallback()/receive()

Signed-off-by: nicholaspai <npai.nyc@gmail.com>
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

vm.prank(defaultAdmin);
(bool success, ) = address(hubPoolProxy).call{ value: 1 ether }("");
assertTrue(success);
assertEq(address(hubPool).balance, balBefore);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we also check the balance of the proxy to make sure the ETH got through?

}

function testFallback() public {
function testTransferOwnership() public {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

hubPoolProxy.setPaused(false);
assertFalse(hubPool.paused());

// Multiple EOA's can be granted the pause role.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, good call

@nicholaspai nicholaspai requested a review from mrice32 January 25, 2024 16:05
@nicholaspai nicholaspai merged commit 67a4450 into master Jan 27, 2024
@nicholaspai nicholaspai deleted the npai/permission-splitter-test branch January 27, 2024 17:27
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.

3 participants