Brian/full scenario analysis #127
Conversation
Pull Request Test Coverage Report for Build 776
💛 - Coveralls |
| ) | ||
| external | ||
| onlyAuthorized | ||
| isNonZero(_quantity) |
There was a problem hiding this comment.
Why are these being removed?
There was a problem hiding this comment.
Because to we might batch deposit 0 tokens from one exchange but we still are passing on an array that contains the 0 so needed to remove that. Unless we want to do some array pruning somewhere.
There was a problem hiding this comment.
Can you give me an example? I think this is suboptimal for gas, prefer some kind of efficient pruning
| uint[] _quantities | ||
| ) | ||
| internal | ||
| isValidBatchTransaction(_tokenAddresses, _quantities) |
There was a problem hiding this comment.
As I go back...I agree that there's no reason to remove those modifiers. It was breaking before and I thought that was the case...I wonder if something was changed in a rebase cause it definitely shouldn't change anything.
| state.orderFills[_order.orderHash] = state.orderFills[_order.orderHash].add(_fillQuantity); | ||
| } | ||
|
|
||
| function getPartialAmount( |
There was a problem hiding this comment.
Can this get moved into a lib
There was a problem hiding this comment.
Sure could was looking for a lib that made sense in our set up so just left it as is. But could move into OrderLibrary I suppose.
|
Couple of questions, i'd like to see another parameter in each scenario that describes the outcome and/or flows that it goes through |
| settleOrder(order, _fillQuantity, _orderData); | ||
|
|
||
| //Issue Set | ||
| //Issue Settle |
There was a problem hiding this comment.
need a space after //, i think the precommit linting should catch these things now though?
…ded function in contract to revert txn if rounding error too large.
…st for getPartialAmount.
…quiredComponentAmounts and Addresses arrays are set up.
…nding error test to get full coverage.
bweick
force-pushed
the
brian/full_scenario_analysis
branch
from
2bf01a3 to
4c18fe9
Compare
4 participants
No description provided.