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

Feat: add update operator for all #141

Merged
merged 10 commits into from
Apr 23, 2019
Merged

Feat: add update operator for all #141

merged 10 commits into from
Apr 23, 2019

Conversation

nachomazzara
Copy link
Collaborator

@nachomazzara nachomazzara commented Apr 17, 2019 •
edited
Loading

  • Add UpdateOperatorForAll for LAND and Estate registry smart contracts

Interface:

setUpdateManager(address _owner, address _operator, bool _approved)

event UpdateManager(
    address indexed _owner,
    address indexed _operator,
    address indexed _caller,
    bool _approved
);

eordano and others added 2 commits April 17, 2019 14:50
@eordano @nachomazzara
Feat/update for all nit (#142)
e8c00fb 
* chore: clean whitespace at end of lines

* nit: || operators at the beginning of lines

Signed-off-by: Esteban Ordano <esteban@decentraland.org>
@nachomazzara
feat: upgrade ropsten
6ca9232
@nachomazzara nachomazzara mentioned this pull request Apr 22, 2019
Merged
abarmat
abarmat previously approved these changes Apr 22, 2019
View reviewed changes
Copy link
Member

@abarmat abarmat left a comment

Choose a reason for hiding this comment

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

Looks good

eordano
eordano reviewed Apr 22, 2019
View reviewed changes
contracts/estate/EstateRegistry.sol Outdated
@@ -39,6 +39,15 @@ contract EstateRegistry is Migratable, IEstateRegistry, ERC721Token, ERC721Recei
_;
}

modifier onlyManager(uint256 estateId) {
Copy link
Member

Choose a reason for hiding this comment

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

canManageOperators?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm idk. It involves also to update content. I thought canManage but is super generic

Copy link
Member

Choose a reason for hiding this comment

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

"OnlyManager" can be interpreted as "not the owner, not the operator, only the manager".

Copy link
Member

Choose a reason for hiding this comment

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

canSetUpdateOperator

eordano
eordano previously approved these changes Apr 23, 2019
View reviewed changes
Copy link
Member

@eordano eordano left a comment

Choose a reason for hiding this comment

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

other than the minor nit on "canManage", this is good to go

@nachomazzara
Copy link
Collaborator Author

@eordano ready to merge

@eordano
Copy link
Member

eordano commented Apr 23, 2019

circleci está de vacas

@eordano eordano merged commit 210ef0a into master Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eordano eordano eordano approved these changes

@abarmat abarmat abarmat left review comments

@nicosantangelo nicosantangelo Awaiting requested review from nicosantangelo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nachomazzara @eordano @abarmat