Add dxswapSwapRelayer #39

karczuRF · 2022-02-07T09:25:18Z

No description provided.

kowalski · 2022-02-07T09:51:46Z

contracts/examples/DXswapSwapRelayer.sol

+            (amountInTokenA > 0 && amountInTokenB == 0) || (amountInTokenA == 0 && amountInTokenB > 0),
+            'DXswapRelayer: INVALID_TOKEN_AMOUNT'
+        );
+        require(priceTolerance <= PARTS_PER_MILLION, 'DXswapRelayer: INVALID_TOLERANCE');


to consult with John: it may require that priceTolerance is at least 0.3% as this the fee uniswap charges.
However, it dxswap has different fee model, than perhaps it's better to leave as is

luzzif · 2022-05-11T08:58:27Z

contracts/examples/DXswapSwapRelayer.sol

+        } else {
+            require(IERC20(tokenA).balanceOf(address(this)) >= amountInTokenA, 'DXswapRelayer: INSUFFICIENT_TOKEN_A');
+        }
+        require(IERC20(tokenB).balanceOf(address(this)) >= amountInTokenB, 'DXswapRelayer: INSUFFICIENT_TOKEN_B');


If only one of the 2 tokens can have an amount > 0 (as per one of the requires above), we can shave off some gas by only checking the balance in the non-zero token amount (the other will always be 0).

luzzif · 2022-05-11T09:00:48Z

contracts/examples/DXswapSwapRelayer.sol

+        orders[orderIndex] = Order({
+            tokenIn: amountInTokenA > 0 ? tokenA : tokenB,
+            tokenOut: amountInTokenA > 0 ? tokenB : tokenA,
+            amountIn: amountInTokenA > 0 ? amountInTokenA : amountInTokenB,


These checks can all be collapsed into one, saving some gas.

(address tokenIn, address tokenOut, uint256 amountIn) = amountInTokenA > 0 ? (tokenA, tokenB, amountInTokenA) : (tokenB, tokenA, amountInTokenB);

luzzif · 2022-05-11T09:12:23Z

contracts/examples/DXswapSwapRelayer.sol

+        Order storage order = orders[orderIndex];
+        require(block.timestamp <= order.deadline, 'DXswapRelayer: DEADLINE_REACHED');
+        require(!oracleCreator.isOracleFinalized(order.oracleId), 'DXswapRelayer: OBSERVATION_ENDED');
+        uint256 amountBounty = GAS_ORACLE_UPDATE.mul(tx.gasprice).add(BOUNTY);


I'd maybe cap the gas price to BASEFEE + a certain maximum amount as a miner tip (let's say 5 or 6 gwei?). I can see grief attacks where someone submits a tx with a high gas price, draining the contract of all ETH. At that point oracles become unworkable because there are no funds to pay bounties (it can also be bad if we want to perform an ETH trade because the ETH can be drained by a malicious user). The user would only gain the bounty, but it would be pretty damn bad for the DAO (although very good for miners).

Nice catch! Agree that would be good to use basefee, but to do that we need to upgrade Solidity, because block.basefee was introduced with 0.8.7. I suggest upgrading to the newest version anyways.
I'm not sure about a miner tip, what do you think @jpkcambridge?

luzzif · 2022-05-11T09:17:34Z

contracts/examples/DXswapSwapRelayer.sol

+        address[] memory path = new address[](2);
+        address swapRouter;
+
+        if (_tokenIn == address(0)) {


Preeeetty sure we can leave this to the router and do a check instead. If tokenIn is ETH, we can use swapExactTokensForETH, swapExactTokensForTokens instead. Just a tip though, not a hard requirement.

luzzif · 2022-05-11T09:18:14Z

contracts/examples/DXswapSwapRelayer.sol

+        }
+
+        path[0] = _tokenIn;
+        path[1] = _tokenOut;


Do we intend to always do direct swaps without hops? If not, we could manually specify the path when creating the order. It gives us more flexibility.

As I was informed we want to do only direct swaps and looking for better paths is out of scope of this contract.

luzzif · 2022-05-11T09:18:40Z

contracts/examples/DXswapSwapRelayer.sol

+        }
+
+        TransferHelper.safeApprove(_tokenIn, swapRouter, _amountIn);
+        TransferHelper.safeApprove(_tokenOut, swapRouter, _minAmountOut);


I don't think this approval is necessary? The router only needs to take the tokenIn, so only that one should be allowed.

luzzif · 2022-05-11T09:19:47Z

contracts/examples/DXswapSwapRelayer.sol

+            address(this),
+            block.timestamp
+        );
+        TransferHelper.safeApprove(_tokenIn, swapRouter, 0);


This shouldn't be necessary. It depends on the token really, but 90% of them automatically subtract the allowance when "used up". Plus, we're allowing pretty damn safe contracts (Uniswap-like routers only).

You're right. That was left just "to be sure" about allowance, but can be removed as you explained.

luzzif · 2022-05-11T09:24:21Z

contracts/examples/DXswapSwapRelayer.sol

+        (uint256 reserveA, uint256 reserveB, ) = IDXswapPair(order.oraclePair).getReserves();
+        require(reserveA >= order.minReserveA && reserveB >= order.minReserveB, 'DXswapRelayer: RESERVE_TOO_LOW');
+
+        oracleCreator.update(order.oracleId);


I'm wondering what happens here if the reserves have changed and in turn oracle parameters are potentially not safe anymore? I.e. someone removes liquidity without it being brought below minimum levels, but changing the oracle's window time as calculated in _consultOracleParameters. I would maybe think about consulting the oracle parameters once again and eventually update them for the next read (and potentially reset the update counter)?

I think it would be ok indeed to consult the oracle and check whether window time has changed or not. Let's keep in minds that the same solution is used for DXswapRelayer contract, so if we consider the oracle might be not safe we will have to change also that one, @jpkcambridge?

luzzif

Added a couple comments, but didn't finish the review. Will resume later.

luzzif · 2022-08-31T09:30:21Z

contracts/examples/DXswapSwapRelayer.sol

@@ -0,0 +1,296 @@
+pragma solidity =0.6.6;


Upgrade Solidity to v0.8.16 and remove the ABI encorder v2 pragma.

luzzif · 2022-08-31T09:31:04Z

contracts/examples/DXswapSwapRelayer.sol

+import './../libraries/DXswapLibrary.sol';
+
+contract DXswapSwapRelayer {
+    using SafeMath for uint256;


When the Solidity version is upgraded, we can remove SafeMath usages

luzzif · 2022-08-31T09:31:22Z

contracts/examples/DXswapSwapRelayer.sol

+contract DXswapSwapRelayer {
+    using SafeMath for uint256;
+
+    event NewOrder(uint256 indexed _orderIndex);


I'd remove the underscore here, in all event param names

luzzif · 2022-08-31T09:33:30Z

contracts/examples/DXswapSwapRelayer.sol

+        bool executed;
+    }
+
+    uint256 public immutable GAS_ORACLE_UPDATE = 168364;


I'm not sure about this. The underlying protocol can change and call gas costs might be different in the future... using gasleft() to calculate the specific amount of gas used to perform the oracle update is a better way of doing things imho.

luzzif · 2022-08-31T09:34:20Z

contracts/examples/DXswapSwapRelayer.sol

+
+    uint256 public immutable GAS_ORACLE_UPDATE = 168364;
+    uint256 public immutable PARTS_PER_MILLION = 1000000;
+    uint256 public immutable BOUNTY = 0.01 ether; // To be decided


Calculating the bounty dinamically would probably be best. Let's say as a percentage of the fee used to update the oracle.

luzzif · 2022-08-31T09:41:11Z

contracts/examples/DXswapSwapRelayer.sol

+    uint256 public immutable PARTS_PER_MILLION = 1000000;
+    uint256 public immutable BOUNTY = 0.01 ether; // To be decided
+    uint8 public immutable PROVISION = 1;
+    uint8 public immutable REMOVAL = 2;


Pretty much all of the above state variables are constants, so it's heavily suggested to use constant instead of immutable. immutable variables are evaluated at runtime and are reserved 32 bytes even for potentially smaller values. This increases deployment costs for no particular reason.

luzzif · 2022-08-31T09:42:01Z

contracts/examples/DXswapSwapRelayer.sol

+    address public immutable uniswapRouter;
+    address public immutable WETH;
+
+    OracleCreator oracleCreator;


Visibility specifier missing. Should this be internal?

luzzif · 2022-08-31T09:42:10Z

contracts/examples/DXswapSwapRelayer.sol

+
+    OracleCreator oracleCreator;
+    uint256 public orderCount;
+    mapping(uint256 => Order) orders;


Same as above.

luzzif · 2022-08-31T09:43:27Z

contracts/examples/DXswapSwapRelayer.sol

+        address _WETH,
+        OracleCreator _oracleCreator
+    ) public {
+        owner = _owner;


Are we ok with no validation?

luzzif · 2022-08-31T09:47:18Z

contracts/examples/DXswapSwapRelayer.sol

+        }
+        require(IERC20(tokenB).balanceOf(address(this)) >= amountInTokenB, 'DXswapRelayer: INSUFFICIENT_TOKEN_B');
+
+        address pair = _pair(tokenA, tokenB, factory);


We could actually call the factory to get the pair address based on the 2 tokens that are into it. This will also let us know if the pair exists or not. If it doesn't we should abort here.

chore: fast-forward `develop` to `master`

chore: v0.3.23 and fast-forward

* migrate to hardhat * fix solc errors * refactor: change init code pair hash * test: update gas usage checks * cleanup & upgrade packages * refactor after cr: add dependency-compiler & control gas report * refactor: remove flatten script & add deploy

karczuRF added 2 commits February 4, 2022 15:07

add swap relayer contract with tests

21f7717

cleanup & renaming

62a5d76

kowalski reviewed Feb 7, 2022

View reviewed changes

karczuRF requested review from jpkcambridge and luzzif February 7, 2022 10:05

luzzif suggested changes May 11, 2022

View reviewed changes

karczuRF added 5 commits June 28, 2022 15:57

migrate to hardhat

4d046f0

fix solc errors

ef70f77

refactor: change init code pair hash

d6e4256

test: update gas usage checks

79c0f6c

feat: add Trade contract and tests

a220bb7

luzzif suggested changes Aug 31, 2022

View reviewed changes

Federico Luzzi and others added 9 commits September 1, 2022 12:38

Merge pull request #46 from levelkdev/develop

4f8d2ca

chore: fast-forward `develop` to `master`

Merge pull request #47 from levelkdev/develop

18fcd32

chore: v0.3.23 and fast-forward

Migrate to hardhat (#42)

cb28c9a

* migrate to hardhat * fix solc errors * refactor: change init code pair hash * test: update gas usage checks * cleanup & upgrade packages * refactor after cr: add dependency-compiler & control gas report * refactor: remove flatten script & add deploy

Merge branch 'master' into feat/DXswapTradeRelayer

555cf8a

fix: compilation errors

e0c18c4

tests: relayer

b676208

Merge branch 'feat/DXswapTradeRelayer' into feat/dxswapSwapRelayer

c130ba4

fix errors after merge

f4dc987

cleanup & renaming

191539a

Add dxswapSwapRelayer #39

Are you sure you want to change the base?

Add dxswapSwapRelayer #39

Uh oh!

Conversation

karczuRF commented Feb 7, 2022

Uh oh!

kowalski Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luzzif May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luzzif left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kowalski Feb 7, 2022 •

edited

Loading

luzzif May 11, 2022 •

edited

Loading