Alex/kyber network proxy wrapper #215
Conversation
Pull Request Test Coverage Report for Build 1859
💛 - Coveralls |
felix2feng
left a comment
felix2feng
left a comment
There was a problem hiding this comment.
This is really good! This is just my initial perusal, but I only have surface-level comments. I'll take another peek in a bit
| ); | ||
|
|
||
| // Update current bytes | ||
| scannedBytes = scannedBytes.add(160); |
There was a problem hiding this comment.
If the trade data size is fixed, can we make this a variable defined on the top of the file vs. a magic number?
There was a problem hiding this comment.
Normally, yes. In solidity, no since it’s another reference which adds gas
| // Transfer any unused issuance order maker token back to user | ||
| uint remainderSourceToken = ERC20.balanceOf(_trade.sourceToken, this); | ||
| if (remainderSourceToken > 0) { | ||
| ERC20.transfer( |
There was a problem hiding this comment.
OPTIMIZATION: Performing a transfer each time seems costly. Is there a way we can accumulate the remainderSourceToken and send it once after all Kyber trades have been executed?
| import { BigNumber } from 'bignumber.js'; | ||
| import { Address, Bytes, KyberTrade } from 'set-protocol-utils'; | ||
|
|
||
| import ChaiSetup from '../../../utils/chaiSetup'; |
There was a problem hiding this comment.
Use path aliases! @utils
| * | ||
| * TODO: Currently passes change back to the issuance order maker | ||
| * | ||
| * @param _maker Unused address of issuance order signer to conform to IExchangeWrapper |
There was a problem hiding this comment.
Comment is a little misleading because the address is used for transferring leftover tokens
felix2feng
left a comment
felix2feng
left a comment
There was a problem hiding this comment.
This is a great. Looking forward to the following integration tests.
Approve modulo getting the small knits in. The optimization piece can be added later as long as we add a card for it.
| * | ||
| * | Data | Location | | ||
| * |----------------------------|-------------------------------| | ||
| * | sourceToken | 0 | |
There was a problem hiding this comment.
Seems useful to add the data type as well.
There was a problem hiding this comment.
Just needs to show which parameter is at which index, the types are revealed in the struct at the top of this file
| ); | ||
| } | ||
|
|
||
| // Ensure the maker token is allowed to be transferred by Set TransferProxy |
There was a problem hiding this comment.
Knit: Space between Transfer and Proxy
There was a problem hiding this comment.
Leaving it in, meant to refer to the contract name
asoong
force-pushed
the
alex/kyber_network_proxy_wrapper
branch
from
a6cfec7 to
4c7334d
Compare
asoong
force-pushed
the
alex/kyber_network_proxy_wrapper
branch
from
4c7334d to
36b3e27
Compare
Things to do separately: