Solidity 0.5.0 #478
Solidity 0.5.0 #478
Conversation
|
Travis is failing cause openzeppelin haven't updated their official package to solidity 0.5.0. CircleCI's image is debain based instead of ubuntu so it does not have direct access solc binary via ppa. As it's running in docker, it can't use snap either. It needs more config. Solidity coverage is not working with solidity 0.5.0 so it doesn't matter anyway. |
|
The project is building fine now (with a custom version of OZ). It will take time to refactor test cases though. |
| @@ -294,27 +286,27 @@ contract SecurityToken is ERC20, ERC20Detailed, ReentrancyGuard, RegistryUpdater | |||
| * @param _module address of module to unarchive | |||
| */ | |||
| function removeModule(address _module) external onlyOwner { | |||
There was a problem hiding this comment.
Assume this is due to contract size issues?
There was a problem hiding this comment.
Oh yes. I totally forgot about this. I also commented out tests that required removal of modules.
Do you prefer to merge this as it as and reduce the size of ST in future or reduce the size before merging?
There was a problem hiding this comment.
Let's merge and create an issue to uncomment the function as a reminder.
There was a problem hiding this comment.
@maxsam4. In this PR I have a concern related to styling because lot of places we lost redability. (Ex- return, constructor, function declartion statements etc.).
we could also remove the updatePolyTokenAddress() because it can be handled by the Dmitriy changes (updateFromRegistry()). It also helps to reduce the size little bit.
| key = Encoder.getKey("registeredTickers_status", _ticker); | ||
| if (getBool(key) != _status) | ||
| set(key, _status); | ||
| if (getBool(key) != _status) set(key, _status); |
There was a problem hiding this comment.
Is the one statement in if is disallowed by the 0.5?, If not then I believe the earlier was better in reading the understanding.
There was a problem hiding this comment.
This has nothing to do with solidity. It's just style preference. Prettier was originally made for Javascript and JS folks prefer to compress if statements like this. We can revert to original one or keep this change. I am fine with any. I have worked with both styles in the past so they both seem kinda natural to me.
contracts/SecurityTokenRegistry.sol
Outdated
| string calldata _ticker, | ||
| string calldata _tokenDetails, | ||
| bool _divisible | ||
| ) external whenNotPausedOrOwner { |
There was a problem hiding this comment.
I think styling could be improved.
function generateSecurityToken(
string calldata _name,
string calldata _ticker,
string calldata _tokenDetails,
bool _divisible
)
external
whenNotPausedOrOwner
{
}
In other functions as well. It makes the code more redable.
There was a problem hiding this comment.
To be honest, I prefer the changed style. i.e modifiers in a single line. It's a personal choice though and I am fine with any. I just don't think it's worth it to spend time in refactoring this.
| + 367 * (_month - 2 - (_month - 14) / 12 * 12) / 12 | ||
| - 3 * ((_year + 4900 + (_month - 14) / 12) / 100) / 4 | ||
| - OFFSET19700101; | ||
| int __days = _day - 32075 + 1461 * (_year + 4800 + (_month - 14) / 12) / 4 + 367 * (_month - 2 - (_month - 14) / 12 * 12) / 12 - 3 * ((_year + 4900 + (_month - 14) / 12) / 100) / 4 - OFFSET19700101; |
There was a problem hiding this comment.
It is a nightmare to understand. can't we go as it was written earlier
There was a problem hiding this comment.
We aren't even supposed to change this file. Once bokky updates the library for solidity 0.5, we'll just use his :).
|
@satyamakgec The styling changes were done by prettier automatically. We can manually change them to our liking in future but I think, for now, we should just let them be. |
|
hmm... It is not a big task to tackle so we can merge this PR with all the changes. |
Even after using my refactor tool, there are still a lot of manual changes remaining.
For example, now variables are not available outside their block. So all code like double for loops now have to define vars again. A lot of explicit conversions are no longer allowed directly so they need to change as well. etc.