Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

fix: [asset-swapper] MultiHop edge cases #2730

Merged
merged 2 commits into from
Oct 15, 2020
Merged

Conversation

dekz
Copy link
Member

@dekz dekz commented Oct 15, 2020

Description

Using includedSources/excludedSources can introduce a few edge cases.

  • It modified feeSources which made it difficult to sometimes find an maker/ETH or taker/ETH price, so cost based routing could get weird.
  • It can result in MultiHop not having anything to MultiHop to (e.g DAI/USDC with Curve only, theres no ability to ETH hop)
  • There are cases where firstHopSource is not present in the FillData but it is required. No valid sources or reverts (didn't dig deep enough to confirm it was a revert)
  • If there was no optimal path then we would throw, there's the possibility that there is no optimal path, but there is a MultiHop path available.

Sims on some pairs which use MultiHop often

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

Copy link
Contributor

@moodlezoup moodlezoup left a comment

Choose a reason for hiding this comment

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

man this shit is hard, thanks for fixing 😅

@dekz dekz merged commit fb21ca5 into development Oct 15, 2020
@dekz dekz deleted the fix/asset-swapper/multi-hop branch October 15, 2020 07:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants