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

[asset-swapper] clip native fill data #2565

Merged
merged 3 commits into from
Apr 24, 2020

Conversation

dekz
Copy link
Member

@dekz dekz commented Apr 24, 2020

Description

There is a scenario where perfectly sized Native orders were penalized more aggressively than they should have been.

The previous behaviour was as follows:

penaltyMakerUnits = 10
input = 2000

SMALL_ORDER
makerAmount = 100
takerAmount = 200

adjustedOutput = 90
adjustedRate = 0.45


BIG_ORDER
makerAmount = 1000
takerAmount = 2000

adjustedOutput = 990
adjustedRate = 0.495

In the scenario where the user wishes to sell 2000 the SMALL_ORDER should be penalized over BIG_ORDER and BIG_ORDER should be favoured.

In the scenario where the user wishes to sell 200 both SMALL_ORDER and BIG_ORDER are sufficient and either is acceptable to fill.

penaltyMakerUnits = 10
input = 200

SMALL_ORDER
makerAmount = 100
takerAmount = 200

adjustedOutput = (min(takerAmount, input) / takerAmount) * makerAmount - penaltyMakerUnits = 90
adjustedRate = 0.45


BIG_ORDER
makerAmount = 1000
takerAmount = 2000

adjustedOutput = (min(takerAmount, input) / takerAmount) * makerAmount - penaltyMakerUnits = 90
adjustedRate = 0.45

We effectively bring large orders in line with small orders when they completely cover the requested amount.

Issue

The following behaviour caused perfectly sized orders to disappear:

  1. Perfectly sized orders were penalized more harshly than large orders
  2. Native orders are sorted by adjustedRate, then a subset of native orders are chosen (enough to fill the requested amount).
  3. Since better rate orders were dropped in 2), they would no longer be present when finding a path.

@coveralls
Copy link

coveralls commented Apr 24, 2020

Coverage Status

Coverage increased (+0.08%) to 79.563% when pulling bf626a0 on fix/asset-swapper-native-adjusted-output into 501070b on development.

@dekz dekz force-pushed the fix/asset-swapper-native-adjusted-output branch from f9ce57d to bf626a0 Compare April 24, 2020 06:13
Copy link
Contributor

@PirosB3 PirosB3 left a comment

Choose a reason for hiding this comment

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

I really appreciate your swift PR

// targetInput can be less than the order size
// whilst the penalty is constant, it affects the adjusted output
// only up until the target has been exhausted.
// A large order and an order at the exact target should be penalized
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be penalized the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

they should be penalized the same, since the cost is constant and the amount is constant. Where having a bigger order size is no advantage.

// the same.
const clippedInput = BigNumber.min(targetInput, input);
// scale the clipped output inline with the input
const clippedOutput = clippedInput.dividedBy(input).times(output);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we be concerned about rounding errors with this operation? of does BigNumber prevent rounding issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

These remain very precise, but I could go either way in having these as whole base units which would lose precision but not necessarily have too much of an impact.

@@ -96,7 +96,7 @@ function clipFillAdjustedOutput(fill: Fill, remainingInput: BigNumber): BigNumbe
return fill.adjustedOutput;
}
const penalty = fill.adjustedOutput.minus(fill.output);
return fill.output.times(remainingInput.div(fill.input)).plus(penalty);
return remainingInput.times(fill.rate).plus(penalty);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this function modified? based on my understanding, it should not impact nativeOrdersToPath which is the core function that you changed

Copy link
Member Author

Choose a reason for hiding this comment

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

I find it just easier to reason about. input * rate = output, versus scaling the previous output by the proportion of remaining vs total.

@dekz dekz merged commit b34edcb into development Apr 24, 2020
@dekz dekz deleted the fix/asset-swapper-native-adjusted-output branch April 24, 2020 22:44
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