Support multiple routes in astroport adapter #99
Conversation
| let return_denom = match routes | ||
| .last() | ||
| .and_then(|last_route| last_route.operations.last()) | ||
| { | ||
| Some(last_op) => last_op.denom_out.clone(), |
There was a problem hiding this comment.
Yeah let's make sure we do validation on entrypoint that the first op denom / offer asset of all routes must be the same, and the last op denom of all routes must be the same in the entrypoint. Then this line is safe
| let offer_asset = get_current_asset_available(&deps, &env, &operation.denom_in)?; | ||
| let offer_asset = match offer_asset { | ||
| Some(offer_asset) => offer_asset, | ||
| None => get_current_asset_available(&deps, &env, &operation.denom_in)?, |
There was a problem hiding this comment.
I believe there's an edge case here, ex:
- 100 OSMO in, 2 route trade, each route has an in of 50 OSMO
- First route is: (Osmo->Atom, Atom->Stars, Stars->Osmo, Osmo->USDC)
- That last operation (Osmo->USDC) would use all the available Osmo asset, leaving none for the 2nd route?
The only real reason why a certain route would be given (instead of just the last osmo->usdc) is if the prior operations net more Osmo and that's why it's the better swap route. So either we want to disallow these types of routes in validation or we will need a diff way to do accounting (accounting differences in balance after each swap)
There was a problem hiding this comment.
damn I didn't know we did routes like this, ok I'll handle this differently
packages/skip/src/swap.rs
Outdated
| AstroportPoolSwap { | ||
| operation: SwapOperation, | ||
| offer_asset: Option<Asset>, | ||
| }, // Only used for the astroport swap adapter contract |
There was a problem hiding this comment.
let's move these comments above the variant now, inline comments look good in one liners, not so much w/ multiliner
| @@ -171,22 +157,40 @@ fn execute_swap( | |||
| return Err(ContractError::Unauthorized); | |||
| } | |||
|
|
|||
| // reset the pre swap out asset amount | |||
| PRE_SWAP_OUT_ASSET_AMOUNT.save(deps.storage, &Uint128::new(0))?; | |||
There was a problem hiding this comment.
I'm pretty sure in theory this isn't needed, but might be good to be safe
There was a problem hiding this comment.
Why in theory is this not needed? Just because we don't expect coinhall to ever give us routes that touch the same denom out as the offer asset?
There was a problem hiding this comment.
And for this to be correct shouldn't this query the contract's balance for the first hop's denom out instead of 0? Which ideally is 0 most of the time but doesn't seem like we should always assume it's 0
There was a problem hiding this comment.
I believe it doesn't query the balance because it does it later, but let me recheck the logic
There was a problem hiding this comment.
Ok now that I'm looking at the code again, it's not querying the balance in execute_swap because it queries the balance in execute_astroport_pool_swap.
And this is why I said that it's not needed in theory, because whatever value gets saved here, gets overwritten anyways.
|
approved. left some comments in response to your PRE_SWAP_OUT_ASSET_AMOUNT comment |
* Revert "Add split route support in all adapters (#103)" This reverts commit 1fa4f21. * Revert "Support multiple routes in astroport adapter (#99)" This reverts commit a95f17d. * Revert "Support split routes in entry point contract (#101)" This reverts commit 94c3bbf. * Revert "Update contract types to support route splitting (#98)" This reverts commit ec1fd57. * Revert "add interface field to SwapOperation (#96)" This reverts commit eb37618.
* add SmartSwapExactAssetIn swap type * Revert "Add split route support in all adapters (#103)" This reverts commit 1fa4f21. * Revert "Support multiple routes in astroport adapter (#99)" This reverts commit a95f17d. * Revert "Support split routes in entry point contract (#101)" This reverts commit 94c3bbf. * Revert "Update contract types to support route splitting (#98)" This reverts commit ec1fd57. * Revert "add interface field to SwapOperation (#96)" This reverts commit eb37618. * remove deployed test contracts * fix fmt issue * update schema * since user_swap is mutatable, we can mutate in place * swap excess funds * update schema * lint change * fix test * add optional interface field to SwapOperation (#108) * return error for largest_route_index --------- Co-authored-by: Jeremy Liu <[email protected]>
* add SmartSwapExactAssetIn swap type * Revert "Add split route support in all adapters (#103)" This reverts commit 1fa4f21. * Revert "Support multiple routes in astroport adapter (#99)" This reverts commit a95f17d. * Revert "Support split routes in entry point contract (#101)" This reverts commit 94c3bbf. * Revert "Update contract types to support route splitting (#98)" This reverts commit ec1fd57. * Revert "add interface field to SwapOperation (#96)" This reverts commit eb37618. * remove deployed test contracts * fix fmt issue * update schema * since user_swap is mutatable, we can mutate in place * swap excess funds * update schema * lint change * fix test * add optional interface field to SwapOperation (#108) * return error for largest_route_index * [API-2792] Add hallswap adapter (#102) * feat: add hallswap adapter * chore: add readme * pr fixes * add optional interface field to SwapOperation * update hallswap adapter to implement updated interface * remove get_hallswap_routes_from_skip_routes --------- Co-authored-by: thal0x <[email protected]> --------- Co-authored-by: Jeremy Liu <[email protected]> Co-authored-by: Yonggiee <[email protected]>
Updates the Astroport adapter contract to support multiple routes.
The main change is to add an optional
offer_assettoAstroportPoolSwapto allow manually setting the swap amount of an operation. With this we can set theoffer_assetof a swap operation to theoffer_assetof the route if it is the first operation in the route.Most validation we will have to do regarding multiple routes appears to be done in the entry point contract, but correct me if I'm wrong.