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: flashloans #105

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

feat: flashloans #105

wants to merge 20 commits into from

Conversation

zavgorodnii
Copy link
Contributor

@zavgorodnii zavgorodnii commented Jun 6, 2024

This PR adds the neutron-flashloans contract that uses another address as the source of funds (in our case, the DAO core address). You can find a detailed description of the contract logic and implementation in README.md, or in the docs.

Note: the neutron-flashloans-user contract that is only used in the integration tests is added to the neutron-dev-contracts repo: neutron-org/neutron-dev-contracts#57.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It also adds the neutron-flashloans-user contract that is only used in the integration tests.

It should be in our dev-contracts repo then.

requested_amount: Vec<Coin>,
) -> Result<Vec<Coin>, ContractError> {
// Prepare the query
let all_balances_query = BankQuery::AllBalances {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is safe.

I can spam the source address with an infinite amount of TF tokens, so after the certain number of created tokens your AllBalances query will return a gas error, thus users can't get new flashloans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you suggest having an allow list of tokens that we are ready to loan?

Copy link
Collaborator

@pr0n00gler pr0n00gler Jun 18, 2024

Choose a reason for hiding this comment

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

Either that, or do a separate BankQuery::Balance { address: String, denom: String } query for each denom.
That way a flashloan can be broken only if a user himself provides a lot of tokens to loan. But it's kinda his problem, not contract's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I like the separate query approach more


// Filter all balances leaving only the balances of the requested coins
let mut pre_loan_balances: Vec<Coin> = vec![];
for requested_coin in requested_amount {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or i can get a gas error here.

TLDR infinite loops are not good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, spamming might be an issue; will fix.

contracts/dao/neutron-flashloans/src/contract.rs Outdated Show resolved Hide resolved
contracts/dao/neutron-flashloans/src/contract.rs Outdated Show resolved Hide resolved
contracts/dao/neutron-flashloans/src/contract.rs Outdated Show resolved Hide resolved
expected_balances: Vec<Coin>,
) -> Result<(), ContractError> {
// Prepare the query
let all_balances_query = BankQuery::AllBalances {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same about a potentially infinite loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged, will fix!

let maybe_source_coin = all_balances_response
.amount
.iter()
.find(|x| x.denom == requested_coin.denom && requested_coin.amount.le(&x.amount));
Copy link
Contributor

Choose a reason for hiding this comment

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

not important but seems kinda inefficient, since if there is not enough of denom it will still check all the other coins (even thought it won't find it?)

contracts/dao/neutron-flashloans/src/contract.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@sotnikov-s sotnikov-s left a comment

Choose a reason for hiding this comment

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

please don't forget to move the flashloan user contract to the dev contracts repo

contracts/dao/neutron-flashloans/src/msg.rs Outdated Show resolved Hide resolved
contracts/dao/neutron-flashloans/src/contract.rs Outdated Show resolved Hide resolved
contracts/dao/neutron-flashloans/src/contract.rs Outdated Show resolved Hide resolved
contracts/dao/neutron-flashloans/src/contract.rs Outdated Show resolved Hide resolved
contracts/dao/neutron-flashloans/src/contract.rs Outdated Show resolved Hide resolved
contracts/dao/neutron-flashloans/src/contract.rs Outdated Show resolved Hide resolved
contracts/dao/neutron-flashloans/src/contract.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@sotnikov-s sotnikov-s left a comment

Choose a reason for hiding this comment

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

all but this tiny issue LGTM

Comment on lines +196 to +200
fn get_pre_loan_balances(
deps: &DepsMut,
source: Addr,
requested_amount: Vec<Coin>,
) -> Result<Vec<Coin>, ContractError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

please update comments in the func body. some of them are outdated

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.

4 participants