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

Challenge 4: fix error message for openzeppelin v5 #247

Closed
rin-st opened this issue Dec 13, 2024 · 2 comments
Closed

Challenge 4: fix error message for openzeppelin v5 #247

rin-st opened this issue Dec 13, 2024 · 2 comments
Assignees

Comments

@rin-st
Copy link
Member

rin-st commented Dec 13, 2024

And remove openzeppelin v4 dependency

context: #245 (comment)

@rin-st rin-st self-assigned this Dec 20, 2024
@rin-st
Copy link
Member Author

rin-st commented Dec 20, 2024

Cause of the error:
I had this line in my solution code
require(token.transfer(msg.sender, tokenOutput), "ethToToken(): reverted swap.");

Since ERC20 transfer should return boolean and it worked before. But from current version ERC20.sol of openzeppelin

We have followed general OpenZeppelin Contracts guidelines: functions revert instead returning `false` on failure. This behavior is nonetheless  conventional and does not conflict with the expectations of ERC20 applications.

transfer follows that, and then in my code token.transfer(msg.sender, tokenOutput) is not boolean when there is an error inside transfer and hence error notification is broken

So if I just change that line everything works fine.

So I removed openzeppelin v4 requirement from the code 6cbf937

Closing the issue

cc @damianmarti

@rin-st rin-st closed this as completed Dec 20, 2024
@damianmarti
Copy link
Member

Cause of the error: I had this line in my solution code require(token.transfer(msg.sender, tokenOutput), "ethToToken(): reverted swap.");

Since ERC20 transfer should return boolean and it worked before. But from current version ERC20.sol of openzeppelin

We have followed general OpenZeppelin Contracts guidelines: functions revert instead returning `false` on failure. This behavior is nonetheless  conventional and does not conflict with the expectations of ERC20 applications.

transfer follows that, and then in my code token.transfer(msg.sender, tokenOutput) is not boolean when there is an error inside transfer and hence error notification is broken

So if I just change that line everything works fine.

So I removed openzeppelin v4 requirement from the code 6cbf937

Closing the issue

cc @damianmarti

Great! Thank you @rin-st !

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

No branches or pull requests

2 participants