implement block timestamp helper functions #92
Conversation
- add blockTime helper to get a block's timestamp or estimated mining date - implement a boolean helper that allows for different language depending on whether the block in question has been mined or not.
topocount
force-pushed
the
feat/blockTime
branch
from
22cbe9a
to
22b2642
Compare
There was a problem hiding this comment.
Left some notes. Most are fairly mechanical but the ones at the end are important to fix, given that cloud-distributed nodes are a PITA to handle as they're not expected to be globally consistent.
Could you also remind me where you were intending to use these specific strings? Having strings that may change based on time means that we will need to refresh / invalidate caches in places that save rendered radspec strings (e.g. Voting descriptions).
test/examples/examples.js
Outdated
| @@ -359,6 +359,18 @@ const cases = [ | |||
| source: "`_bool ? 'h' + _var + 'o' : 'bye'`", | |||
| bindings: { _bool: bool(true), _var: string('ell') } | |||
| }, 'hello'], | |||
| [{ | |||
There was a problem hiding this comment.
Can we put these tests in the helperCases (around l120 in this file).
test/examples/examples.js
Outdated
| @@ -359,6 +359,18 @@ const cases = [ | |||
| source: "`_bool ? 'h' + _var + 'o' : 'bye'`", | |||
| bindings: { _bool: bool(true), _var: string('ell') } | |||
| }, 'hello'], | |||
| [{ | |||
| source: 'get a past date: `@blockTime(block, showBlock)`', | |||
There was a problem hiding this comment.
| source: 'get a past date: `@blockTime(block, showBlock)`', | |
| source: 'Get a past date: `@blockTime(block, showBlock)`', |
test/examples/examples.js
Outdated
| @@ -359,6 +359,18 @@ const cases = [ | |||
| source: "`_bool ? 'h' + _var + 'o' : 'bye'`", | |||
| bindings: { _bool: bool(true), _var: string('ell') } | |||
| }, 'hello'], | |||
| [{ | |||
| source: 'get a past date: `@blockTime(block, showBlock)`', | |||
| bindings: { block: int('8765432'), showBlock: bool(false) } | |||
There was a problem hiding this comment.
Might as well add a test for when showBlock is true
test/examples/examples.js
Outdated
| //[{ | ||
| // source: 'get a future date: `@blockTime(block)`', | ||
| // bindings: { block: int('20976543') } | ||
| //}, 'get a future date: Wed Mar 19 2025 (block number: 20976543)'], |
There was a problem hiding this comment.
Should this test be uncommented?
test/examples/examples.js
Outdated
| // bindings: { block: int('20976543') } | ||
| //}, 'get a future date: Wed Mar 19 2025 (block number: 20976543)'], | ||
| [{ | ||
| source: 'see if block is mined: `@isBlockMined(block)`', |
There was a problem hiding this comment.
| source: 'see if block is mined: `@isBlockMined(block)`', | |
| source: 'See if block is mined: `@isBlockMined(block)`', |
And might as well add a test for an already mined block.
src/helpers/blockTime.js
Outdated
| } | ||
| } | ||
|
|
||
| export const isBlockMined = (eth) => |
There was a problem hiding this comment.
Let's move this to a different file, since the other helpers are generally 1 per file
src/helpers/blockTime.js
Outdated
| const MILLISECONDS_IN_A_SECOND = 1000 | ||
|
|
||
| export const blockTime = (eth) => | ||
| async (blockNumber, showBlock = true, averageBlockTime = 13.965) => { |
There was a problem hiding this comment.
| async (blockNumber, showBlock = true, averageBlockTime = 13.965) => { | |
| async (blockNumber, showBlock = true, averageBlockTime = 15) => { |
Given the ice age and hardfork shenanigans, it's pretty hard to come up with a value for this so I'd suggest either expecting 10, 12, or 15s blocks (I forget exactly what the average target is).
src/helpers/blockTime.js
Outdated
| export const blockTime = (eth) => | ||
| async (blockNumber, showBlock = true, averageBlockTime = 13.965) => { | ||
| let timestamp | ||
| const currentBlock = await eth.getBlockNumber() |
There was a problem hiding this comment.
Rather than doing this eth_blockNumber, I would suggest just doing a eth_getBlockByNumber here, since that will contain both the block number and timestamp needed for later computation.
src/helpers/blockTime.js
Outdated
| const currentBlock = await eth.getBlockNumber() | ||
|
|
||
| if (currentBlock >= blockNumber) { | ||
| timestamp = (await eth.getBlock(blockNumber)).timestamp * MILLISECONDS_IN_A_SECOND |
There was a problem hiding this comment.
Note that when using some node endpoints that are served over a global cloud network (e.g. Infura), using eth.getBlockNumber() and eth.getBlock() together may not work as expected. You may get a higher latest block number from eth.getBlockNumber() than what is retrievable from eth.getBlock() (eth.getBlock() will return {}).
I would recommend just using a eth.getBlock('latest') above and going off of that. The 'latest' is accepted in the JSONRPC spec and should guarantee that we at least get a close-to-latest block from the network.
src/helpers/blockTime.js
Outdated
|
|
||
| return { | ||
| type: 'string', | ||
| value: `${new Date(timestamp).toDateString()}${showBlock ? ` (block number: ${blockNumber})` : ''}` |
There was a problem hiding this comment.
We already use date-fns in this library (see helpers/transformTime.js) and we should:
- Provide a default format
- Allow users to select their own format
Moreover, for calculating future time, we may want to add an indication that the time is an estimate.
- remove any calls to `eth_blockNumber` - add date-fns format helper and expose formatting externally - move `isBlockMined` to a separate file - re-alphabetize import and exports in index.js - utilize regex tests for the future blocktime test
Thanks for the thorough review Brett! We see these being helpful for use with rewards radspec strings in terms of providing helpful details for votes on rewards, mostly. We also more generally see these helpers allowing for more apps to be built around logic that relies on future block numbers, as opposed to timestamps, ie more token distribution use-cases where block timestamps aren't as useful. |
date
depending on whether the block in question has been mined or not.