Conversation
|
CI is currently failing: |
adrake33
left a comment
adrake33
left a comment
There was a problem hiding this comment.
Looks good overall, but did have some questions/suggestions.
| event Mint(address indexed target, uint256 value); | ||
| event Burn(address indexed target, uint256 value); | ||
|
|
||
| mapping (address => uint) public wards; |
There was a problem hiding this comment.
Nit: recommend using uint256 instead of uint in this code since we use uint256 explicitly throughout the rest of the code.
| uint256 public supply; | ||
| mapping(address => mapping(address => uint256)) internal allowed; | ||
|
|
||
| uint8 constant public decimals = 18; |
There was a problem hiding this comment.
It looks like this variable is unused and can be removed.
| function getTypeName() public view returns (bytes32) { | ||
| return "Cash"; | ||
| function sub(uint x, uint y) internal pure returns (uint z) { | ||
| require((z = x - y) <= x, "math-sub-underflow"); |
There was a problem hiding this comment.
It seems like this and the other math functions below should be merged with SafeMathUint256.sol. Is there a reason to keep them separate?
| string constant public name = "Cash"; | ||
| string constant public symbol = "CASH"; | ||
|
|
||
| uint256 constant public DAI_ONE = 10 ** 27; |
There was a problem hiding this comment.
Why is 10 ^ 27 used? Isn't Dai precision 18 digits?
| balances[_target] = balances[_target].sub(_amount); | ||
| supply = supply.sub(_amount); | ||
|
|
||
| emit Burn(_target, _amount); |
There was a problem hiding this comment.
Don't we want to emit a Transfer event here since this is done in the mint function?
| @@ -1,9 +1,13 @@ | |||
| import * as path from 'path'; | |||
| import { join } from 'bluebird'; | |||
There was a problem hiding this comment.
This import looks like it can be removed.
| require((z = x - y) <= x); | ||
| } | ||
|
|
||
| function hope(address usr) public { can[msg.sender][usr] = 1; } |
There was a problem hiding this comment.
I'm assuming can, hope, nope, wish, & suck are Dai terminology? Recommend adding more comments to the Dai-related contracts to improve clarity.
| function migrateMarketOut(IUniverse _destinationUniverse) public returns (bool) { | ||
| IMarket _market = IMarket(msg.sender); | ||
| require(isContainerForMarket(_market)); | ||
| markets[msg.sender] = false; |
There was a problem hiding this comment.
msg.sender should probably be cast as an address here.
| markets[msg.sender] = false; | ||
| uint256 _cashBalance = marketBalance[address(msg.sender)]; | ||
| uint256 _marketOI = _market.getShareToken(0).totalSupply().mul(_market.getNumTicks()); | ||
| withdraw(address(this), _cashBalance, msg.sender); |
| return bit == usr || can[bit][usr] == 1; | ||
| } | ||
|
|
||
| function suck(address, address v, uint rad) public { |
There was a problem hiding this comment.
What is the purpose of the first param? It doesn't seem to be used.
| return mint(usr, wad); | ||
| } | ||
|
|
||
| function joinBurn(address usr, uint wad) public returns (bool) { |
There was a problem hiding this comment.
These two functions don't follow variable naming convention. Both in their word choice and in the _ prefix.
| event Mint(address indexed target, uint256 value); | ||
| event Burn(address indexed target, uint256 value); | ||
|
|
||
| mapping (address => uint) public wards; |
There was a problem hiding this comment.
Recommend against copying Maker's naming convention (wards).
3 participants
Integrates the DSR with our escrowed funds.
Funds are now escrowed on a Universe basis. If DSR is on funds are saved in the daiPot and if not they are simply kept as DAI (Cash).
The DSR can be toggled on or off when it crosses a threshold determined by the maximum movement in the DSR (TODO currently on getting that value from the actual MDAI contracts). Toggling awards the tx sender minted REP.
When funds are needed in Dai we exit only the amount required from the pot. Some rounding correction is required for that part of the code.
At any time anyone may sweep excess funds acquired through interest into the next dispute window using the
sweepInterestfunction on the Universe.These changes required some additional refactoring to get the Universe creation gas costs down and to get the Market contract byte size sown.