Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove cogateway deactivation logic #518

Merged

Conversation

0xsarvesh
Copy link
Contributor

This should be merged after #512

Removed deactivation logic from GatewayBase and now only gateway and co-gateway can be deactivated. Also, the stake can only be called if the gateway is activated.

@0xsarvesh 0xsarvesh mentioned this pull request Dec 5, 2018
@schemar
Copy link
Contributor

schemar commented Dec 5, 2018

@sarvesh-ost can you please merge develop and resolve the conflicts before we review it?

@0xsarvesh
Copy link
Contributor Author

@sarvesh-ost can you please merge develop and resolve the conflicts before we review it?

It's now available for review.

Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘💨 Nice one!
Only two minor things...

@@ -197,6 +200,17 @@ contract EIP20Gateway is GatewayBase {
/** Maps messageHash to the Unstake object. */
mapping(bytes32 /*messageHash*/ => Unstake) unstakes;

/* modifiers */

/** checks that contract is activated */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** checks that contract is activated */
/** Checks that contract is active. */


await _setup(accounts, gateway);
await _prepareData();
await _stake(utils.ResultType.FAIL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the gateway activated in the other test cases? 😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have the ability to activate.
So we can now delete TestEIP20Gateway contract.
Previously it was the part of linking process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the mock contract, it has been set to true on construction.
Link

Copy link
Contributor

@deepesh-kn deepesh-kn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Few inline comments.

contracts/gateway/EIP20Gateway.sol Outdated Show resolved Hide resolved
test/gateway/eip20_gateway/activate_gateway.js Outdated Show resolved Hide resolved
test/gateway/eip20_gateway/activate_gateway.js Outdated Show resolved Hide resolved
test/gateway/eip20_gateway/activate_gateway.js Outdated Show resolved Hide resolved
test/gateway/eip20_gateway/deactivate_gateway.js Outdated Show resolved Hide resolved

await _setup(accounts, gateway);
await _prepareData();
await _stake(utils.ResultType.FAIL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have the ability to activate.
So we can now delete TestEIP20Gateway contract.
Previously it was the part of linking process.

Copy link
Contributor

@deepesh-kn deepesh-kn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🙌

@deepesh-kn deepesh-kn merged commit 80ae1a5 into OpenST:develop Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants