refactor: use block.timestamp instead of block.number #46
Conversation
|
Tried bumping husky to fix a weird error I'm getting in |
xorsal
force-pushed
the
feat/rollback-to-timestamps
branch
from
fc4e298
to
73f2a6f
Compare
solidity/contracts/Oracle.sol
Outdated
| responseCreatedAt[_responseId] = uint128(block.number); | ||
| responseCreatedAt[_responseId] = uint128(block.timestamp); | ||
|
|
||
| emit ResponseProposed(_requestId, _responseId, _response, block.number); |
There was a problem hiding this comment.
It should be questioned whether emitting the block.number or block.timestamp is useful anymore.
Also, does it make sense to cast to uint128?
There was a problem hiding this comment.
It should be questioned whether emitting the block.number or block.timestamp is useful anymore.
I agree. Initially I thought block.number was unnecessary and changing it to emit block.timestamp was better, but in truth, both values can be easily obtained. I wonder if it changes anything for the offchain team.
edit: 0xYaco confirmed they don't use this parameter, and they can even take it from the event metadata.
Also, does it make sense to cast to uint128?
This one seems altogether unnecessary. I couldn't find a specific use for it in any of the four mappings using uint128 block.numbers (now timestamps as per this PR).
edit: timestamp could be packed into uint40 (max 34865 years) or uint96 for (2.51 quintillion years), but I don't think it's worthy.
block number and timestamp can be easily obtained from the event's metadata.
Remove unnecessary uint128 casting
xorsal
force-pushed
the
feat/rollback-to-timestamps
branch
from
d1186a0
to
ca89660
Compare
🤖 Linear
We probably need to update events as well.
Closes OPO-647