Paraswap Integration for TokenExchangerV2 #121
Conversation
olivdb
force-pushed
the
paraswap
branch
3 times, most recently
from
2e9a89f
to
0112dad
Compare
contracts/modules/TokenExchanger.sol
Outdated
| @@ -33,139 +35,238 @@ contract TokenExchanger is OnlyOwnerModule { | |||
|
|
|||
| // Mock token address for ETH | |||
| address constant internal ETH_TOKEN_ADDRESS = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE; | |||
| // Signatures of Paraswap's trade methods | |||
| bytes4 constant internal MULTISWAP = 0xcbd1603e; | |||
There was a problem hiding this comment.
Maybe useful to have the computation of the signatures in the comment (bytes4(keccak256(.....)) so we know exactly what is called.
contracts/modules/TokenExchanger.sol
Outdated
| */ | ||
| function trade( | ||
| function multiSwap( |
There was a problem hiding this comment.
Why not name this method sell since the other one is called buy? It will probably avoid some confusion.
There was a problem hiding this comment.
Yes, I hesitated but in the end decided to use naming consistent with Paraswap's contract (which uses buy and multiSwap). Open to both.
There was a problem hiding this comment.
I would go for buy and sell irrespective of their names in Paraswap. In the end it will make it clearer for us what each method does (i.e. simpler for clients, support, etc).
| // TODO: Use a "safe approve" logic similar to the one implemented below in other modules | ||
| if (_token != ETH_TOKEN_ADDRESS) { | ||
| _existingAllowance = ERC20(_token).allowance(_wallet, paraswapProxy); | ||
| if (_existingAllowance < uint256(-1)) { |
There was a problem hiding this comment.
Why is this condition necessary?
There was a problem hiding this comment.
If the user has granted an unlimited allowance to Paraswap (via WalletConnect), we should not change the allowance. That's not only to save gas in this particular transaction, but also because many tokens (DAI, USDT, etc) optimise transferFrom when the allowance is uint(-1) so as to save gas and I think we should not break this optimisation when the user opts for it.
There was a problem hiding this comment.
Also, without this condition, SafeMath.add(_existingAllowance, _amount) would revert when _existingAllowance == uint(-1).
| paraswapProxy = IAugustusSwapper(_paraswap).getTokenTransferProxy(); | ||
| referrer = _referrer; | ||
|
|
||
| for (uint i = 0; i < _authorisedExchanges.length; i++) { |
There was a problem hiding this comment.
I think using a black list instead of a white list would give us more flexibility. When Paraswap adds a new DEX we will want to benefit from it without having to redeploy a new module. That's one of the main benefit of using an aggregator. We should only redeploy a new module when the added DEX does not meet our security requirements.
There was a problem hiding this comment.
For the record we have decided to keep the (fixed) whitelist as it is the only option that does not place any security/trust assumption on both Argent and Paraswap.
contracts/modules/TokenExchanger.sol
Outdated
| uint256 _minConversionRate | ||
| uint256 _maxSrcAmount, | ||
| uint256 _destAmount, | ||
| uint256 _expectedDestAmount, |
There was a problem hiding this comment.
Is _expectedDestAmount returned by the API and is it different than _destAmount in a buy? Maybe _destAmount is not needed?
There was a problem hiding this comment.
This is a typo: it should be _expectedSrcAmount. It's given by the API and is only used by Paraswap's Bought event.
…rts caused by hardcoded one
| uint256 _minConversionRate | ||
| uint256 _maxSrcAmount, | ||
| uint256 _destAmount, | ||
| uint256 _expectedSrcAmount, |
There was a problem hiding this comment.
This is a detail but I would expect the _expectedSrcAmount to be right after the _maxSrcAmount in the parameters of the method so you have the expected and the worst case amounts next to each other. I think it makes the inputs more readable.
| paraswapProxy = IAugustusSwapper(_paraswap).getTokenTransferProxy(); | ||
| referrer = _referrer; | ||
|
|
||
| for (uint i = 0; i < _authorisedExchanges.length; i++) { |
There was a problem hiding this comment.
For the record we have decided to keep the (fixed) whitelist as it is the only option that does not place any security/trust assumption on both Argent and Paraswap.
This PR introduces a new version of the TokenExchanger module that relies on Paraswap instead of Kyber. This brings the following benefits: