From c618d87e11de30debaeb14d682a1c5c2023d3b54 Mon Sep 17 00:00:00 2001 From: felix2feng Date: Wed, 20 Mar 2019 16:12:33 -0700 Subject: [PATCH 1/4] Fix case --- .../core/modules/ExchangeIssuanceModule.sol | 27 +++++++++++- .../RebalancingSetExchangeIssuanceModule.sol | 43 +++++++++++++++---- ...balancingSetExchangeIssuanceModule.spec.ts | 36 ++++++++++++---- utils/wrappers/coreWrapper.ts | 2 + 4 files changed, 89 insertions(+), 19 deletions(-) diff --git a/contracts/core/modules/ExchangeIssuanceModule.sol b/contracts/core/modules/ExchangeIssuanceModule.sol index 088ae782c..cbc4449b0 100644 --- a/contracts/core/modules/ExchangeIssuanceModule.sol +++ b/contracts/core/modules/ExchangeIssuanceModule.sol @@ -152,7 +152,7 @@ contract ExchangeIssuanceModule is _exchangeIssuanceParams.sendTokens ); - // Redeem Set into the vault, attributed to this contract + // Redeem Set into the vault, attributing components to this contract coreInstance.redeemModule( msg.sender, address(this), @@ -178,6 +178,9 @@ contract ExchangeIssuanceModule is _exchangeIssuanceParams.receiveTokenAmounts ); + // Withdraw non-exchanged components to the user + withdrawRemainingComponentsToUser(_exchangeIssuanceParams.setAddress); + emit LogExchangeRedeem( _exchangeIssuanceParams.setAddress, msg.sender, @@ -277,4 +280,26 @@ contract ExchangeIssuanceModule is ); } } + + function withdrawRemainingComponentsToUser( + address _setAddress + ) + private + { + address[] memory baseSetComponents = ISetToken(_setAddress).getComponents(); + uint256[] memory baseSetWithdrawQuantities = new uint256[](baseSetComponents.length); + for (uint256 i = 0; i < baseSetComponents.length; i++) { + uint256 withdrawQuantity = vaultInstance.getOwnerBalance(baseSetComponents[i], address(this)); + + baseSetWithdrawQuantities[i] = withdrawQuantity; + } + + // Return the unexchanged components to the user + coreInstance.batchWithdrawModule( + address(this), + msg.sender, + baseSetComponents, + baseSetWithdrawQuantities + ); + } } diff --git a/contracts/core/modules/RebalancingSetExchangeIssuanceModule.sol b/contracts/core/modules/RebalancingSetExchangeIssuanceModule.sol index 30242bdee..02cb16f6e 100644 --- a/contracts/core/modules/RebalancingSetExchangeIssuanceModule.sol +++ b/contracts/core/modules/RebalancingSetExchangeIssuanceModule.sol @@ -29,6 +29,7 @@ import { IRebalancingSetToken } from "../interfaces/IRebalancingSetToken.sol"; import { ISetToken } from "../interfaces/ISetToken.sol"; import { ITransferProxy } from "../interfaces/ITransferProxy.sol"; import { IWETH } from "../../lib/IWETH.sol"; +import { ModuleCoreState } from "./lib/ModuleCoreState.sol"; /** @@ -39,16 +40,13 @@ import { IWETH } from "../../lib/IWETH.sol"; * issue a rebalancing Set */ contract RebalancingSetExchangeIssuanceModule is + ModuleCoreState, ReentrancyGuard { using SafeMath for uint256; /* ============ State Variables ============ */ - // Address and instance of Core contract - address public core; - ICore private coreInstance; - // Address and instance of Transfer Proxy contract address public transferProxy; @@ -83,19 +81,21 @@ contract RebalancingSetExchangeIssuanceModule is * @param _transferProxy The address of the TransferProxy * @param _exchangeIssuanceModule The address of ExchangeIssuanceModule * @param _wrappedEther The address of wrapped ether + * @param _vault The address of Vault */ constructor( address _core, address _transferProxy, address _exchangeIssuanceModule, - address _wrappedEther + address _wrappedEther, + address _vault ) public + ModuleCoreState( + _core, + _vault + ) { - // Commit the address and instance of Core to state variables - core = _core; - coreInstance = ICore(_core); - // Commit the address and instance of Transfer Proxy to state variables transferProxy = _transferProxy; @@ -249,6 +249,9 @@ contract RebalancingSetExchangeIssuanceModule is // Send eth to user msg.sender.transfer(wethBalance); + // Non-exchanged components are returned to the user + returnExcessBaseSetComponents(_exchangeIssuanceParams.setAddress); + emit LogPayableExchangeRedeem( _rebalancingSetAddress, msg.sender, @@ -367,4 +370,26 @@ contract RebalancingSetExchangeIssuanceModule is "RebalancingSetExchangeIssuanceModule.validateRedeemInputs: Base Set quantities must match" ); } + /** + * Withdraw any non-exchanged components to the user + * + * @param _setAddress Address of the Base Set + */ + function returnExcessBaseSetComponents( + address _setAddress + ) + private + { + address[] memory baseSetComponents = ISetToken(_setAddress).getComponents(); + for (uint256 i = 0; i < baseSetComponents.length; i++) { + uint256 withdrawQuantity = ERC20Wrapper.balanceOf(baseSetComponents[i], address(this)); + if (withdrawQuantity > 0) { + ERC20Wrapper.transfer( + baseSetComponents[i], + msg.sender, + withdrawQuantity + ); + } + } + } } diff --git a/test/contracts/core/modules/rebalancingSetExchangeIssuanceModule.spec.ts b/test/contracts/core/modules/rebalancingSetExchangeIssuanceModule.spec.ts index 0ac6d143b..a60b63420 100644 --- a/test/contracts/core/modules/rebalancingSetExchangeIssuanceModule.spec.ts +++ b/test/contracts/core/modules/rebalancingSetExchangeIssuanceModule.spec.ts @@ -103,6 +103,7 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { transferProxy.address, exchangeIssuanceModule.address, weth.address, + vault.address, ); await coreWrapper.addModuleAsync(core, rebalancingSetExchangeIssuanceModule.address); @@ -380,6 +381,8 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { let customExchangeRedeemQuantity: BigNumber; let baseSetComponent: StandardTokenMockContract; + let nonExchangedWethQuantity: BigNumber; + let baseSetToken: SetTokenContract; let baseSetNaturalUnit: BigNumber; let rebalancingSetToken: RebalancingSetTokenContract; @@ -403,9 +406,9 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { // Create component token baseSetComponent = await erc20Wrapper.deployTokenAsync(tokenPurchaser); - // Create the Set (1 component) - const componentAddresses = [baseSetComponent.address]; - const componentUnits = [new BigNumber(10 ** 10)]; + // Create the Set (2 component where one is WETH) + const componentAddresses = [baseSetComponent.address, weth.address]; + const componentUnits = [new BigNumber(10 ** 10), new BigNumber(10 ** 10)]; baseSetNaturalUnit = new BigNumber(10 ** 9); baseSetToken = await coreWrapper.createSetTokenAsync( core, @@ -430,10 +433,8 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { exchangeRedeemSetAddress = baseSetToken.address; exchangeRedeemQuantity = customExchangeRedeemQuantity || new BigNumber(10 ** 10); exchangeRedeemSendTokenExchangeIds = [SetUtils.EXCHANGES.ZERO_EX]; - exchangeRedeemSendTokens = componentAddresses; - exchangeRedeemSendTokenAmounts = componentUnits.map( - unit => unit.mul(exchangeRedeemQuantity).div(baseSetNaturalUnit) - ); + exchangeRedeemSendTokens = [componentAddresses[0]]; + exchangeRedeemSendTokenAmounts = [componentUnits[0].mul(exchangeRedeemQuantity).div(baseSetNaturalUnit)]; exchangeRedeemReceiveTokens = [weth.address]; exchangeRedeemReceiveTokenAmounts = [etherQuantityToReceive]; @@ -486,6 +487,20 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { tokenPurchaser ); + nonExchangedWethQuantity = componentUnits[1].mul(exchangeRedeemQuantity).div(baseSetNaturalUnit); + + // Approve Weth to the transferProxy + await weth.approve.sendTransactionAsync( + transferProxy.address, + nonExchangedWethQuantity, + { from: tokenPurchaser, gas: DEFAULT_GAS } + ); + + // Generate wrapped Ether for the caller + await weth.deposit.sendTransactionAsync( + { from: tokenPurchaser, value: nonExchangedWethQuantity.toString(), gas: DEFAULT_GAS } + ); + // Issue the Base Set to the vault await core.issueInVault.sendTransactionAsync( baseSetToken.address, @@ -534,13 +549,16 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { expect(expectedRBSetTokenBalance).to.bignumber.equal(currentRBSetTokenBalance); }); - it('should increment the users eth balance by the correct quantity', async () => { + it.only('should increment the users eth balance by the correct quantity', async () => { const previousEthBalance = new BigNumber(await web3.eth.getBalance(subjectCaller)); const txHash = await subject(); const totalGasInEth = await getGasUsageInEth(txHash); - const expectedEthBalance = previousEthBalance.add(etherQuantityToReceive).sub(totalGasInEth); + const expectedEthBalance = previousEthBalance + .add(etherQuantityToReceive) + .add(nonExchangedWethQuantity) + .sub(totalGasInEth); const currentEthBalance = await web3.eth.getBalance(subjectCaller); expect(currentEthBalance).to.bignumber.equal(expectedEthBalance); diff --git a/utils/wrappers/coreWrapper.ts b/utils/wrappers/coreWrapper.ts index 3c86c68cc..1a8aec28c 100644 --- a/utils/wrappers/coreWrapper.ts +++ b/utils/wrappers/coreWrapper.ts @@ -504,6 +504,7 @@ export class CoreWrapper { transferProxy: Address, exchangeIssuanceModule: Address, wrappedEther: Address, + vault: Address, from: Address = this._contractOwnerAddress ): Promise { const erc20WrapperLibrary = await ERC20Wrapper.new( @@ -517,6 +518,7 @@ export class CoreWrapper { transferProxy, exchangeIssuanceModule, wrappedEther, + vault, { from }, ); From e07734fd92d9c1d963c16f90d074c8c8d283c720 Mon Sep 17 00:00:00 2001 From: felix2feng Date: Wed, 20 Mar 2019 17:04:59 -0700 Subject: [PATCH 2/4] Add tests --- .../core/modules/ExchangeIssuanceModule.sol | 8 +- ...balancingSetExchangeIssuanceModule.spec.ts | 83 +++++++++++++++++-- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/contracts/core/modules/ExchangeIssuanceModule.sol b/contracts/core/modules/ExchangeIssuanceModule.sol index cbc4449b0..f17db303e 100644 --- a/contracts/core/modules/ExchangeIssuanceModule.sol +++ b/contracts/core/modules/ExchangeIssuanceModule.sol @@ -178,7 +178,7 @@ contract ExchangeIssuanceModule is _exchangeIssuanceParams.receiveTokenAmounts ); - // Withdraw non-exchanged components to the user + // Withdraw any remaining non-exchanged components to the user withdrawRemainingComponentsToUser(_exchangeIssuanceParams.setAddress); emit LogExchangeRedeem( @@ -281,6 +281,12 @@ contract ExchangeIssuanceModule is } } + /** + * Withdraws any remaining un-exchanged components from the Vault in the posession of this contract + * to the caller + * + * @param _setAddress Address of the Base Set + */ function withdrawRemainingComponentsToUser( address _setAddress ) diff --git a/test/contracts/core/modules/rebalancingSetExchangeIssuanceModule.spec.ts b/test/contracts/core/modules/rebalancingSetExchangeIssuanceModule.spec.ts index a60b63420..b4e177c1f 100644 --- a/test/contracts/core/modules/rebalancingSetExchangeIssuanceModule.spec.ts +++ b/test/contracts/core/modules/rebalancingSetExchangeIssuanceModule.spec.ts @@ -379,6 +379,10 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { let subjectCaller: Address; let customExchangeRedeemQuantity: BigNumber; + let customExchangeRedeemSendTokenAmounts: BigNumber[]; + let customBaseSetComponent: StandardTokenMockContract; + let customComponentAddresses: Address[]; + let customComponentUnits: BigNumber[]; let baseSetComponent: StandardTokenMockContract; let nonExchangedWethQuantity: BigNumber; @@ -404,11 +408,11 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { etherQuantityToReceive = ether(2); // Create component token - baseSetComponent = await erc20Wrapper.deployTokenAsync(tokenPurchaser); + baseSetComponent = customBaseSetComponent || await erc20Wrapper.deployTokenAsync(tokenPurchaser); // Create the Set (2 component where one is WETH) - const componentAddresses = [baseSetComponent.address, weth.address]; - const componentUnits = [new BigNumber(10 ** 10), new BigNumber(10 ** 10)]; + const componentAddresses = customComponentAddresses || [baseSetComponent.address, weth.address]; + const componentUnits = customComponentUnits || [new BigNumber(10 ** 10), new BigNumber(10 ** 10)]; baseSetNaturalUnit = new BigNumber(10 ** 9); baseSetToken = await coreWrapper.createSetTokenAsync( core, @@ -434,7 +438,8 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { exchangeRedeemQuantity = customExchangeRedeemQuantity || new BigNumber(10 ** 10); exchangeRedeemSendTokenExchangeIds = [SetUtils.EXCHANGES.ZERO_EX]; exchangeRedeemSendTokens = [componentAddresses[0]]; - exchangeRedeemSendTokenAmounts = [componentUnits[0].mul(exchangeRedeemQuantity).div(baseSetNaturalUnit)]; + exchangeRedeemSendTokenAmounts = customExchangeRedeemSendTokenAmounts || + [componentUnits[0].mul(exchangeRedeemQuantity).div(baseSetNaturalUnit)]; exchangeRedeemReceiveTokens = [weth.address]; exchangeRedeemReceiveTokenAmounts = [etherQuantityToReceive]; @@ -549,7 +554,7 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { expect(expectedRBSetTokenBalance).to.bignumber.equal(currentRBSetTokenBalance); }); - it.only('should increment the users eth balance by the correct quantity', async () => { + it('should increment the users eth balance by the correct quantity', async () => { const previousEthBalance = new BigNumber(await web3.eth.getBalance(subjectCaller)); const txHash = await subject(); @@ -588,6 +593,74 @@ contract('RebalancingSetExchangeIssuanceModule', accounts => { await SetTestUtils.assertLogEquivalence(formattedLogs, expectedLogs); }); + describe('when the Set has a component that has not been exchanged', async () => { + let nonExchangedNonWethComponent: StandardTokenMockContract; + + before(async () => { + nonExchangedNonWethComponent = await erc20Wrapper.deployTokenAsync(tokenPurchaser); + + customBaseSetComponent = await erc20Wrapper.deployTokenAsync(tokenPurchaser); + customComponentAddresses = [ + customBaseSetComponent.address, + weth.address, + nonExchangedNonWethComponent.address, + ]; + customComponentUnits = [new BigNumber(10 ** 10), new BigNumber(10 ** 10), new BigNumber(10 ** 10)]; + + await erc20Wrapper.approveTransfersAsync( + [nonExchangedNonWethComponent], + transferProxy.address, + tokenPurchaser + ); + }); + + after(async () => { + customBaseSetComponent = undefined; + customComponentAddresses = undefined; + customComponentUnits = undefined; + }); + + it('should send the extra asset to the caller', async () => { + const previousReturnedAssetBalance = await nonExchangedNonWethComponent.balanceOf.callAsync(subjectCaller); + const expectedReturnedAssetBalance = previousReturnedAssetBalance.add( + customComponentUnits[2].mul(exchangeRedeemQuantity).div(baseSetNaturalUnit) + ); + + await subject(); + + const currentReturnedAssetBalance = await nonExchangedNonWethComponent.balanceOf.callAsync(subjectCaller); + expect(expectedReturnedAssetBalance).to.bignumber.equal(currentReturnedAssetBalance); + }); + }); + + describe('when the quantity of send token is less than the components redeemed', async () => { + let halfBaseComponentQuantity: BigNumber; + + before(async () => { + const componentUnit = new BigNumber(10 ** 10); + const naturalUnit = new BigNumber(10 ** 9); + const redeemQuantity = new BigNumber(10 ** 10); + + halfBaseComponentQuantity = componentUnit.mul(redeemQuantity).div(naturalUnit).div(2); + + customExchangeRedeemSendTokenAmounts = [halfBaseComponentQuantity]; + }); + + after(async () => { + customExchangeRedeemSendTokenAmounts = undefined; + }); + + it('should send the unsold components to the caller', async () => { + const previousReturnedAssetBalance = await baseSetComponent.balanceOf.callAsync(subjectCaller); + const expectedReturnedAssetBalance = previousReturnedAssetBalance.add(halfBaseComponentQuantity); + + await subject(); + + const currentReturnedAssetBalance = await baseSetComponent.balanceOf.callAsync(subjectCaller); + expect(expectedReturnedAssetBalance).to.bignumber.equal(currentReturnedAssetBalance); + }); + }); + describe('when the receive tokens length is greater than 1', async () => { beforeEach(async () => { subjectExchangeIssuanceParams.receiveTokens = [weth.address, weth.address]; From df961350de0a5557c3a076b38fbefb01138b11e7 Mon Sep 17 00:00:00 2001 From: felix2feng Date: Wed, 20 Mar 2019 18:46:43 -0700 Subject: [PATCH 3/4] Add another test --- deployments/stages/3_modules.ts | 2 ++ .../modules/exchangeIssuanceModule.spec.ts | 24 +++++++++++++++---- ...ncingSetExchangeIssuanceScenarios.spec.ts} | 0 3 files changed, 22 insertions(+), 4 deletions(-) rename test/scenarios/{payable-exchange-issuance/payableExchangeIssuanceScenarios.spec.ts => rebalancing-set-exchange-issuance/rebalancingSetExchangeIssuanceScenarios.spec.ts} (100%) diff --git a/deployments/stages/3_modules.ts b/deployments/stages/3_modules.ts index bc64e175a..3d2e19720 100755 --- a/deployments/stages/3_modules.ts +++ b/deployments/stages/3_modules.ts @@ -144,6 +144,7 @@ export class ModulesStage implements DeploymentStageInterface { const transferProxyAddress = await getContractAddress(TransferProxy.contractName); const exchangeIssuanceAddress = await getContractAddress(ExchangeIssuanceModule.contractName); const erc20WrapperAddress = await getContractAddress(ERC20Wrapper.contractName); + const vaultAddress = await getContractAddress(Vault.contractName); const wethAddress = await findDependency(DEPENDENCY.WETH); const originalByteCode = RebalancingSetExchangeIssuanceModule.bytecode; @@ -158,6 +159,7 @@ export class ModulesStage implements DeploymentStageInterface { transferProxyAddress, exchangeIssuanceAddress, wethAddress, + vaultAddress, ], }).encodeABI(); diff --git a/test/contracts/core/modules/exchangeIssuanceModule.spec.ts b/test/contracts/core/modules/exchangeIssuanceModule.spec.ts index 2855f8587..0aba67405 100644 --- a/test/contracts/core/modules/exchangeIssuanceModule.spec.ts +++ b/test/contracts/core/modules/exchangeIssuanceModule.spec.ts @@ -539,10 +539,13 @@ contract('ExchangeIssuanceModule', accounts => { let subjectExchangeIssuanceParams: ExchangeIssuanceParams; let subjectExchangeOrdersData: Bytes; + let setComponentUnit: BigNumber; let naturalUnit: BigNumber; let setToken: SetTokenContract; let receiveToken: StandardTokenMockContract; + let nonExchangedComponent: StandardTokenMockContract; + let totalReceiveToken: BigNumber; let exchangeRedeemSetAddress: Address; @@ -577,10 +580,11 @@ contract('ExchangeIssuanceModule', accounts => { const firstComponent = erc20Wrapper.kyberReserveToken(SetTestUtils.KYBER_RESERVE_SOURCE_TOKEN_ADDRESS); const secondComponent = await erc20Wrapper.deployTokenAsync(contractDeployer); + nonExchangedComponent = await erc20Wrapper.deployTokenAsync(contractDeployer); receiveToken = erc20Wrapper.kyberReserveToken(SetTestUtils.KYBER_RESERVE_DESTINATION_TOKEN_ADDRESS); - const componentTokens = [firstComponent, secondComponent]; - const setComponentUnit = ether(4); + const componentTokens = [firstComponent, secondComponent, nonExchangedComponent]; + setComponentUnit = ether(4); const componentAddresses = componentTokens.map(token => token.address); const componentUnits = componentTokens.map(token => setComponentUnit); naturalUnit = ether(2); @@ -605,7 +609,7 @@ contract('ExchangeIssuanceModule', accounts => { exchangeRedeemSendTokens = exchangeRedeemSendTokens || [firstComponent.address, secondComponent.address]; exchangeRedeemSendTokenAmounts = - exchangeRedeemSendTokenAmounts || _.map(componentUnits, unit => unit + exchangeRedeemSendTokenAmounts || _.map(componentUnits.slice(0, 2), unit => unit .mul(exchangeRedeemQuantity) .div(naturalUnit) ); @@ -684,7 +688,7 @@ contract('ExchangeIssuanceModule', accounts => { ); await erc20Wrapper.approveTransfersAsync( - [firstComponent, secondComponent], + [firstComponent, secondComponent, nonExchangedComponent], transferProxy.address, contractDeployer ); @@ -736,6 +740,18 @@ contract('ExchangeIssuanceModule', accounts => { await expect(newBalance).to.be.bignumber.equal(expectedNewBalance); }); + it('returns the correct quantity of non-exchanged tokens', async () => { + const existingBalance = await nonExchangedComponent.balanceOf.callAsync(exchangeIssuanceCaller); + + await subject(); + + const incrementQuantity = setComponentUnit.mul(exchangeRedeemQuantity).div(naturalUnit); + const expectedNewBalance = existingBalance.add(incrementQuantity); + const newBalance = await nonExchangedComponent.balanceOf.callAsync(exchangeIssuanceCaller); + + await expect(newBalance).to.be.bignumber.equal(expectedNewBalance); + }); + it('emits correct LogExchangeRedeem event', async () => { const txHash = await subject(); diff --git a/test/scenarios/payable-exchange-issuance/payableExchangeIssuanceScenarios.spec.ts b/test/scenarios/rebalancing-set-exchange-issuance/rebalancingSetExchangeIssuanceScenarios.spec.ts similarity index 100% rename from test/scenarios/payable-exchange-issuance/payableExchangeIssuanceScenarios.spec.ts rename to test/scenarios/rebalancing-set-exchange-issuance/rebalancingSetExchangeIssuanceScenarios.spec.ts From 2bdf49e06831b69853d89482141b1c7d40aee751 Mon Sep 17 00:00:00 2001 From: felix2feng Date: Wed, 20 Mar 2019 19:28:35 -0700 Subject: [PATCH 4/4] Add check for 0 --- contracts/core/modules/ExchangeIssuanceModule.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/core/modules/ExchangeIssuanceModule.sol b/contracts/core/modules/ExchangeIssuanceModule.sol index f17db303e..ded13b2fa 100644 --- a/contracts/core/modules/ExchangeIssuanceModule.sol +++ b/contracts/core/modules/ExchangeIssuanceModule.sol @@ -296,8 +296,9 @@ contract ExchangeIssuanceModule is uint256[] memory baseSetWithdrawQuantities = new uint256[](baseSetComponents.length); for (uint256 i = 0; i < baseSetComponents.length; i++) { uint256 withdrawQuantity = vaultInstance.getOwnerBalance(baseSetComponents[i], address(this)); - - baseSetWithdrawQuantities[i] = withdrawQuantity; + if (withdrawQuantity > 0) { + baseSetWithdrawQuantities[i] = withdrawQuantity; + } } // Return the unexchanged components to the user