ERC20 totalSupply changed from public property to a function#666
Conversation
|
Heh, PR 666 😈 |
| */ | ||
| 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).
frangio
left a comment
There was a problem hiding this comment.
Not happy with the trailing underscore notation but cannot think of a better alternative.
1972eab to
3e11499
Compare
|
@frangio please re-review :-) |
| */ | ||
| 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).