Implement ERC721MarginLoan #218

BrendanChou · 2018-04-20T18:32:02Z

No description provided.

AntonioJuliano · 2018-04-20T22:02:41Z

contracts/margin/external/ERC721MarginPosition.sol

@@ -54,7 +54,7 @@ contract ERC721MarginPosition is
        address margin
    )
        public
-        ERC721Token("dYdX Margin Positions", "DYDX-M")
+        ERC721Token("dYdX Margin Positions", "DYDX-MarginPosition")


This is too long for a symbol. I liked the old one better, if you want to know what it means, just look at the name

AntonioJuliano · 2018-04-20T22:10:22Z

contracts/margin/external/ERC721MarginLoan.sol

+    // Mapping from an address to other addresses that are approved to be margin-callers
+    mapping (address => mapping (address => bool)) public approvedCallers;
+
+    // Mapping from a positionId to the value of totalOwedTokensRepaidToLender for the position when


I don't think you should use a variable name to describe part of what this is

especially if totalOwedTokensRepaidToLender is not another defined state variable

AntonioJuliano · 2018-04-20T22:11:42Z

contracts/margin/external/ERC721MarginLoan.sol

+
+    // Mapping from a positionId to the address of the owedToken of that position. Needed because
+    // margin erases this information when the position is closed.
+    mapping (bytes32 => address) public owedTokenForPosition;


hmm... I hadn't thought about this. This is a bit unfortunate we have to have this

AntonioJuliano · 2018-04-20T22:22:16Z

contracts/margin/external/ERC721MarginLoan.sol

+        address margin
+    )
+        public
+        ERC721Token("dYdX Margin Loans", "DYDX-Loan")


I also feel DYDX-Loan is too long for a symbol

AntonioJuliano · 2018-04-20T22:22:47Z

contracts/margin/external/ERC721MarginLoan.sol

+    // ============ Token-Holder functions ============
+
+    /**
+     * Approves any address to margin-call any of the positions owned by the sender.


approves "an" address

AntonioJuliano · 2018-04-20T22:37:54Z

contracts/margin/external/ERC721MarginLoan.sol

+    )
+        external
+        onlyMargin
+        nonReentrant


same thing for nonReentrant

+1 leave in for this one

AntonioJuliano · 2018-04-20T22:38:03Z

contracts/margin/external/ERC721MarginLoan.sol

+    )
+        external
+        onlyMargin
+        nonReentrant


AntonioJuliano · 2018-04-20T22:39:00Z

contracts/margin/external/ERC721MarginLoan.sol

+    )
+        external
+        onlyMargin
+        nonReentrant


AntonioJuliano · 2018-04-20T22:42:35Z

contracts/margin/external/ERC721MarginLoan.sol

+
+        address owedToken = owedTokenForPosition[positionId];
+        address owner = ownerOf(uint256(positionId));
+        TokenInteract.transfer(owedToken, owner, tokensToSend);


I think we should add an assert(TokenInteract.balanceOf(owedToken , address(this)) > tokensToSend

AntonioJuliano · 2018-04-20T22:44:28Z

contracts/margin/external/ERC721MarginLoan.sol

+        address owedToken = owedTokenForPosition[positionId];
+        address owner = ownerOf(uint256(positionId));
+        TokenInteract.transfer(owedToken, owner, tokensToSend);
+


I don't know if you want to do it in this function or another helper, but we should burn the token if the position is closed. Also delete owedTokensRepaidSinceLastWithdraw and owedTokenForPosition as commented above

also don't do owedTokensRepaidSinceLastWithdraw[positionId] = totalRepaid; on line 303 if we're just about to delete it anyways

BrendanChou · 2018-04-20T23:59:33Z

I can't override non-view interface function with a view implementation function without the compiler complaining. As also complained about here: ethereum/solidity#3412

It's dumb because it should be able to do that as the visibility doesn't affect the ABI

AntonioJuliano · 2018-04-21T00:13:11Z

contracts/margin/external/ERC20/ERC20Short.sol

@@ -27,7 +27,7 @@ contract ERC20Short is ERC20Position {
            margin,
            initialTokenHolder,
            trustedRecipients,
-            "DYDX-S"
+            "D/SS"


just one "S" like the nouns are "leveraged long" and "short"

AntonioJuliano · 2018-04-21T00:14:03Z

contracts/margin/external/ERC20/ERC20Long.sol

@@ -62,6 +62,6 @@ contract ERC20Long is ERC20Position {
        pure
        returns (bytes)
    {
-        return "dYdX Tokenized Long";
+        return "dYdX ERC20 Leveraged-Long";


"dYdX Leveraged-Long Token"

AntonioJuliano · 2018-04-21T00:14:13Z

contracts/margin/external/ERC20/ERC20Short.sol

@@ -62,6 +62,6 @@ contract ERC20Short is ERC20Position {
        pure
        returns (bytes)
    {
-        return "dYdX Tokenized Short";
+        return "dYdX ERC20 Short-Sell";


"dYdX Short Token"

AntonioJuliano · 2018-04-21T00:14:35Z

contracts/margin/external/ERC20/ERC20Short.sol

@@ -62,6 +62,6 @@ contract ERC20Short is ERC20Position {
        pure
        returns (bytes)
    {
-        return "dYdX Tokenized Short";
+        return "dYdX Short-Sell";


"dYdX Short Token"

AntonioJuliano · 2018-04-21T00:14:47Z

contracts/margin/external/ERC20/ERC20Long.sol

@@ -62,6 +62,6 @@ contract ERC20Long is ERC20Position {
        pure
        returns (bytes)
    {
-        return "dYdX Tokenized Long";
+        return "dYdX Leveraged-Long";


"dYdX Leveraged Long Token"

AntonioJuliano · 2018-04-21T00:15:09Z

contracts/margin/external/ERC20/ERC20Short.sol

@@ -27,7 +27,7 @@ contract ERC20Short is ERC20Position {
            margin,
            initialTokenHolder,
            trustedRecipients,
-            "DYDX-S"
+            "d/SS"


just "d/S"

the nouns are "Short" and "Leveraged Long"

AntonioJuliano · 2018-04-21T00:18:29Z

contracts/margin/external/ERC721MarginLoan.sol

+    )
+        external
+        onlyMargin
+        nonReentrant


+1 leave in for this one

AntonioJuliano · 2018-04-21T00:20:19Z

contracts/margin/external/ERC721MarginLoan.sol

+        address owner = ownerOf(uint256(positionId));
+        require(owner != address(0));
+
+        address owedToken = owedTokenAddress[positionId];


maybe also assert(owedToken != address(0) I know it shouldn't be, but just to be sure

I think it's fine to not have that assert. It definitely shouldn't be, but if there were a problem that big, it should be asserted in the base protocol instead. Secondly it should revert on TokenInteract.transfer() anyway if the address is zero

implement ERC721MarginLoan

dc858d8

BrendanChou requested a review from AntonioJuliano April 20, 2018 18:32

BrendanChou force-pushed the bc_erc721loan branch from 2c4f44a to dc858d8 Compare April 20, 2018 18:32

BrendanChou mentioned this pull request Apr 20, 2018

Tracking Solidity Changes #219

Closed

AntonioJuliano suggested changes Apr 20, 2018

View reviewed changes

fix for tests

3aa7b83

BrendanChou force-pushed the bc_erc721loan branch from 2eb61ae to 3aa7b83 Compare April 21, 2018 00:00

BrendanChou changed the title ~~implement ERC721MarginLoan~~ Implement ERC721MarginLoan Apr 21, 2018

fix names

cb10f80

AntonioJuliano reviewed Apr 21, 2018

View reviewed changes

name fixes

8e7274d

AntonioJuliano approved these changes Apr 21, 2018

View reviewed changes

fix tests for names

52cc4bc

BrendanChou merged commit cd224a2 into master Apr 21, 2018

BrendanChou deleted the bc_erc721loan branch April 21, 2018 01:29

Implement ERC721MarginLoan #218

Implement ERC721MarginLoan #218

Conversation

BrendanChou commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrendanChou commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment