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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EIP3156 FlashLoan support (ERC20 extension) #2540

Closed
Amxx opened this issue Feb 24, 2021 · 11 comments 路 Fixed by #2543
Closed

Add EIP3156 FlashLoan support (ERC20 extension) #2540

Amxx opened this issue Feb 24, 2021 · 11 comments 路 Fixed by #2543

Comments

@Amxx
Copy link
Collaborator

Amxx commented Feb 24, 2021

馃 Motivation
ERC3156 is now final

馃摑 Details
Provide an extension for ERC20 that implements this ERC.

@frangio
Copy link
Contributor

frangio commented Feb 24, 2021

This sounds nice. Have the EIP or an implemenation been audited? Are there production deployments?

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 25, 2021

I'm not 100% sure, @albertocuestacanada might be able to tell use more. Maybe yield uses it.

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 25, 2021

there is this ERC3156 compatible uniswap wrapper: https://ftmscan.com/address/0xa89a83890cc5d43d710846cefcb4a41007a37347#code

@alcueca
Copy link
Contributor

alcueca commented Feb 25, 2021

Hi there!

As all EIPs that make it to Final, ERC3156 has been thoroughly audited pro-bono by an all-star team, including your own Austin Williams.

There are two ERC3156 compliant wrappers in mainnet and fantom, MakerDAO's MIP-25 is compliant, audited, and scheduled to go live soon, and Cover Protocol is also a compliant borrower. WETH10 is scheduled to go live soon and is also a compliant lender.

Yield doesn't use it in v1, but v2 will implement it for sure.

@alcueca
Copy link
Contributor

alcueca commented Feb 25, 2021

Please note that the reference implementation is here: https://github.com/albertocuestacanada/ERC3156

Not to be confused with the ERC3156-Wrappers, which are a personal project:
https://github.com/albertocuestacanada/ERC3156-Wrappers

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 25, 2021

Implementation proposal: https://github.com/Amxx/openzeppelin-contracts/blob/feature/ERC3156/contracts/token/ERC20/extensions/draft-ERC3156.sol

I just wonder why the return value for IERC3156FlashBorrower.onFlashLoan is bytes32(keccak256("ERC3156FlashBorrower.onFlashLoan")) and not bytes4(IERC3156FlashBorrower(0).onFlashLoan.selector) as most other ERC do ...

@alcueca
Copy link
Contributor

alcueca commented Feb 27, 2021

The return value is a sentinel to make sure the lender didn't accidentally call the fallback function. bytes32(keccak256("ERC3156FlashBorrower.onFlashLoan")) sounds like a reasonable choice, returning the function signature would be a bit weak. Is returning the signature as a sentinel a thing?

Maybe you should give a hand to @MicahZoltu in reviewing ERCs. That's quite a detail to know about.

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 27, 2021

Its true that returning a bytes4 is "weaker". This is the de-facto standard for many eip however (721, 777, 1155, 1271, ...)

@MicahZoltu
Copy link
Contributor

I don't see why the seed for a particular sentinel needs to be standardized? It is just "a well known value" and its actual contents don't matter at all.

@Ungolim
Copy link

Ungolim commented Mar 17, 2021

any updates on this?

@Amxx
Copy link
Collaborator Author

Amxx commented Mar 17, 2021

PR #2543 is basically good to go. Should be in 4.1 release.

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 a pull request may close this issue.

5 participants