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.

Sign up for GitHub

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

Jump to bottom

Added organization library #513

Merged
merged 14 commits into from
Dec 5, 2018
Merged

Added organization library #513

merged 14 commits into from
Dec 5, 2018

Conversation

schemar
Copy link
Contributor

@schemar schemar commented Dec 4, 2018

The organization library can be used for contracts that must be managed
by an organization that can manage owners, admins, and workers.

Fixes #484

@schemar
Added organization library
4f18dc0 
The organization library can be used for contracts that must be managed
by an organization that can manage owners, admins, and workers.

Fixes OpenST#484
@schemar schemar self-assigned this Dec 4, 2018
@schemar schemar mentioned this pull request Dec 4, 2018
Merged
0xsarvesh
0xsarvesh suggested changes Dec 5, 2018
View reviewed changes
Copy link
Contributor

@0xsarvesh 0xsarvesh left a comment

Choose a reason for hiding this comment

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

Nice 👍 🙌

Few comments and question inline.

We have Owner, Admin and Workers keys. The owner can add an admin, admin and owner can add workers. Ideally, I would expect owner and admin should be able to perform all those operations that worker can perform.
Where ever we add onlyWorker modifier, it should also allow admin and owner key to perform that operation? 🤔

contracts/lib/IsWorkerInterface.sol Show resolved Hide resolved
contracts/lib/Organization.sol Show resolved Hide resolved
contracts/lib/Organization.sol Outdated Show resolved Hide resolved
contracts/lib/Organization.sol Outdated Show resolved Hide resolved
contracts/lib/OrganizationInterface.sol Show resolved Hide resolved
test/lib/organization/complete_ownership_transfer.js Outdated Show resolved Hide resolved
test/lib/organization/is_worker.js Outdated Show resolved Hide resolved
test/lib/organization/set_worker.js Show resolved Hide resolved
test/lib/organization/set_worker.js Show resolved Hide resolved
deepesh-kn
deepesh-kn reviewed Dec 5, 2018
View reviewed changes
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.

👍 Looks nice.
I just have a few inline comments.
One of the questions was why are we using assert.ok in a test? It will always pass for any undefined data.

contracts/lib/Organization.sol Outdated Show resolved Hide resolved
test/lib/organization/complete_ownership_transfer.js Outdated Show resolved Hide resolved
test/lib/organization/initiate_ownership_transfer.js Outdated Show resolved Hide resolved
deepesh-kn
deepesh-kn reviewed Dec 5, 2018
View reviewed changes
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.

👍
I just have one concern related to a test case Checks for added worker, isWorker returns true. Please check inline comment.

test/lib/organization/unset_worker.js Outdated Show resolved Hide resolved
test/lib/organization/is_worker.js Outdated Show resolved Hide resolved
test/lib/organization/initiate_ownership_transfer.js Outdated Show resolved Hide resolved
test/lib/organization/complete_ownership_transfer.js Outdated Show resolved Hide resolved
contracts/lib/OrganizationInterface.sol Show resolved Hide resolved
schemar
schemar commented Dec 5, 2018
View reviewed changes
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.

I did all the requested changes.

contracts/lib/OrganizationInterface.sol Show resolved Hide resolved
test/lib/organization/complete_ownership_transfer.js Outdated Show resolved Hide resolved
test/lib/organization/initiate_ownership_transfer.js Outdated Show resolved Hide resolved
test/lib/organization/is_worker.js Outdated Show resolved Hide resolved
test/lib/organization/unset_worker.js Outdated Show resolved Hide resolved
deepesh-kn
deepesh-kn approved these changes Dec 5, 2018
View reviewed changes
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 🚀 ⏩

0xsarvesh
0xsarvesh approved these changes Dec 5, 2018
View reviewed changes
Copy link
Contributor

@0xsarvesh 0xsarvesh 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 c983607 into OpenST:develop Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xsarvesh 0xsarvesh 0xsarvesh approved these changes

@deepesh-kn deepesh-kn deepesh-kn approved these changes

@gulshanvasnani gulshanvasnani Awaiting requested review from gulshanvasnani

Assignees

@schemar schemar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@schemar @deepesh-kn @0xsarvesh @benjaminbollen