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 EIP: Flash Loans #7400

Closed
wants to merge 25 commits into from
Closed

Conversation

alcueca
Copy link
Contributor

@alcueca alcueca commented Jul 25, 2023

Draft for a new Flash Loans ERC. Based on 3156, it offers more flexibility by segregating the asset and control flows. The pull architecture has also been changes to a push architecture.

@alcueca alcueca requested a review from eth-bot as a code owner July 25, 2023 16:18
@github-actions github-actions bot added c-new Creates a brand new proposal c-update Modifies an existing proposal s-draft This EIP is a Draft s-final This EIP is Final t-erc labels Jul 25, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jul 25, 2023

File EIPS/eip-7399.md

Requires 1 more reviewers from @axic, @Pandapip1, @SamWilsn, @xinbenlv

@eth-bot eth-bot changed the title Flash loans v2 Add EIP: Flash Loans Jul 25, 2023
@eth-bot eth-bot added the e-consensus Waiting on editor consensus label Jul 25, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jul 25, 2023
@github-actions github-actions bot removed c-update Modifies an existing proposal s-final This EIP is Final w-ci Waiting on CI to pass labels Jul 25, 2023
@eth-bot eth-bot added the e-review Waiting on editor to review label Jul 25, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jul 25, 2023
EIPS/eip-7400.md Outdated
/// @param fee The fee to be paid
/// @param data The ABI encoded data to be passed to the callback
/// @return result ABI encoded result of the callback
function(address, address, IERC20, uint256, uint256, bytes memory) external returns (bytes memory) callback
Copy link
Contributor

Choose a reason for hiding this comment

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

function callbacks are not an officially defined type in the ABIv2 spec, so this behavior is potentially undefined and specific to how Solidity handles it internally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I wanted to ask you specifically about this.

I would prefer to do the specification in YAML, and I assume in that case the callback would be split into a callback selector and a callback address.

Then, I assume that in the solidity reference implementation I could use a callback type, while in a potential vyper implementation of would be to separate variables.

Would that be acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, I believe this code produces a ABI tuple (address, bytes4) which would be more ideal for an ERC. You could make a note that Solidity will interpret a function pointer as a tuple and vice versa, and also mention the constraints that the function should have (i.e. it returns bytes, it's nonpayable, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

nope, nevermind it gets encoded as bytes24 according to docs:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@alcueca alcueca Jul 25, 2023

Choose a reason for hiding this comment

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

Often there are several possible flows in the flash loan callback (leverage/deleverage, borrow/repay, etc) which the user needs to define an enum for, then encode in data, then decode in the callback, to finally route the flow to the right spot. Example.

Allowing the callback signature to be user defined removes this, allowing the user to specify that he wants the callback to be Foo.bar or Foo.baz.

It looks complicated in the spec, but not so much in the implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also allows arbitrary callback target, is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this standard aims to segregate the asset and control flows on flash loans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some encoding and decoding of function types to bytes24 here. It does produce much cleaner code than using .call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please let's continue this conversation here, I'd like to merge this PR and link the .md as soon as possible to avoid confusion between the PR number and the EIP number.

EIPS/eip-7400.md Outdated Show resolved Hide resolved
EIPS/eip-7400.md Outdated Show resolved Hide resolved
EIPS/eip-7399.md Outdated Show resolved Hide resolved
alcueca and others added 4 commits July 27, 2023 08:19
Co-authored-by: Andrew B Coathup <28278242+abcoathup@users.noreply.github.com>
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jul 27, 2023
@fubuloubu
Copy link
Contributor

@fubuloubu, is there a way to give a kick to @eth-bot to review this?

No idea

@alcueca
Copy link
Contributor Author

alcueca commented Jul 29, 2023

@abcoathup?

Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

Partial review

EIPS/eip-7399.md Outdated Show resolved Hide resolved
EIPS/eip-7399.md Outdated Show resolved Hide resolved
EIPS/eip-7399.md Outdated Show resolved Hide resolved
EIPS/eip-7399.md Show resolved Hide resolved
EIPS/eip-7399.md Show resolved Hide resolved
EIPS/eip-7399.md Outdated Show resolved Hide resolved
EIPS/eip-7399.md Show resolved Hide resolved
EIPS/eip-7399.md Outdated Show resolved Hide resolved
EIPS/eip-7399.md Show resolved Hide resolved
EIPS/eip-7399.md Outdated Show resolved Hide resolved
EIPS/eip-7399.md Outdated Show resolved Hide resolved
EIPS/eip-7399.md Outdated Show resolved Hide resolved
Pandapip1 and others added 3 commits July 29, 2023 20:05
* Rename Lender Specification to IERC7399
* Better comments on Lender Specification
* More detail on the rationale for a function type callback
@github-actions
Copy link

The commit bc26809 (as a parent of b31f1ae) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jul 31, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Aug 7, 2023
@alcueca alcueca requested a review from Pandapip1 August 7, 2023 14:48
EIPS/eip-7399.md Outdated

```

The `flashFee` function MUST return the fee charged for a loan of `amount` `asset`. If the asset is not supported `flashFee` MUST revert. If the lender doesn't have enough liquidity to loan `amount` the fee returned must be `type(uint256).max`.
Copy link

Choose a reason for hiding this comment

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

For some contracts, the difference between a "not supported" asset and an asset for which the lender has zero liquidity can be unclear. For example UniV4 singleton contract: it technically supports all assets, but it can have zero liquidity for some of them. So should it reverts or return type(uint).max in this case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case I would return type(uint).max, since the asset is technically supported but has no liquidity.

The rationale is that borrowers will usually know which assets they can get from each lender, but call maxFlashLoan anyway to find out if they can borrow a particular amount at a particular time. Reverting on no liquidity would be bad UX.

Returning type(uint).max on flashFee follows the same logic. Borrowers are expected to know what they are doing and where to get each asset from. They can bypass the maxFlashLoan check and use flashFee instead to get both data points (enough liquidity and fee to pay) in a single call. A revert on flashFee is unexpected and notifies the borrower that they are doing something wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, this is a great point, and probably needs to be added to the EIP. In the wrappers we have just released I'm not following a consistent approach on flashFee for Aave (probably needs to revert more), Balancer (shouldn't revert) and Uniswap (shouldn't revert).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the EIP, thanks for your feedback 👍


Arbitrary callback functions on callback receivers allows to implement different behaviours to flash loans on callback receivers without the need for encoding a function router using the `data` argument. A function call type is 24 bytes of which the first 20 bytes are the target address and the last 4 bytes are the function signature.

The `amount + fee` are pushed to the `payment receiver` to allow for the segregation of asset and control flows. While a "pull" architecture is more prevalent, "push" architectures are also common. For those cases where the `lender` can't implement a "push" architecture, a simple wrapper contract can offer an ERC-7399 external interface, while using liquidity from the `lender` using a "pull" architecture.
Copy link

Choose a reason for hiding this comment

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

I think that push architectures are compatible with less contracts. Let's say that you have a lending pool which exposes an EIP-7399 flash, a supply and a withdraw entry-point. One could drain the contract by doing: flash(amount: x) then in the callback call supply(amount: x), which returns the assets to the lending pool, so the balance check passes and the flashLoan can end. Then the user is able call withdraw(amount: x) and get back all the supplied funds.

There are some ways to prevent this, but those are much less direct than implementing a pull flashLoan.

Copy link
Contributor Author

@alcueca alcueca Aug 9, 2023

Choose a reason for hiding this comment

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

A push architecture is not particularly challenging to build. In your example, the call to supply should account for the amount used in some way, so that it cannot be used as well for repaying the loan. That's how Uniswap, Balancer and Euler flash loans work (or worked), for example.

pull architectures are more common, but are also more inefficient when integrating several contracts and moving assets around. With ERC-7399 we want flash loans to be powerful and efficient, so a push architecture works better.

If you must use a pull architecture, you can just use some purpose-built holding contract that the borrower can push the repayment to, and that the lender can pull the repayment from.

@alcueca
Copy link
Contributor Author

alcueca commented Aug 14, 2023

@Pandapip1, could I get a hand here, please?

@SamWilsn
Copy link
Contributor

I am closing this pull request because we are in the process of separating EIPs and ERCs into distinct repositories. Unfortunately, as far as we are aware, GitHub does not provide any tools to ease this migration, so every pull request will need to be re-opened manually.

As this is a PR to create / modify an ERC, I will kindly ask you to redirect this to the new repository at ethereum/ERCs. We have prepared a guide to help with the process.

If there is relevant history here, please link to this PR from the new pull request.

On behalf of the EIP Editors, I apologize for this inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants