ERC20 totalSupply changed from public property to a function #666
Conversation
|
Heh, PR 666 😈 |
| @@ -22,7 +22,7 @@ contract SimpleToken is StandardToken { | |||
| * @dev Constructor that gives msg.sender all of existing tokens. | |||
| */ | |||
| function SimpleToken() public { | |||
| totalSupply = INITIAL_SUPPLY; | |||
| totalSupply_ = INITIAL_SUPPLY; | |||
There was a problem hiding this comment.
I thought we had said that we were going to use an underscore as a prefix, instead of a suffix, am I right?
There was a problem hiding this comment.
Yeah, hence the last comment on the PR:
Regarding the naming convention for the backing field: since we are using leading underscores for the parameters, I unilaterally decided to use trailing underscore for the backing fields. Other options I considered were using a dollar sign (
$totalSupply or totalSupply$ ), or some variant of Hungarian notation (u256TotalSupply).
spalladino
force-pushed
the
feature/434-totalSupply_as_function
branch
from
1972eab
to
3e11499
Compare
|
@frangio please re-review :-) |
| @@ -7,7 +7,7 @@ pragma solidity ^0.4.18; | |||
| * @dev see https://github.com/ethereum/EIPs/issues/179 | |||
| */ | |||
| contract ERC20Basic { | |||
| uint256 public totalSupply; | |||
| function totalSupply() public view returns (uint256); | |||
There was a problem hiding this comment.
I got a compilation error because of this. It tells the following:
zeppelin-solidity/contracts/token/ERC20/ERC20Basic.sol:10:3: TypeError: Redeclaring an already implemented function as abstract
function totalSupply() public view returns (uint256);
^---------------------------------------------------^
,zeppelin-solidity/contracts/token/ERC20/ERC20Basic.sol:10:3: TypeError: Redeclaring an already implemented function as abstract
function totalSupply() public view returns (uint256);
^---------------------------------------------------^
There was a problem hiding this comment.
Where did that happen @hlogeon? If you can reproduce would you mind opening a new issue please?
totalSupply -> totalSupply_ in JoyToken.sol according to: OpenZeppelin/openzeppelin-contracts#666
totalSupply -> totalSupply_ in JoyToken.sol according to: OpenZeppelin/openzeppelin-contracts#666
npm run lint:fix) and fixed all issues.Fixes #434
🚀 Description
Having
ERC20#totalSupplydefined as a public property made it impossible to override it in any child class, due to a solidity limitation. As such, it was impossible to change the getter behaviour while retaining the ERC20 interface, at least without modifying the OpenZeppelin base contract (which is something we definitely want to avoid).This PR removes the public property definition from the
ERC20Basicinterface, and replaces it with just the signature for the getter. The implementation is provided inBasicToken, the first contract that implements that interface, backed by atotalSupply_internal field.Regarding the naming convention for the backing field: since we are using leading underscores for the parameters, I unilaterally decided to use trailing underscore for the backing fields. Other options I considered were using a dollar sign (
$totalSupplyortotalSupply$), or some variant of Hungarian notation (u256TotalSupply).