Comments and cleanup

packages/contracts/src/utils/asset_wrapper.ts

@@ -12,6 +12,10 @@ interface ProxyIdToAssetWrappers {
     [proxyId: number]: AbstractAssetWrapper;
 }
 
+/**
+ * This class abstracts away the differences between ERC20 and ERC721 tokens so that
+ * the logic that uses it does not need to care what standard a token belongs to.
+ */
 export class AssetWrapper {
     private _proxyIdToAssetWrappers: ProxyIdToAssetWrappers;
     constructor(assetWrappers: AbstractAssetWrapper[]) {
@@ -29,7 +33,6 @@ export class AssetWrapper {
                 const balance = await assetWrapper.getBalanceAsync(userAddress, assetData);
                 return balance;
             }
-
             case constants.ERC721_PROXY_ID: {
                 const assetWrapper = this._proxyIdToAssetWrappers[proxyId] as ERC721Wrapper;
                 const assetProxyData = assetProxyUtils.decodeERC721AssetData(assetData);
@@ -41,7 +44,6 @@ export class AssetWrapper {
                 const balance = isOwner ? new BigNumber(1) : new BigNumber(0);
                 return balance;
             }
-
             default:
                 throw errorUtils.spawnSwitchErr('proxyId', proxyId);
         }
@@ -54,7 +56,6 @@ export class AssetWrapper {
                 await assetWrapper.setBalanceAsync(userAddress, assetData, desiredBalance);
                 return;
             }
-
             case constants.ERC721_PROXY_ID: {
                 if (!desiredBalance.eq(0) && !desiredBalance.eq(1)) {
                     throw new Error(`Balance for ERC721 token can only be set to 0 or 1. Got: ${desiredBalance}`);
@@ -82,11 +83,6 @@ export class AssetWrapper {
                         tokenOwner,
                         userAddress,
                     );
-                } else if (
-                    (userAddress !== tokenOwner && desiredBalance.eq(0)) ||
-                    (tokenOwner === userAddress && desiredBalance.eq(1))
-                ) {
-                    return; // noop
                 } else if (tokenOwner === userAddress && desiredBalance.eq(0)) {
                     // Burn token
                     await erc721Wrapper.burnAsync(assetProxyData.tokenAddress, assetProxyData.tokenId, userAddress);
@@ -94,7 +90,6 @@ export class AssetWrapper {
                 }
                 break;
             }
-
             default:
                 throw errorUtils.spawnSwitchErr('proxyId', proxyId);
         }
@@ -107,7 +102,6 @@ export class AssetWrapper {
                 const allowance = await assetWrapper.getProxyAllowanceAsync(userAddress, assetData);
                 return allowance;
             }
-
             case constants.ERC721_PROXY_ID: {
                 const assetWrapper = this._proxyIdToAssetWrappers[proxyId] as ERC721Wrapper;
                 const erc721ProxyData = assetProxyUtils.decodeERC721AssetData(assetData);
@@ -123,7 +117,6 @@ export class AssetWrapper {
                 const allowance = isProxyApproved || isProxyApprovedForAllAsync ? new BigNumber(1) : new BigNumber(0);
                 return allowance;
             }
-
             default:
                 throw errorUtils.spawnSwitchErr('proxyId', proxyId);
         }
@@ -140,7 +133,6 @@ export class AssetWrapper {
                 await assetWrapper.setAllowanceAsync(userAddress, assetData, desiredAllowance);
                 return;
             }
-
             case constants.ERC721_PROXY_ID: {
                 if (!desiredAllowance.eq(0) && !desiredAllowance.eq(1)) {
                     throw new Error(`Allowance for ERC721 token can only be set to 0 or 1. Got: ${desiredAllowance}`);
@@ -164,8 +156,9 @@ export class AssetWrapper {
                     assetProxyData.tokenAddress,
                     assetProxyData.tokenId,
                 );
-                // TODO: We should have a way to deal with this. Things get hairier once we are testing
+                // HACK: We do not currently support ApprovedForAll when setting proxy allowance
-                // batch fills
+                // This was intentional since unsetting ApprovedForAll, will unset approval for unrelated
+                // tokens other then the one specified in the call to this method.
                 if (isProxyApprovedForAll) {
                     throw new Error(`We don't currently support the use of "approveAll" functionality for ERC721.`);
                 }
@@ -177,21 +170,16 @@ export class AssetWrapper {
                 if (!isProxyApproved && desiredAllowance.eq(1)) {
                     await erc721Wrapper.approveProxyAsync(assetProxyData.tokenAddress, assetProxyData.tokenId);
                 } else if (isProxyApproved && desiredAllowance.eq(0)) {
+                    // Remove approval
                     await erc721Wrapper.approveAsync(
                         constants.NULL_ADDRESS,
                         assetProxyData.tokenAddress,
                         assetProxyData.tokenId,
                     );
-                } else if (
-                    (!isProxyApproved && desiredAllowance.eq(0)) ||
-                    (isProxyApproved && desiredAllowance.eq(1))
-                ) {
-                    return; // noop
                 }
 
                 break;
             }
-
             default:
                 throw errorUtils.spawnSwitchErr('proxyId', proxyId);
         }