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

Feat: many update operators #146

Merged
merged 5 commits into from Aug 6, 2019

Conversation

@nachomazzara
Copy link
Collaborator

commented Jul 22, 2019

Set many update operators to Estates and LANDs

EstateRegistry:

  • Set many update operators
  • Set many LAND update operators (Note that calls setManyUpdateOperator from LANDRegistry by checking if the LAND is part of the Estate)

LANDRegistry:

  • Set many update operators
uint256[] _estateIds,
address _operator
)
public

This comment has been minimized.

Copy link
@fmiras

fmiras Jul 23, 2019

Member

Wouldn't be better if we put the modifiers here as well?

This comment has been minimized.

Copy link
@nachomazzara

nachomazzara Jul 23, 2019

Author Collaborator

I don't think so. That way we will check the same twice and therefore consume more gas.

@@ -1002,6 +1002,7 @@ contract LANDRegistry {
function decodeTokenId(uint value) external pure returns (int, int);
function updateLandData(int x, int y, string data) external;
function setUpdateOperator(uint256 assetId, address operator) external;
function setManyUpdateOperator(uint256[] landIds, address operator) external;

This comment has been minimized.

Copy link
@fmiras

fmiras Jul 23, 2019

Member

The function is public on implementation but external at the interface, is that ok?

This comment has been minimized.

Copy link
@nachomazzara

nachomazzara Jul 23, 2019

Author Collaborator

Yes, there is no issue with that

@eordano
Copy link
Member

left a comment

Looks good to me. I'd run a spell/grammer checker on the names of the test methods but otherwise 👍

Update contracts/estate/EstateRegistry.sol
Co-Authored-By: Federico Miras <miras.federico@gmail.com>

@nachomazzara nachomazzara dismissed stale reviews from eordano and fmiras via 83400fe Jul 31, 2019

@@ -310,17 +310,38 @@ contract LANDRegistry is Storage, Ownable, FullAssetRegistry, ILANDRegistry {
}
}

/**
* @notice Set estates updateOperator

This comment has been minimized.

Copy link
@abarmat

abarmat Aug 2, 2019

Member

Typo, should be Set LAND updateOperator

canSetUpdateOperator(assetId)
{
updateOperator[assetId] = operator;
emit UpdateOperator(assetId, operator);
}

/**
* @notice Set estates updateOperator

This comment has been minimized.

Copy link
@abarmat

abarmat Aug 2, 2019

Member

Typo, should be Set many LAND updateOperator

@abarmat
abarmat approved these changes Aug 2, 2019
Copy link
Member

left a comment

Good set of tests 👍

@nachomazzara nachomazzara merged commit 4d454f7 into master Aug 6, 2019

1 of 2 checks passed

security/pgp-check Failed to verify signature: Missing key for nacho@decentraland.org or signature is empty, signature:
ci/circleci Your tests passed on CircleCI!
Details

@nachomazzara nachomazzara deleted the feat/many-update-operators branch Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.