Feature/etcm 354/forkid #1018
Feature/etcm 354/forkid #1018
Conversation
d6daa34 to
06a87e1
Compare
|
were the geth test of any use? https://github.com/etclabscore/core-geth/blob/master/core/forkid/forkid_test.go#L132-L169 |
|
|
||
| def gatherForks(config: BlockchainConfig): List[BigInt] = | ||
| (config.daoForkConfig.map(_.forkBlockNumber).toList ++ | ||
| config.forkBlockNumbers.productIterator.toList.flatMap { |
There was a problem hiding this comment.
this assumes the order of fields in config case classes, right?
There was a problem hiding this comment.
The list is being sorted at the very end
There was a problem hiding this comment.
I would put that part with the productIterator inside ForkBlockNumbers directly. Even though it seem reasonable that every BigInt there will be a fork block number we never really know. If we put this function inside ForkBlockNumbers we make this assumption more explicit for devs modifying the case class.
If we want to avoid any logic in the config part, we could add a comment in ForkBlockNumbers which explain that every BigInt there must be a fork block number.
What do you think ?
e74e555 to
6b2d9d9
Compare
| } | ||
| case default => None | ||
| }) | ||
| .filterNot(v => v == 0 || v == noFork) |
There was a problem hiding this comment.
I don't get why we filter out noFork. It does not seem to be used somewhere else
There was a problem hiding this comment.
The same configuration structure BlockchainConfig is re-used for multiple chains. Forks which shouldn't occur in a given chain are configured to happen at block height "1000000000000000000" which is equivalent to "never". We don't want those fork numbers on our fork id list
|
|
||
| def gatherForks(config: BlockchainConfig): List[BigInt] = | ||
| (config.daoForkConfig.map(_.forkBlockNumber).toList ++ | ||
| config.forkBlockNumbers.productIterator.toList.flatMap { |
There was a problem hiding this comment.
I would put that part with the productIterator inside ForkBlockNumbers directly. Even though it seem reasonable that every BigInt there will be a fork block number we never really know. If we put this function inside ForkBlockNumbers we make this assumption more explicit for devs modifying the case class.
If we want to avoid any logic in the config part, we could add a comment in ForkBlockNumbers which explain that every BigInt there must be a fork block number.
What do you think ?
Implement gatherForks
8b6bc37 to
017780c
Compare
Description
First part of https://eips.ethereum.org/EIPS/eip-2124
Adds the ForkId calculation and it's RLP encoding