Skip to content

Improve encapsulation on ERC165 and update code style guide#1379

Merged
nventuro merged 3 commits into
OpenZeppelin:masterfrom
ZumZoom:fix/internal-underscore
Oct 12, 2018
Merged

Improve encapsulation on ERC165 and update code style guide#1379
nventuro merged 3 commits into
OpenZeppelin:masterfrom
ZumZoom:fix/internal-underscore

Conversation

@ZumZoom

@ZumZoom ZumZoom commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

Following #1282 the same prefix underscoring rules should apply to the internal state variables as to the internal functions. Luckily it is already applied in most cases.

@nventuro nventuro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @ZumZoom, thanks for bringing this up! However, I think what's really going on here is that _supportedInterfaces should be private, not internal, and it should be accessed via supportsInterface and _registerInterface. @ElOpio WDYT?

@nventuro nventuro requested a review from come-maiz October 4, 2018 09:24
@nventuro nventuro self-assigned this Oct 4, 2018
@nventuro nventuro added contracts Smart contract code. breaking change Changes that break backwards compatibility of the public API. labels Oct 4, 2018
@nventuro nventuro added this to the v2.0 milestone Oct 4, 2018
@ZumZoom

ZumZoom commented Oct 4, 2018

Copy link
Copy Markdown
Contributor Author

Then it is also the case for ERC165.sol which has the same implementation.

@come-maiz

Copy link
Copy Markdown

I agree with @nventuro. We are trying to make all our state variables private. However, for ERC165 we would need to ask ourself what would this mean with relation to the ERC specification. If we make this state variable private, are we deviating from the expected interface?

@ZumZoom

ZumZoom commented Oct 6, 2018

Copy link
Copy Markdown
Contributor Author

The only difference that migration from internal to private makes is impossibility to deregister interfaces. Also supportsInterface visibility is external which means child classes would use external call to access it.

From the ERC specification point of view it seems like nothing is changed.

@nventuro

nventuro commented Oct 7, 2018

Copy link
Copy Markdown
Contributor

@ZumZoom you're right, but that is an unintended side effect of the current implementation: if we want to support deregistering, we should provide a method to explicitly do it. I'd go for private for the 2.0 release: we can extend this functionality in the future.

@ZumZoom ZumZoom changed the title Use prefix underscore for internal state variables Improve encapsulation on ERC165 and update code style Oct 7, 2018
@ZumZoom ZumZoom changed the title Improve encapsulation on ERC165 and update code style Improve encapsulation on ERC165 and update code style guide Oct 8, 2018

@nventuro nventuro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for taking care of this @ZumZoom! Little fixes like this one help the whole project be more cohesive :)

@nventuro nventuro merged commit 03dfb29 into OpenZeppelin:master Oct 12, 2018
@ZumZoom ZumZoom deleted the fix/internal-underscore branch October 13, 2018 08:54
come-maiz pushed a commit that referenced this pull request Oct 21, 2018
* use prefix underscore for internal state variables

* make _supportedInterfaces private

(cherry picked from commit 03dfb29)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Changes that break backwards compatibility of the public API. contracts Smart contract code.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants