This repository was archived by the owner on Jan 18, 2023. It is now read-only.
Exchange wrapper optimizations, minimize unnecessary transfers/approvals #218
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bweick
reviewed
Sep 27, 2018
| uint256[] memory componentTokensAmounts = new uint256[](_tradeCount); | ||
|
|
||
| uint256 scannedBytes = 0; | ||
| // Prase and execute the trade at the current offset via the KyberNetworkProxy, each kyber trade is 160 bytes |
bweick
approved these changes
Sep 27, 2018
Pull Request Test Coverage Report for Build 1934
💛 - Coveralls |
asoong
force-pushed
the
alex/exchange_wrapper_optimizations
branch
from
8ba7af9 to
dd085d8
Compare
felix2feng
reviewed
Sep 28, 2018
| _maker | ||
| // Transfer any unused or remainder maker token back to the issuance order user | ||
| uint remainderSourceToken = ERC20.balanceOf(_makerToken, this); | ||
| if (remainderSourceToken > 0) { |
Contributor
There was a problem hiding this comment.
componentTokensReceived during the trade was the makerToken, then an unintended amount would be returned to the _maker and the issuance order would fail.
Contributor
There was a problem hiding this comment.
adding comment to request changes
Contributor
Author
There was a problem hiding this comment.
why is that a problem? why wouldn't we want it to revert? this seems like an optimization to make at the assertion level
| ); | ||
| } | ||
|
|
||
| // Ensure the maker token is allowed to be transferred by Set TransferProxy |
Contributor
There was a problem hiding this comment.
Destination token vs maker token?
| { | ||
| address[] memory takerTokens = new address[](_orderCount); | ||
| uint256[] memory takerAmounts = new uint256[](_orderCount); | ||
| // Ensure the taker token is allowed to be transferred by ZeroEx Proxy |
Contributor
There was a problem hiding this comment.
I know what you mean, but this is a slightly confusing comment given that the _makerToken is passed in.
felix2feng
suggested changes
Sep 28, 2018
| _maker | ||
| // Transfer any unused or remainder maker token back to the issuance order user | ||
| uint remainderSourceToken = ERC20.balanceOf(_makerToken, this); | ||
| if (remainderSourceToken > 0) { |
Contributor
There was a problem hiding this comment.
adding comment to request changes
Contributor
|
Compile issues 😢 |
felix2feng
reviewed
Sep 28, 2018
| Buffer.concat(zeroExOrdersExchangeHeader), | ||
| Buffer.concat(zeroExOrderBuffer), | ||
| ])); | ||
| console.log(subjectExchangeOrdersData); |
asoong
force-pushed
the
alex/exchange_wrapper_optimizations
branch
from
c6b26de to
7452809
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Passing in additional parameters into the wrappers. This will require a redeploy of contracts for it to work on TestNet since the wrappers and parameters that core pass are no longer sufficient.