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

Add customizable fee receiver to ERC20FlashMint #3327

Merged
merged 9 commits into from May 6, 2022
Merged

Add customizable fee receiver to ERC20FlashMint #3327

merged 9 commits into from May 6, 2022

Conversation

mazenkhalil
Copy link
Contributor

@mazenkhalil mazenkhalil commented Apr 7, 2022

Fixes #3316
Allow implementers to set the address for which the flash loan fee amount is transferred to. Instead of burning the loan fee.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@mazenkhalil mazenkhalil changed the title ERC3156 Flash loans extension #3316 ERC3156 Flash loans extension improvement #3316 Apr 7, 2022
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

This is looking pretty good, thank you. Some minor comments.

contracts/mocks/ERC20FlashMintFeeReceiverMock.sol Outdated Show resolved Hide resolved
contracts/mocks/ERC20FlashMintMock.sol Outdated Show resolved Hide resolved
contracts/token/ERC20/extensions/ERC20FlashMint.sol Outdated Show resolved Hide resolved
contracts/token/ERC20/extensions/ERC20FlashMint.sol Outdated Show resolved Hide resolved
@frangio frangio changed the title ERC3156 Flash loans extension improvement #3316 Add customizable fee receiver to ERC20FlashMint Apr 7, 2022
}

function flashFee(address token, uint256 amount) public view virtual override returns (uint256) {
super.flashFee(token, amount);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This super call is not needed and can be confusing, even if it is just a mock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this super call is to cover the validation defined in the super implementation. I have opened another ticket to fix the flashFee function #3331. Upon that change this super call won't be required anymore.

Amxx
Amxx previously requested changes Apr 11, 2022
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

Great work overall. There are a few things regarding testing that can be improved.

We also need a Changelog entry before we merge that PR.

@frangio frangio dismissed Amxx’s stale review April 12, 2022 23:27

Comments have been addressed

@frangio frangio enabled auto-merge (squash) April 12, 2022 23:27
frangio
frangio previously approved these changes Apr 12, 2022
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thank you @mazenkhalil. This looks good.

@Amxx Amxx added this to the 4.7 milestone May 2, 2022
@frangio frangio disabled auto-merge May 6, 2022 21:46
@frangio frangio merged commit 3b9381d into OpenZeppelin:master May 6, 2022
@mazenkhalil mazenkhalil deleted the fix/erc3156-flash-loans-extension-improvement-#3316 branch May 9, 2022 08:29
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.

ERC3156 Flash loans extension improvement
3 participants