feat: superchainerc20 tests by 0xDiscotech · Pull Request #5 · defi-wonderland/optimism

0xDiscotech · 2024-07-15T16:44:22Z

CLOSES OPT-143

linear · 2024-07-15T16:44:31Z

OPT-143 Create superc20 contract unit tests

0xDiscotech · 2024-07-15T16:46:01Z

Tried to stick to their testing standard. But I'm unsure if the testFuzz_mint_zeroAddressTo_reverts, testFuzz_burn_zeroAddressFrom_reverts and testFuzz_relayERC20_zeroAddressTo_reverts are needed since they're not Unit tests. But I've seen a similar behavior on other tests so I think it doesn't harm to keep them if we are in doubt.

0xDiscotech · 2024-07-15T16:46:37Z

+
+        // Mint some tokens to `_from` so then they can be burned
+        vm.prank(BRIDGE);
+        superchainERC20.mint(_from, _amount);


deal() didn't work here so I used the mint()

I believe it's okay. I'm not sure if deal increases the totalSupply, and the SuperchainERC20 tokens start with a totalSupply of 0. If you don't mint, the totalSupply would underflow. Either way, how did you attempt to use deal here, and with what error did it fail?

deal(address(SuperchainERC20), _from, _amount) didn't work?

0xDiscotech · 2024-07-15T16:47:35Z

+
+        // Mint some tokens to the sender so then they can be sent
+        vm.prank(BRIDGE);
+        superchainERC20.mint(_sender, _amount);


Same here, deal() didn't work here so I used mint()

0xng · 2024-07-15T17:39:12Z

Tried to stick to their testing standard. But I'm unsure if the testFuzz_mint_zeroAddressTo_reverts, testFuzz_burn_zeroAddressFrom_reverts and testFuzz_relayERC20_zeroAddressTo_reverts are needed since they're not Unit tests. But I've seen a similar behavior on other tests so I think it doesn't harm to keep them if we are in doubt.

Let's leave them for now. They shouldn't be tested, as this is functionality incorporated by OZ, but as long as we use OZ it's ok to have them. If we switch to solady or solmate, we will need to remove it (plus they should fail if we do so)

0xng · 2024-07-15T17:07:06Z

+        vm.assume(_caller != BRIDGE);
+
+        // Expect the revert with `CallerNotBridge` selector
+        vm.expectRevert(abi.encodeWithSelector(CallerNotBridge.selector));


no need to encode it, should allow you to do vm.expectRevert(CallerNotBridge.selector);

0xng · 2024-07-15T17:11:07Z

+
+        // Call the `mint` function with the zero address
+        vm.prank(BRIDGE);
+        address _to = address(0);


check if there's a ZERO_ADDRESS constant, or create it otherwise and replace, you can probably do some:
``superchainERC20.mint({_to: ZERO_ADDRESS, _amount})` kind of thing to have it as a one liner

0xng · 2024-07-15T17:22:39Z

+
+        // Mint some tokens to `_from` so then they can be burned
+        vm.prank(BRIDGE);
+        superchainERC20.mint(_from, _amount);


I believe it's okay. I'm not sure if deal increases the totalSupply, and the SuperchainERC20 tokens start with a totalSupply of 0. If you don't mint, the totalSupply would underflow. Either way, how did you attempt to use deal here, and with what error did it fail?

deal(address(SuperchainERC20), _from, _amount) didn't work?

0xng · 2024-07-15T17:28:07Z

+        bytes memory _message = abi.encodeCall(superchainERC20.relayERC20, (_to, _amount));
+        _mockAndExpect(
+            MESSENGER,
+            abi.encodeWithSelector(


abi.encodeCall here would be tidier

0xng · 2024-07-15T17:29:33Z

+        // Expect the revert with `CallerNotBridge` selector
+        vm.expectRevert(abi.encodeWithSelector(RelayMessageCallerNotL2ToL2CrossDomainMessenger.selector));
+
+        // Call the `sendERC20` function with the non-messenger caller


Suggested change

// Call the `sendERC20` function with the non-messenger caller

// Call the `relayERC20` function with the non-messenger caller

0xng · 2024-07-15T17:29:54Z

+        // Ensure the caller is not the messenger
+        vm.assume(_caller != MESSENGER);
+
+        // Expect the revert with `CallerNotBridge` selector


Suggested change

// Expect the revert with `CallerNotBridge` selector

// Expect the revert with `RelayMesasgeCallerNotL2ToL2CrossDomainMessenger` selector

0xng · 2024-07-15T17:30:19Z

+        assertEq(superchainERC20.balanceOf(_sender), _senderBalanceBefore - _amount);
+    }
+
+    /// @dev Tests the `sendERC20` function reverts when the caller is not the bridge.


Suggested change

/// @dev Tests the `sendERC20` function reverts when the caller is not the bridge.

/// @dev Tests the `relayERC20` function reverts when the caller is not the L2ToL2CrossDomainMessenger.

0xDiscotech · 2024-07-15T17:43:04Z

Tried to stick to their testing standard. But I'm unsure if the testFuzz_mint_zeroAddressTo_reverts, testFuzz_burn_zeroAddressFrom_reverts and testFuzz_relayERC20_zeroAddressTo_reverts are needed since they're not Unit tests. But I've seen a similar behavior on other tests so I think it doesn't harm to keep them if we are in doubt.

Let's leave them for now. They shouldn't be tested, as this is functionality incorporated by OZ, but as long as we use OZ it's ok to have them. If we switch to solady or solmate, we will need to remove it (plus they should fail if we do so)

Since this pov, I'm more inclined to removing those tests. But lmk which is your decision 👍

0xDiscotech · 2024-07-15T17:45:52Z

deal(address(SuperchainERC20), _from, _amount) didn't work?

Yes, I tested using deal() in that way and the revert is bc of the underflow:

0xng · 2024-07-15T20:44:11Z

    }

-    /// @dev Tests the `burn` succeeds and emits the `Burn` event.
+    /// @dev Tests the `burn` bruns the amount and emits the `Burn` event.


burns is mispelled here

0xng · 2024-07-15T20:45:37Z

+        // Mock the call over the `sendMessage` function and expect it to be called properly
+        _mockAndExpect(
+            MESSENGER,
+            abi.encodeWithSelector(


hehe i meant using abi.encodeCall instead of abi.encodeWithSelector not moving the message declaration within the body, but it's ok

ah jaja, will undo that and use abi.encodeCall then

…thereum-optimism#19272) * contracts: implement audit code fixes and add tests Add onlyDelegateCall enforcement to upgradeSuperchain, upgrade, and migrate functions (#17). Include msg.sender in deploy salt to prevent cross-caller CREATE2 collisions (#17). Add duplicate instruction key detection in upgrade validation (#9). Validate startingRespectedGameType against enabled game configs (#10). Add code-existence check in loadBytes (#18). Add setUp guard to VerifyOPCM.runSingle (#4). Remove unused _findChar function (#5). Pass real AddressManager in migrator proxy deploy args (#11). Add tests covering all audit fix behaviors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * contracts: regenerate semver-lock.json for OPContractsManagerV2 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * contracts: bump OPContractsManagerV2 version to 7.0.10 Semver-diff requires a patch version bump when bytecode changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

0xDiscotech added 8 commits July 12, 2024 22:33

feat: add constructor and mint function tests

97cee42

feat: add burn function tests

06ab7c7

feat: add decimals function test

4743657

Merge branch 'superc20' into feat/superchainerc20-tests

35707ea

feat: add tests for send and relay erc20 functions

127b761

chore: remove console log on contract

0da6fc1

chore: update test functions names

59b5e0a

feat: add test on zero to

a1bbd6e

0xDiscotech requested review from 0xng and hexshire July 15, 2024 16:44

0xDiscotech self-assigned this Jul 15, 2024

0xDiscotech commented Jul 15, 2024

View reviewed changes

0xng reviewed Jul 15, 2024

View reviewed changes

0xDiscotech added 2 commits July 15, 2024 14:52

fix: comments

96d18ba

chore: remove selector encoding on expected reverts

9540a5d

0xDiscotech requested a review from 0xng July 15, 2024 18:13

0xng reviewed Jul 15, 2024

View reviewed changes

chore: fix misspell

5cdee63

0xDiscotech requested a review from 0xng July 15, 2024 21:46

0xng approved these changes Jul 16, 2024

View reviewed changes

hexshire approved these changes Jul 16, 2024

View reviewed changes

0xDiscotech merged commit 6ff5ecb into superc20 Jul 16, 2024

0xDiscotech deleted the feat/superchainerc20-tests branch July 16, 2024 16:19

	// Call the `sendERC20` function with the non-messenger caller
	// Call the `relayERC20` function with the non-messenger caller

	// Expect the revert with `CallerNotBridge` selector
	// Expect the revert with `RelayMesasgeCallerNotL2ToL2CrossDomainMessenger` selector

	/// @dev Tests the `sendERC20` function reverts when the caller is not the bridge.
	/// @dev Tests the `relayERC20` function reverts when the caller is not the L2ToL2CrossDomainMessenger.

Conversation

0xDiscotech commented Jul 15, 2024

Uh oh!

linear bot commented Jul 15, 2024

Uh oh!

0xDiscotech commented Jul 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xng commented Jul 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xDiscotech commented Jul 15, 2024

Uh oh!

0xDiscotech commented Jul 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants