Exchange Redeem Feature #402

felix2feng · 2019-02-28T22:33:41Z

Wrote exchangeRedeem feature with unit tests
Renamed exchangeIssueParams to exchangeInteractData; Bumped to v30 set-protocol-utils
Added depositModule and withdrawModule as convenience methods.
Added coverage by testing ExchangeInteractLibrary

coveralls · 2019-02-28T22:50:45Z

Pull Request Test Coverage Report for Build 4617

47 of 47 (100.0%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 100.0%

Totals
Change from base Build 4608:	0.2%
Covered Lines:	919
Relevant Lines:	919

💛 - Coveralls

bweick · 2019-03-01T01:15:49Z

contracts/core/modules/ExchangeRedeemModule.sol

+        nonReentrant
+    {
+        // Validate ExchangeRedeemParams
+        validateExchangeIssueParams(_exchangeInteractData);


Can we give this a more general name? The juxtapostion between the comment and this is kinda confusing

Yeah in a separate PR, I'm going to rename everything to exchangeIssuance (requires set-protocol-utils updates)

I ended up adding it to this PR

asoong · 2019-03-01T07:24:50Z

contracts/core/exchange-wrappers/ZeroExExchangeWrapper.sol

                setTransferProxy,
                receiveTokenAmounts[i]
-            );
+            ); 


White space

asoong · 2019-03-01T07:27:55Z

contracts/core/extensions/CoreIssuance.sol


    /**
-     * Exchange Set tokens for underlying components
+     * Exchange Set tokens for underlying components into the vault


into the vault?

asoong · 2019-03-01T07:30:44Z

contracts/core/interfaces/ICore.sol

-     * @param  _token           Address of token being withdrawn
     * @param  _from            Address to decredit for withdraw
     * @param  _to              Address to transfer tokens to
+     * @param  _token           Address of token being withdrawn


I swear this has been moved around multiple times already. Last time I remember I decided on the previous way because it read better like a sentence:

"Transferring _token _from address _to address"

Ok - I'm happy either way as long as its consistent. I think it got muddled when we started adding the batch functions where the arrays are put on the bottom

address _from, address _to, address[] calldata _tokens, uint256[] calldata _quantities )

Can decide once and for all in a subsequent PR

asoong · 2019-03-01T07:31:17Z

contracts/core/lib/ExchangeInteractLibrary.sol


 /**
- * @title ExchangeValidationLibrary
+ * @title ExchangeInteractLibrary


Interact? What is this supposed to imply?

open for other names. Other idea is ExchangeIssuanceLibrary (hopefully implying issuance covers issue and redeem)

asoong · 2019-03-01T07:31:25Z

contracts/core/lib/ExchangeInteractLibrary.sol

 */
-library ExchangeValidationLibrary {
+library ExchangeInteractLibrary {
+    // ============ Structs ============


Add white spaces around these

asoong · 2019-03-01T07:56:43Z

test/contracts/core/lib/exchangeInteractLibraryMock.spec.ts

+    it('should not revert', async () => {
+      await subject();
+
+      expect(1).to.equal(1);


asoong · 2019-03-01T07:58:10Z

test/contracts/core/lib/exchangeInteractLibraryMock.spec.ts

+
+    beforeEach(async () => {
+      subjectSetAddress = setToken.address;
+      subjectQuantity = subjectQuantity || ether(4);


you dont need this || pattern, if you set it in the cases own beforeEach, it will override

asoong · 2019-03-01T07:59:54Z

test/contracts/core/lib/exchangeInteractLibraryMock.spec.ts

+    it('should not revert', async () => {
+      await subject();
+
+      expect(1).to.equal(1);


Would this tell you if it reverted? Maybe check that the resulting transaction hash is not null?

asoong · 2019-03-01T08:01:56Z

contracts/core/lib/ExchangeInteractLibrary.sol

+        address[] sentTokens;
+        uint256[] sentTokenAmounts;
+        address[] receiveTokens;
+        uint256[] receiveTokenAmounts;


One is past tense and the other is present. in favor of receivedTokens

asoong · 2019-03-01T08:03:55Z

test/contracts/core/modules/exchangeRedeemModule.spec.ts

+
+  before(async () => {
+    ABIDecoder.addABI(Core.abi);
+    ABIDecoder.addABI(ExchangeRedeemModule.abi);


Can we add multiple ABIs? I cant find it but I swear in the past there was a rule that we could only add one.

Yeah i believe so. Creation and Checking exchange redeem logs are working

asoong · 2019-03-01T19:32:07Z

contracts/core/extensions/CoreModuleInteraction.sol

+        external
+        onlyModule
+    {
+


asoong · 2019-03-01T19:32:17Z

contracts/core/extensions/CoreModuleInteraction.sol

+        uint256[] memory quantityArray = new uint256[](1);
+        quantityArray[0] = _quantity;
+
+


asoong · 2019-03-01T19:32:29Z

contracts/core/extensions/CoreModuleInteraction.sol

+        external
+        onlyModule
+    {
+


asoong · 2019-03-01T19:32:35Z

contracts/core/extensions/CoreModuleInteraction.sol

+        uint256[] memory quantityArray = new uint256[](1);
+        quantityArray[0] = _quantity;
+
+


asoong · 2019-03-01T19:33:43Z

contracts/core/lib/ExchangeIssuanceLibrary.sol

-            "ExchangeValidationLibrary.validateSentTokenParams: Sent token inputs must be of the same length"
+            _sendTokenExchangeIds.length == _sendTokens.length && 
+            _sendTokens.length == _sendTokenAmounts.length,
+            "ExchangeIssuanceLibrary.validateSendTokenParams: Sent token inputs must be of the same length"


felix2feng force-pushed the felix/exchange-redemption branch from 4419864 to 338a816 Compare March 1, 2019 01:08

bweick reviewed Mar 1, 2019

View reviewed changes

asoong reviewed Mar 1, 2019

View reviewed changes

asoong approved these changes Mar 1, 2019

View reviewed changes

felix2feng added 20 commits March 1, 2019 11:43

wrote and test

3ca15b0

Add test

acc5232

Clean up

6adb92e

Update some comments

420dd16

Fix params

6619e5c

Fix tests

23ff4c9

Fix coverage

67271ff

Add to export and deploy script

9c0e83c

Add coverage to quantity 0

46ea555

Add more coverage

17abd7e

Try to see if coverage works

89c59a2

Add exchange validation library mock spec

7ef2c43

Add exchange validation library mock spec 2

a50b92f

Fix linting

8599c39

Fix naming

ad32e28

Remove unused imports

9db49b2

Refactor to coreIssuance

81e631a

Refactor to send vs sent

37b5ff8

Improve comments

1c738e2

Fix whitespace

5acd5e4

felix2feng force-pushed the felix/exchange-redemption branch from 67f4583 to 5acd5e4 Compare March 1, 2019 19:45

felix2feng added 2 commits March 1, 2019 12:00

Update coreModule spec

e6809d1

Improve moduleinteraction specs

920b5b3

felix2feng merged commit 4c46497 into master Mar 1, 2019

felix2feng deleted the felix/exchange-redemption branch March 1, 2019 20:27

		uint256[] memory quantityArray = new uint256[](1);
		quantityArray[0] = _quantity;

Exchange Redeem Feature #402

Exchange Redeem Feature #402

Uh oh!

Conversation

felix2feng commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 4617

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

felix2feng commented Feb 28, 2019 •

edited

Loading

coveralls commented Feb 28, 2019 •

edited

Loading