Remove param address _facilitator from GatewayRedeemInterface.redeem and EIP20CoGateway.redeem #517

gulshanvasnani · 2018-12-05T03:51:01Z

Removed facilitator param from redeem method in GatewayRedeemInterface and EIP20CoGateway contracts.
Also,facilitator has been removed from redeem and stake struct.
It contains unit test-cases to test redeem method.
Burn method in utilitytoken is in sync with UtilityTokenInterface.
Fixes:- #475

schemar

🙌 LGTM

…litator_redeem

…gh_475_remove_facilitator_redeem

deepesh-kn

Nice 👍
Just a few inline comments.

deepesh-kn · 2018-12-06T17:52:37Z

contracts/test/MockEIP20CoGateway.sol

+ *
+ * @notice Used for test only.
+ */
+contract MockEIP20CoGateway is EIP20CoGateway {


Rename this as TestEIP20CoGateway, since it's not mocking the functions but uses addition functions to set data.

This contract is removed.

deepesh-kn · 2018-12-06T17:55:02Z

contracts/test/MockEIP20CoGateway.sol

+
+    /* Public functions */
+
+    function setOutboxStatus(bytes32 messageHash) public {


Documentation is missing.
We should also add a dev note why this function was needed.

Same as above

test/gateway/eip20_cogateway/redeem.js

deepesh-kn · 2018-12-06T17:58:04Z

test/gateway/eip20_cogateway/redeem.js

+    await utilityToken.approve(
+        eip20CoGateway.address,
+        new BN(1000),
+        {from: accounts[7]},


{from: accounts[7]} -> {form: staker}

test/gateway/eip20_cogateway/redeem.js

deepesh-kn · 2018-12-06T18:27:04Z

test/gateway/eip20_cogateway/redeem.js

+        assert.strictEqual(
+            ((await utilityToken.balanceOf(accounts[7])).cmp(
+                new BN(stakerBalance - amount))),
+            0,


Same as above.

deepesh-kn · 2018-12-06T18:47:52Z

test/gateway/eip20_gateway/deactivate_gateway.js

+
+    it('should deactivate if activated', async function () {
+
+        assert((await gateway.deactivateGateway.call({from: organisation})));


we can

split this to be more readable.

use assert.strictEqual.

add assert fail reason.

These changes were introduced when i pulled the below branch to sync up :-
https://github.com/OpenSTFoundation/mosaic-contracts/pull/518/files

deepesh-kn · 2018-12-06T18:50:30Z

test/gateway/eip20_gateway/deactivate_gateway.js

+        );
+    });
+
+    it('should deactivated by organization only', async function () {


should fail to deactivated when the caller is not an organization address.

Same as above.

…thub.com/gulshanvasnani/mosaic-contracts into gulshan/gh_475_remove_facilitator_redeem

…litator_redeem

…gh_475_remove_facilitator_redeem

deepesh-kn

🐎 Almost done.
There are few tests missing.
Added inline comment for some cleanups.

contracts/gateway/EIP20CoGateway.sol

test/gateway/eip20_cogateway/redeem.js

deepesh-kn · 2018-12-10T13:54:17Z

test/gateway/eip20_cogateway/redeem.js

+
+    });
+
+});


newline at the end of the file.

I think there can be more test cases.

should fail when the coGateway is not approved

should fail when the redeemer has balance less than the bounty amount

should fail when the redeemer balance is less than the redeem amount

Thank you.
I have added all the above test-cases.

test/gateway/eip20_cogateway/redeem.js

gulshanvasnani · 2018-12-11T09:25:27Z

contracts/gateway/EIP20CoGateway.sol

@@ -957,19 +948,19 @@ contract EIP20CoGateway is GatewayBase {
        require(


I couldn't test this condition.
Please recommend how to test it.

Do you think that moving this condition to line 920 makes sense?

I have moved it at 920 line number.

0xsarvesh

Nice 🙌 🚀

This is almost done. Few comments inline.

contracts/gateway/GatewayRedeemInterface.sol

test/gateway/eip20_cogateway/redeem.js

…gh_475_remove_facilitator_redeem

…thub.com/gulshanvasnani/mosaic-contracts into gulshan/gh_475_remove_facilitator_redeem

0xsarvesh

LGTM 🚀⚡️🌀 🎊
Nice work 🙌

There 2 changes required in unit test cases:

should fail when the redeemer 's base token balance is less than the bounty amount : This should assert this "Payable amount should be equal to the bounty amount."
Please add this test : should fail if the previous process is in revocation declared state

test/gateway/eip20_cogateway/redeem.js

…ed,previous process is in declared revocation state and used TestEIP20CoGateway contract.

0xsarvesh

Thank you for adding test cases. Redeem test cases really looks good to me. 🥇 💯 👍

deepesh-kn

Just few comments, it will be good to go.

deepesh-kn · 2018-12-13T07:53:19Z

contracts/gateway/EIP20CoGateway.sol

@@ -957,19 +948,19 @@ contract EIP20CoGateway is GatewayBase {
        require(


Do you think that moving this condition to line 920 makes sense?

contracts/test/TestEIP20CoGateway.sol

test/gateway/eip20_cogateway/redeem.js

deepesh-kn · 2018-12-13T08:20:50Z

test/gateway/eip20_cogateway/redeem.js

+
+    it('should fail when cogateway is not approved with redeem amount', async function () {
+
+        let testEIP20CoGateway2 = await TestEIP20CoGateway.new(


Why create a new object?
This can be done in 2 ways:

Change the redeemer address.

Increase the redeem amount.

Thank you.
I have increased the amount.

test/gateway/eip20_cogateway/redeem.js

…thub.com/gulshanvasnani/mosaic-contracts into gulshan/gh_475_remove_facilitator_redeem

…ndeclared

…emove_facilitator_redeem

deepesh-kn

LGTM ✈️

Removed facilitator address

50d7894

benjaminbollen added the review label Dec 5, 2018

0xsarvesh assigned gulshanvasnani Dec 5, 2018

abhayks1 mentioned this pull request Dec 5, 2018

Update TokenHolder.redeem signature after _facilitator argument is removed from CoGateway.redeem() in mosaic-contracts. OpenST/openst-contracts#81

Closed

Merge branch 'develop' into gulshan/gh_475_remove_facilitator_redeem

3ea2f6d

schemar reviewed Dec 5, 2018

View reviewed changes

schemar requested review from 0xsarvesh and deepesh-kn December 5, 2018 10:36

gulshanvasnani added 2 commits December 5, 2018 16:37

Merge branch 'Remove_gateway_linking' into gulshan/gh_475_remove_faci…

f2326fd

…litator_redeem

Merged PR's

3225318

0xsarvesh mentioned this pull request Dec 5, 2018

Support zero gas price and gas limit in stake & redeem #521

Merged

gulshanvasnani and others added 6 commits December 5, 2018 23:26

Test cases for redeem method

7331ba9

Merge remote-tracking branch 'remotes/upstream/develop' into gulshan/…

470bdea

…gh_475_remove_facilitator_redeem

Travis build fix and moved redeem.js to eip20_cogateway

c06ac66

Changed dependency for bignumber to bn.js

b92b94b

Verification of event

9c9e9ea

Merge branch 'develop' into gulshan/gh_475_remove_facilitator_redeem

a6d7011

deepesh-kn reviewed Dec 6, 2018

View reviewed changes

gulshanvasnani added 4 commits December 7, 2018 13:06

PR feedback changes

c6f9588

Merge branch 'gulshan/gh_475_remove_facilitator_redeem' of https://gi…

9b7b4dc

…thub.com/gulshanvasnani/mosaic-contracts into gulshan/gh_475_remove_facilitator_redeem

Merge branch 'Remove_gateway_linking' into gulshan/gh_475_remove_faci…

2742e06

…litator_redeem

Merge remote-tracking branch 'remotes/upstream/develop' into gulshan/…

830dec0

…gh_475_remove_facilitator_redeem

deepesh-kn reviewed Dec 10, 2018

View reviewed changes

PR feedback changes

27a2c94

gulshanvasnani commented Dec 11, 2018

View reviewed changes

Merge branch 'develop' into gulshan/gh_475_remove_facilitator_redeem

0802e1e

0xsarvesh suggested changes Dec 11, 2018

View reviewed changes

0xsarvesh and others added 3 commits December 11, 2018 20:56

Merge branch 'develop' into gulshan/gh_475_remove_facilitator_redeem

c70cff2

Merge remote-tracking branch 'remotes/upstream/develop' into gulshan/…

90c06db

…gh_475_remove_facilitator_redeem

Merge branch 'gulshan/gh_475_remove_facilitator_redeem' of https://gi…

470df85

…thub.com/gulshanvasnani/mosaic-contracts into gulshan/gh_475_remove_facilitator_redeem

gulshanvasnani added 5 commits December 12, 2018 13:08

Updated and added new test cases

c1d467c

Fixed indentation

90261c0

Removed hard coding of approval amount and fixed test-case

7e8f4b3

Indentation changes for expectRevert method

6394012

Corrected the error message and updated the test case messages

2da6509

0xsarvesh approved these changes Dec 12, 2018

View reviewed changes

test/gateway/eip20_cogateway/redeem.js Outdated Show resolved Hide resolved

deepesh-kn mentioned this pull request Dec 12, 2018

Gateway: Unit test for EIP20CoGateway redeem() #436

Closed

gulshanvasnani and others added 3 commits December 12, 2018 21:23

Added test case when redeemer sends less base token value than requir…

d44013f

…ed,previous process is in declared revocation state and used TestEIP20CoGateway contract.

Added TestEIP20CoGateway

68ae9ff

Merge branch 'develop' into gulshan/gh_475_remove_facilitator_redeem

57c961d

0xsarvesh reviewed Dec 13, 2018

View reviewed changes

deepesh-kn reviewed Dec 13, 2018

View reviewed changes

gulshanvasnani added 7 commits December 13, 2018 17:34

Modified test case as per the PR feedback

12d605e

Merge branch 'gulshan/gh_475_remove_facilitator_redeem' of https://gi…

0c56943

…thub.com/gulshanvasnani/mosaic-contracts into gulshan/gh_475_remove_facilitator_redeem

Added unit test case to verify that the message status at outbox is u…

44c2c6c

…ndeclared

Merge remote-tracking branch 'upstream/develop' into gulshan/gh_475_r…

0625c5a

…emove_facilitator_redeem

Added burner address for eip20cogateway constructor

d1eb483

Merge remote-tracking branch 'upstream/develop' into gulshan/gh_475_r…

9fe9391

…emove_facilitator_redeem

Styling changes

cc94151

deepesh-kn approved these changes Dec 13, 2018

View reviewed changes

deepesh-kn merged commit 84ae349 into OpenST:develop Dec 13, 2018

benjaminbollen removed the review label Dec 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove param address _facilitator from GatewayRedeemInterface.redeem and EIP20CoGateway.redeem #517

Remove param address _facilitator from GatewayRedeemInterface.redeem and EIP20CoGateway.redeem #517

gulshanvasnani commented Dec 5, 2018 •

edited

Loading

schemar left a comment

deepesh-kn left a comment

deepesh-kn Dec 6, 2018

gulshanvasnani Dec 12, 2018

deepesh-kn Dec 6, 2018

gulshanvasnani Dec 12, 2018

deepesh-kn Dec 6, 2018

deepesh-kn Dec 6, 2018

deepesh-kn Dec 6, 2018

gulshanvasnani Dec 7, 2018

deepesh-kn Dec 6, 2018

gulshanvasnani Dec 7, 2018

deepesh-kn left a comment

deepesh-kn Dec 10, 2018

gulshanvasnani Dec 11, 2018

gulshanvasnani Dec 11, 2018

deepesh-kn Dec 13, 2018

gulshanvasnani Dec 13, 2018

0xsarvesh left a comment

0xsarvesh left a comment •

edited

Loading

0xsarvesh left a comment

deepesh-kn left a comment

deepesh-kn Dec 13, 2018

deepesh-kn Dec 13, 2018

gulshanvasnani Dec 13, 2018

deepesh-kn left a comment


		/* Public functions */

		function setOutboxStatus(bytes32 messageHash) public {


		it('should deactivate if activated', async function () {

		assert((await gateway.deactivateGateway.call({from: organisation})));

		@@ -957,19 +948,19 @@ contract EIP20CoGateway is GatewayBase {
		require(


		it('should fail when cogateway is not approved with redeem amount', async function () {

		let testEIP20CoGateway2 = await TestEIP20CoGateway.new(

Remove param address _facilitator from GatewayRedeemInterface.redeem and EIP20CoGateway.redeem #517

Remove param address _facilitator from GatewayRedeemInterface.redeem and EIP20CoGateway.redeem #517

Conversation

gulshanvasnani commented Dec 5, 2018 • edited Loading

schemar left a comment

Choose a reason for hiding this comment

deepesh-kn left a 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

deepesh-kn left a 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

0xsarvesh left a comment

Choose a reason for hiding this comment

0xsarvesh left a comment • edited Loading

Choose a reason for hiding this comment

0xsarvesh left a comment

Choose a reason for hiding this comment

deepesh-kn left a 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

deepesh-kn left a comment

Choose a reason for hiding this comment

gulshanvasnani commented Dec 5, 2018 •

edited

Loading

0xsarvesh left a comment •

edited

Loading