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

Gateways are now organized #515

Merged
merged 8 commits into from Dec 7, 2018
Merged

Conversation

schemar
Copy link
Contributor

@schemar schemar commented Dec 4, 2018

⚠️ First review and merge #514 ⚠️

I made the GatewayBase organized. As a result, the contracts that
directly or indirectly extend the GatewayBase must now provide an
address of a worker manager contract that implements the interface
IsWorkerInterface. All relevant modifiers were replaced by
onlyWorker.

Tests were updated to reflect the change. In order to easily test
organized contracts, I created a MockWorkerManager that can be used in
tests and has a single valid worker address.

I also re-organized the contracts/test directory a little bit with
additional sub-directories.

Fixes #486

I made the `GatewayBase` organized. As a result, the contracts that
directly or indirectly extend the `GatewayBase` must now provide an
address of a worker manager contract that implements the interface
`IsWorkerInterface`. All relevant modifiers were replaced by
`onlyWorker`.

Tests were updated to reflect the change. In order to easily test
organized contracts, I created a `MockWorkerManager` that can be used in
tests and has a single valid worker address.

I also re-organized the `contracts/test` directory a little bit with
additional sub-directories.

Fixes OpenST#486
@schemar schemar mentioned this pull request Dec 5, 2018
Copy link
Contributor Author

@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.

@deepesh-kn @gulshanvasnani This PR is now ready for review.

I added isOwner to the IsMemberInterface (formally isWorkerInterface) and the onlyOrganization modifier.

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.

⭐️ 🌟
Just 2 inline comment.

Also, a lot of existing tests (touched in this PR) contains expectThrow instead of expectRevert and no error message is provided. We can create a ticket for that. What's your opinion?

contracts/gateway/EIP20CoGateway.sol Outdated Show resolved Hide resolved
contracts/test/gateway/MockEIP20Gateway.sol Outdated Show resolved Hide resolved
Copy link
Contributor Author

@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.

Addressed the mentioned changes 👍

Regarding this question:

Also, a lot of existing tests (touched in this PR) contains expectThrow instead of expectRevert and no error message is provided. We can create a ticket for that. What's your opinion?

There is #406. We could extend that to include error messages.

contracts/gateway/EIP20CoGateway.sol Outdated Show resolved Hide resolved
contracts/test/gateway/MockEIP20Gateway.sol Outdated Show resolved Hide resolved
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 💯

There is #406. We could extend that to include error messages.
👍

@deepesh-kn deepesh-kn merged commit 1c5d25b into OpenST:develop Dec 7, 2018
@schemar schemar deleted the organized_gateways branch December 7, 2018 15:48
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.

None yet

4 participants