Uint and Time helpers #375
Conversation
|
|
contracts/common/Initializable.sol
Outdated
| * Using a function rather than `block.number` allows us to easily mock the block number in | ||
| * tests. | ||
| */ | ||
| function getBlockNumber64() internal view returns (uint64) { |
There was a problem hiding this comment.
I think it'd be nice if we moved these time helpers into their own contract file, e.g. TimeHelpers.
contracts/lib/misc/Uint64Helpers.sol
Outdated
| pragma solidity ^0.4.18; | ||
|
|
||
|
|
||
| library Uint64Helpers { |
There was a problem hiding this comment.
I'd call this Uint256Helpers, since it's working on uint256s.
test/lib_uint64_helpers.js
Outdated
| it('fails converting from uint256 to uint64 if too big', async () => { | ||
| const a = new web3.BigNumber(2).pow(64) | ||
| return assertRevert(async () => { | ||
| assert.equal((await uint64Mock.convert(a)).toString(), a, "Values should match") |
There was a problem hiding this comment.
Small nitpick: this can just be await uint64Mock.convert(a) rather than an assert() test.
There was a problem hiding this comment.
Haha, yes, problems of copy-pasting...
Address PR #375 comments.
|
Lacking coverage will be fixed with #372. |
contracts/lib/misc/TimeHelpers.sol
Outdated
| pragma solidity ^0.4.18; | ||
|
|
||
|
|
||
| contract TimeHelpers { |
There was a problem hiding this comment.
Last thing 😅, I'd move this to contracts/common since it's not imported / copied from somewhere else (it's similar to IsContract, which also lives there).
Address PR #375 comments.
contracts/common/TimeHelpers.sol
Outdated
| * tests. | ||
| */ | ||
| function getBlockNumber64() internal view returns (uint64) { | ||
| return uint64(block.number); |
There was a problem hiding this comment.
On these conversions, can we use the new Uint256Helpers?
There was a problem hiding this comment.
Yes, we can, and I thought about it, but I considered unnecessary to import another library as I thought if block number or timestamp were ever bigger than 2^64 (~1.8e19, for time almost 6e11 years) this would be a minor problem. ;-)
There was a problem hiding this comment.
Despite being improbable, I think it's best to use the safe conversion. We use SafeMath when incrementing a lot of these time-based variables in the apps so we should apply the same rigour here.
There was a problem hiding this comment.
Ok, I'll change it, although I kind of disagree. I don't remember by heart all that occurrences, but I think they mostly involve "external" data (parameters, computed, etc), while here we are just converting "system" values (block.number and now) so I think it's pointless.
contracts/common/TimeHelpers.sol
Outdated
| * tests. | ||
| */ | ||
| function getBlockNumber64() internal view returns (uint64) { | ||
| return block.number.toUint64(); |
There was a problem hiding this comment.
If we do getBlockNumber().toUint64() it is easier to mock, as you only have to mock getBlockNumber() and the result of both getBlockNumber() and getBlockNumber64() will be the same.
contracts/common/TimeHelpers.sol
Outdated
| * tests. | ||
| */ | ||
| function getTimestamp64() internal view returns (uint64) { | ||
| return now.toUint64(); |
| @@ -0,0 +1,11 @@ | |||
| pragma solidity ^0.4.18; | |||
There was a problem hiding this comment.
Shouldn't this file be in the common directory as well?
Address PR 375 comments.
test/time_helpers.js
Outdated
| }) | ||
|
|
||
| it('checks block number', async () => { | ||
| assert.equal((await timeHelpersMock.getBlockNumberExt.call()).toString(), (await timeHelpersMock.getBlockNumber64Ext.call()).toString(), "block number should match") |
There was a problem hiding this comment.
We should also test that the returned value is the actual timestamp or block number.
Address PR 375 comments.
Closes #361
TODO: