Common: add BN support #1150
Common: add BN support #1150
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
@ryanio oh sorry, not sure if you are still working on this here, just updated the branch (by merge), if this conflicts with anything you do locally, just force-push. |
|
@holgerd77 no problem, thanks, yes just updating some of the dependent packages now based on these new changes, will follow as additional commits |
ryanio
force-pushed
the
common-bn-support
branch
from
6084a47 to
7d588ee
Compare
ryanio
force-pushed
the
common-bn-support
branch
from
7d588ee to
aa17707
Compare
| @@ -188,17 +191,17 @@ export default class Common extends EventEmitter { | |||
| * @param blockNumber | |||
| * @returns The name of the HF | |||
| */ | |||
| getHardforkByBlockNumber(blockNumber: number): string { | |||
| getHardforkByBlockNumber(blockNumber: BNLike): string { | |||
| blockNumber = toType(blockNumber, TypeOutput.BN) | |||
There was a problem hiding this comment.
Oh, yeah, that plays out really nice here. 😄
| ): boolean { | ||
| opts = opts !== undefined ? opts : {} | ||
| const onlyActive = opts.onlyActive === undefined ? false : opts.onlyActive | ||
| hardfork1 = this._chooseHardfork(hardfork1, opts.onlySupported) |
There was a problem hiding this comment.
Side note, just because I am seeing it here: we might want to consider to remove onlySupported and/or onlyActive HF options on the next breaking release - this is not really used (maybe in the VM, but I would think this is rather not necessary to be managed in Common) - have added a reminder note to v6 Release planning.
| if (this._unsignedTxImplementsEIP155()) { | ||
| v += this.common.chainId() * 2 + 8 | ||
| vBN.iadd(this.common.chainIdBN().muln(2).addn(8)) |
| @@ -136,7 +136,7 @@ export class EthProtocol extends Protocol { | |||
| encodeStatus(): any { | |||
| // TODO: add latestBlock for more precise ETH/64 forkhash switch | |||
| return { | |||
| networkId: this.chain.networkId, | |||
| networkId: this.chain.networkId.toNumber(), | |||
There was a problem hiding this comment.
This change is currently breaking the client CLI start on Yolo v3.
There was a problem hiding this comment.
Just checked, for RLPx the networkId can actually be removed completely from here, since the devp2p library is not using this passed in networkId any more but using the networkId from Common directly.
Not sure about libp2p though.
This PR adds big number support to Common by updating functions with
numberinput types toBNLike.See this comment for the full list of functions changed.
For functions containing
numberoutputs, alternative methods have been added (e.g.chainId->chainIdBN) and the original methods have been deprecated with suggestion to use the BN alternatives for large number support.Closes #914