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

feat(flash-loan): add flash loan #168

Merged
merged 14 commits into from
Jul 28, 2023
Merged

feat(flash-loan): add flash loan #168

merged 14 commits into from
Jul 28, 2023

Conversation

Rubilmax
Copy link
Contributor

@Rubilmax Rubilmax commented Jul 24, 2023

@makcandrov
Copy link
Contributor

I'd argue that #167 or #166 (from #120) already implements flashloans

I still think it is better to implement a separate flashLoan function. It is more intuitive, elegant, and gas-efficient (gas refunds are capped at 20%, so not really useful).

@Rubilmax
Copy link
Contributor Author

I still think it is better to implement a separate flashLoan function. It is more intuitive, elegant, and gas-efficient (gas refunds are capped at 20%, so not really useful).

Just realized that anyway callbacks limit flash loans to a single market's liquidity, which is not enough. So there's no alternative to flash loans actually.

@Rubilmax Rubilmax marked this pull request as ready for review July 24, 2023 14:10
pakim249CAL
pakim249CAL previously approved these changes Jul 24, 2023
@makcandrov
Copy link
Contributor

I thought so too, but in fact, you're not capped by the liquidity because you initially "virtually supplied" the liquidity to the market, so there won't be any liquidity issue when withdrawing

src/Blue.sol Outdated Show resolved Hide resolved
src/libraries/SafeTransferLib.sol Outdated Show resolved Hide resolved
src/Blue.sol Outdated Show resolved Hide resolved
src/Blue.sol Show resolved Hide resolved
src/Blue.sol Outdated Show resolved Hide resolved
src/interfaces/IFlashBorrower.sol Show resolved Hide resolved
src/interfaces/IFlashLender.sol Show resolved Hide resolved
src/mocks/FlashBorrowerMock.sol Outdated Show resolved Hide resolved
test/hardhat/Blue.spec.ts Outdated Show resolved Hide resolved
src/mocks/FlashBorrowerMock.sol Outdated Show resolved Hide resolved
src/interfaces/IFlashBorrower.sol Outdated Show resolved Hide resolved
src/interfaces/IFlashBorrower.sol Outdated Show resolved Hide resolved
src/interfaces/IFlashLender.sol Show resolved Hide resolved
src/interfaces/IFlashBorrower.sol Outdated Show resolved Hide resolved
src/Blue.sol Outdated Show resolved Hide resolved
Co-authored-by: MathisGD <74971347+MathisGD@users.noreply.github.com>
Signed-off-by: Romain Milon <rmilon@gmail.com>
Rubilmax and others added 4 commits July 25, 2023 09:56
Co-authored-by: MathisGD <74971347+MathisGD@users.noreply.github.com>
Signed-off-by: Romain Milon <rmilon@gmail.com>
Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com>
Signed-off-by: Romain Milon <rmilon@gmail.com>
Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com>
Signed-off-by: Romain Milon <rmilon@gmail.com>
Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com>
Signed-off-by: Romain Milon <rmilon@gmail.com>
Rubilmax and others added 2 commits July 25, 2023 11:04
Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com>
Signed-off-by: Romain Milon <rmilon@gmail.com>
@Rubilmax
Copy link
Contributor Author

Pushed a new, non-ERC3156-compliant version. I find it weird that we ignore the input of one of our expected integrator: Contango. Their usecase seems to be generalizable and cannot actually be solved using transient storage. Currently, they are required to use storage, which costs a lot compared to their suggested solution, that we refuse to implement for the sake of minimalism.

@makcandrov
Copy link
Contributor

makcandrov commented Jul 25, 2023

If we are not ERC3156 compliant (which I'm in favor of), I think it's better to change the flashloan function name and the callback name so that they are not misleading and no mistakes can be made by the integrators

@Rubilmax
Copy link
Contributor Author

Pushed a new, non-ERC3156-compliant version. I find it weird that we ignore the input of one of our expected integrator: Contango. Their usecase seems to be generalizable and cannot actually be solved using transient storage. Currently, they are required to use storage, which costs a lot compared to their suggested solution, that we refuse to implement for the sake of minimalism.

Also note that if we apply this to flash loans, we should apply this to callbacks for #120 too (usecases are equivalent).

If we are not ERC3153 compliant (which I'm in favor of), I think it's better to change the flashloan function name and the callback name so that they are not misleading and no mistakes can be made by the integrators

Note that signatures are different! But I'm ok with both solution.

src/Blue.sol Show resolved Hide resolved
src/Blue.sol Show resolved Hide resolved
@makcandrov
Copy link
Contributor

Also note that if we apply this to flash loans, we should apply this to callbacks for #120 too (usecases are equivalent).

I agree that we could do that, although I'm not sure if everyone is okay with returning an additional variable that is almost always empty.

Note that signatures are different! But I'm ok with both solution.

The signature is different, but the flashloan function's selector is still the same (the return type doesn't influence the selector).

@Rubilmax
Copy link
Contributor Author

Rubilmax commented Jul 26, 2023

Just realized other protocols expose a function where you can input multiple tokens to flash loan across multiple assets at once. What do you think about this? Should we do the same? What are the downsides?

@MathisGD
Copy link
Contributor

Im against at a first sight. It adds complexity to the function / interaction for unknown usecases, that you can quite easily cover by just calling an other flashloan in your callback function.

@Rubilmax
Copy link
Contributor Author

Rubilmax commented Jul 26, 2023

Im against at a first sight. It adds complexity to the function / interaction for unknown usecases, that you can quite easily cover by just calling an other flashloan in your callback function.

I didn't get your point, can you elaborate/rephrase it pls? What other flashloan would you call in which callback function?

@MathisGD
Copy link
Contributor

If you need to flashloan 2 tokens you can just call an other flashloan in your callback of the first one.

@pakim249CAL
Copy link
Contributor

If you need to flashloan 2 tokens you can just call an other flashloan in your callback of the first one.

This would incur gas costs in multiple loops of calls between contracts though, which can add up quickly if there is a desire to flashloan many tokens.

@MathisGD
Copy link
Contributor

A warm call is only 100. We don't even have one usecase in mind for flash loaning multiple tokens (right ?). I'm strongly against.

src/interfaces/IERC20.sol Outdated Show resolved Hide resolved
src/Blue.sol Show resolved Hide resolved
@MathisGD MathisGD merged commit 752f383 into main Jul 28, 2023
2 checks passed
@MathisGD MathisGD deleted the feat/flashloan branch July 28, 2023 07:57
@MathisGD MathisGD mentioned this pull request Jul 28, 2023
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.

Implement flashloans
6 participants