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

Liquidator: add Sanctum swap #919

Merged
merged 12 commits into from
Apr 10, 2024
Merged

Liquidator: add Sanctum swap #919

merged 12 commits into from
Apr 10, 2024

Conversation

farnyser
Copy link
Contributor

No description provided.

@farnyser farnyser force-pushed the serge/lst-swap branch 4 times, most recently from 5723575 to 66bdbc8 Compare March 21, 2024 08:27
@github-actions github-actions bot added the dependency Dependency updates label Mar 22, 2024
@farnyser farnyser changed the title Liquidator: add Sanctum swap (draft) Liquidator: add Sanctum swap Mar 25, 2024
@farnyser farnyser marked this pull request as ready for review March 25, 2024 07:01
bin/liquidator/src/rebalance.rs Show resolved Hide resolved
}
}

let results = futures::future::join_all(jobs).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

future pr: instead of configuring individual timeouts, it could be good to give a general quote timeout that applies here

bin/liquidator/src/rebalance.rs Outdated Show resolved Hide resolved
let accounts: Vec<Account> = live_rpc_client
.get_multiple_accounts(&lookup_table.addresses)
.await?
.drain(..)
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't iter() do the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iter() give a &Option<T> and I want a Option<T>

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, into_iter() then? :)

bin/liquidator/src/rebalance.rs Outdated Show resolved Hide resolved
bin/liquidator/src/rebalance.rs Outdated Show resolved Hide resolved
let in_price = get_or_fetch_price(r.input_mint);
let out_price = get_or_fetch_price(r.output_mint);
let amount = out_price * I80F48::from_num(r.out_amount)
- in_price * I80F48::from_num(r.in_amount);
Copy link
Contributor

@ckamm ckamm Mar 30, 2024

Choose a reason for hiding this comment

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

mmh, this is not a sufficient criterion now that you've removed the favoring of the primary route

say someone wants to swap mSOL -> KIN. Due to the configuration we also check the alternate route mSOL -> SOL. This latter route is near guaranteed to have better amount than the one to KIN. But it will still often do worse if you factor in the SOL -> KIN second step that's needed afterwards.

suggestion: two step approach?

  • fetch the routes between the correct in and out mints first (including sanctum if enabled and it's both LSTs) and find the best route (by amount) that fits in a transaction
  • if this fails, fetch the alternate routes (which are just the first step of a multi-part route) and grab the best one of those the same way

that also reduces the number of quote requests sent to the swap endpoints (being rate limited is a real concern)

lib/client/src/swap/sanctum.rs Outdated Show resolved Hide resolved
lib/client/src/swap/sanctum.rs Show resolved Hide resolved
}
}

async fn load_lst(&mut self, live_rpc_client: &RpcClient) -> anyhow::Result<HashSet<Pubkey>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

this does not even need to take a self argument anymore

let accounts: Vec<Account> = live_rpc_client
.get_multiple_accounts(&lookup_table.addresses)
.await?
.drain(..)
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, into_iter() then? :)

.await;

match req_res {
Ok(v) => Ok(keys.drain(..).zip(v.value).collect::<Vec<_>>()),
Copy link
Contributor

Choose a reason for hiding this comment

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

really think the .drain(..) looks odd, into_iter() again?

Copy link
Contributor

Choose a reason for hiding this comment

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

minor: this possibly got overlooked

lib/client/src/gpa.rs Outdated Show resolved Hide resolved
lib/client/src/gpa.rs Outdated Show resolved Hide resolved
.drain(..)
.map(|v| v.unwrap())
.flatten()
.filter_map(|x| x.1.map(|o| (x.0, o)))
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer: .filter_map(|(pubkey, account_opt)| account_opt.map(|acc| (pubkey, acc))) (but even that needs some thought to read, unfortunately)

Is it even acceptable to not return data for some of the accounts that were requested? Could you put in the doc comment that this is could return no data without an error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd appreciate the comment

bin/liquidator/src/rebalance.rs Outdated Show resolved Hide resolved
bin/liquidator/src/rebalance.rs Outdated Show resolved Hide resolved
bin/liquidator/src/rebalance.rs Outdated Show resolved Hide resolved
bin/liquidator/src/rebalance.rs Outdated Show resolved Hide resolved
bin/liquidator/src/rebalance.rs Outdated Show resolved Hide resolved
@farnyser farnyser merged commit 01d5237 into dev Apr 10, 2024
13 checks passed
@farnyser farnyser deleted the serge/lst-swap branch April 10, 2024 09:35
farnyser added a commit that referenced this pull request Apr 18, 2024
liquidator: add sanctum swap
(cherry picked from commit 01d5237)
microwavedcola1 pushed a commit that referenced this pull request Apr 22, 2024
* liquidator: randomly select token/perps for rebalancing to avoid failing at every try if one token is having an issue (#921)

(cherry picked from commit e3a7ed9)

* liquidator: do not panic if token or perp rebalancing fails (#927)

(cherry picked from commit 0b7e62e)

* Liquidator: add Sanctum swap (#919)

liquidator: add sanctum swap
(cherry picked from commit 01d5237)

* liquidator: add more LST for sanctum swap (#944)

(cherry picked from commit c0b61b3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency Dependency updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants