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

AssetSwapper: Fix quote optimizer fee bug #2526

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

dorothy-zbornak
Copy link
Contributor

@dorothy-zbornak dorothy-zbornak commented Mar 20, 2020

Description

When the optimizer was building a quote, it wasn't clipping the input/output amounts of a tail-end fill by the remaining input amount. So a large enough fill (usually native orders) could be attributed a higher return rate than it really should. This fix also apparently improves quoted and realized prices slightly, even though I wasn't trying to.

Also updated the ethgasstation poll rate to ~half the block time, because gas price is a lot more mercurial during the end of the world.

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.

@dorothy-zbornak dorothy-zbornak force-pushed the fix/asset-swapper/optimizer-fee-bugs branch from 6bba84b to 0fdd318 Compare March 20, 2020 19:59
@dorothy-zbornak dorothy-zbornak marked this pull request as ready for review March 20, 2020 20:00
@coveralls
Copy link

coveralls commented Mar 20, 2020

Coverage Status

Coverage decreased (-0.08%) to 79.462% when pulling 0fdd318 on fix/asset-swapper/optimizer-fee-bugs into 7a68260 on development.

Comment on lines +98 to +99
const penalty = fill.adjustedOutput.minus(fill.output);
return fill.output.times(remainingInput.div(fill.input)).plus(penalty);
Copy link
Member

Choose a reason for hiding this comment

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

This is negative right? If so it would be clearer if it was:

Suggested change
const penalty = fill.adjustedOutput.minus(fill.output);
return fill.output.times(remainingInput.div(fill.input)).plus(penalty);
const penalty = fill.output.minus(fill.adjustedOutput);
return fill.output.times(remainingInput.div(fill.input)).minus(penalty);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, it can be positive for buys.

@dorothy-zbornak dorothy-zbornak merged commit 99b0a95 into development Mar 24, 2020
@dorothy-zbornak dorothy-zbornak deleted the fix/asset-swapper/optimizer-fee-bugs branch March 24, 2020 13:56
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.

4 participants