[order-utils] Add rate and sorting utilities #953
Conversation
| * Defaults to 0 | ||
| * @return The rate (takerAsset/makerAsset) of the order adjusted for fees | ||
| */ | ||
| getFeeAdjustedRateOfOrder(signedOrder: SignedOrder, feeRate: BigNumber = constants.ZERO_AMOUNT): BigNumber { |
There was a problem hiding this comment.
Does it have to be SignedOrder, or can it just be Order? Is the convention to always useSignedOrder?
There was a problem hiding this comment.
hmm yea good call, I had some issues with some other functions that took in orders and had to return orders, in those cases you actually have to use generics so that the type system knows a SignedOrder should be spit out if a SignedOrder is passed in, let me see if I can finagle it
| * @param signedFeeOrder An object that conforms to the signedOrder interface | ||
| * @return The rate (WETH/ZRX) of the fee order adjusted for fees | ||
| */ | ||
| getFeeAdjustedRateOfFeeOrder(signedFeeOrder: SignedOrder): BigNumber { |
There was a problem hiding this comment.
Should we assert that the pair is indeed WETH/ZRX? It could be another schema ?
There was a problem hiding this comment.
It's a bit hard to assert w/o having to pass in the network context, i think its fine, the calculations don't necessarily rely on the order being WETH/ZRX, the results just don't make sense if a different type of order is passed in
| const sortOrders = (signedOrders: SignedOrder[], rateCalculator: RateCalculator): SignedOrder[] => { | ||
| const copiedOrders = _.cloneDeep(signedOrders); | ||
| const feeOrderComparator = getOrderComparator(rateCalculator); | ||
| copiedOrders.sort(feeOrderComparator); |
There was a problem hiding this comment.
Javascript sort isn't stable (it can sort in a different order depending on browser/environment, see: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort). We should use _.sortBy here just in case.
There was a problem hiding this comment.
You actually can't sort via a comparator using _.sortBy
| ) => { | ||
| const firstOrderRate = rateCalculator(firstSignedOrder); | ||
| const secondOrderRate = rateCalculator(secondSignedOrder); | ||
| if (firstOrderRate.lt(secondOrderRate)) { |
There was a problem hiding this comment.
Nit: .comparedTo does this for you (http://mikemcl.github.io/bignumber.js/#cmp)
Description
When calculating rates for orders we need to take into account how much fees the taker needs to pay to fill the order. In the case of fee orders (ZRX/WETH) the fee is taken out of the makerAmount. In the case of non ZRX orders (ex. REP/WETH) a feeRate can be provided to estimate how much takerAsset is actually required to fill that order when performing fee abstraction
Testing instructions
Types of changes
Checklist:
[WIP]if it is a work in progress.[sol-cov] Fixed bug.