Colin/liquid exchange #5
Conversation
| @@ -1,12 +1,25 @@ | |||
| pragma solidity ^0.8.10; | |||
| import "../../openzeppelin-contracts/contracts/token/ERC20/ERC20.sol"; | |||
There was a problem hiding this comment.
forge install transmissions11/solmate
Better module!
| import "../../portfolio/lib/solmate/src/utils/FixedPointMathLib.sol"; // This import goes directly to the contract | ||
| import "../../openzeppelin-contracts/contracts/token/ERC20/ERC20.sol"; |
There was a problem hiding this comment.
In foundry.toml, you can make a 'remappings' entry that will make these imports easier:
remappings = [
'solmate/=lib/portfolio/lib/solmate/src'
]
Then in contract:
import "solmate/utils/FixedPointMath.sol";
| uint256 public constant WAD = 10**18; | ||
|
|
||
| // Each LiquidExchange contract will be deployed with a pair of token addresses and an initial price | ||
| constructor(address _arbiter_token_x, address _arbiter_token_y, uint256 _price) { |
There was a problem hiding this comment.
Styling nit:
Solidity is usually camel case, but I don't mind.
I DO mind leading underscores. Leading underscores are reserved for internal variables and functions. Trailing underscores are good for arguments.
| arbiter_token_x = ERC20(_arbiter_token_x); | ||
| arbiter_token_x_address = _arbiter_token_x; |
There was a problem hiding this comment.
Don't need two! Just need the address, and you can cast between the types like this:
address token = 0x01
ERC20 token0 = ERC20(token);
address token1 = address(token0);
| function getPrice() public view returns (uint256) { | ||
| return price; | ||
| } |
There was a problem hiding this comment.
While getPrice is fine, since price is a public variable, it creates a getter price() already
| // Set allowances | ||
| // arbiter_token_x.increaseAllowance(msg.sender, _amount_in); | ||
| // arbiter_token_y.increaseAllowance(admin, amount_out); //might not be necessary if we increase the allowance for the manager outside of this |
There was a problem hiding this comment.
So:
You call this contract, which means its asking for YOUR allowance for THIS contract to spend tokens. This means you have to send a transaction to the token to approve this contract as a spender.
| // arbiter_token_y.increaseAllowance(admin, amount_out); //might not be necessary if we increase the allowance for the manager outside of this | ||
| // Transfer amount in and amount out | ||
| arbiter_token_x.transferFrom(msg.sender, admin, _amount_in); | ||
| arbiter_token_y.transferFrom(admin, msg.sender, amount_out); |
There was a problem hiding this comment.
The from argument in transferFrom should ALWAYS be msg.sender.
You should use transfer here, and have this contract hold the tokens.
| arbiter_token_y.transferFrom(msg.sender, admin, _amount_in); | ||
| arbiter_token_x.transferFrom(admin, msg.sender, amount_out); | ||
| } else { | ||
| revert("Invalid token"); |
There was a problem hiding this comment.
Use a custom error:
error InvalidToken();
revert InvalidToken();
There was a problem hiding this comment.
Following up on this? @Autoparallel, I am at a spot where i want to decode the events on this market.
Linking to relevant arbiter PR