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

Improved test case for testRevertsIfTransferFromFails #83

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

a1111198
Copy link

This PR updates the test case testRevertsIfTransferFromFails to improve clarity and avoid confusion regarding the endogenous vs. exogenous nature of the contracts involved.

Changes Made:

  • Renamed mock contract to mockWethTransferFailed for better clarity.
  • Adjusted the test setup to differentiate the collateral contract from the DSC contract.

Diff:

// Arrange - Setup
address owner = msg.sender;
vm.prank(owner);
-MockFailedTransferFrom mockDsc = new MockFailedTransferFrom();
-tokenAddresses = [address(mockDsc)];
+MockFailedTransferFrom mockWethTransferFailed = new MockFailedTransferFrom();
+tokenAddresses = [address(mockWethTransferFailed)];
feedAddresses = [ethUsdPriceFeed];
vm.prank(owner);
-DSCEngine mockDsce = new DSCEngine(tokenAddresses, feedAddresses, address(mockDsc));
-mockDsc.mint(user, amountCollateral);
+DSCEngine mockDsce = new DSCEngine(tokenAddresses, feedAddresses, address(dsc));
+mockWethTransferFailed.mint(user, amountCollateral);

// Arrange - User
vm.startPrank(user);
-ERC20Mock(address(mockDsc)).approve(address(mockDsce), amountCollateral);
+ERC20Mock(address(mockWethTransferFailed)).approve(address(mockDsce), amountCollateral);
// Act / Assert
vm.expectRevert(DSCEngine.DSCEngine__TransferFailed.selector);
-mockDsce.depositCollateral(address(mockDsc), amountCollateral);
+mockDsce.depositCollateral(address(mockWethTransferFailed), amountCollateral);
vm.stopPrank();

By making these changes, the test case now more clearly represents an exogenous collateral scenario, reducing potential confusion.

@PatrickAlphaC
Copy link
Member

Could you change it so the formatting isn't different? You should be able to run forge fmt to get the same formatting as me.

@a1111198
Copy link
Author

a1111198 commented Jul 2, 2024

I've run forge fmt to standardize the formatting and committed the changes

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