Integer overflow fixed in formatters

[nivida]

Description

Fixes #3227 & #1905

Affected Methods:

• getBlockNumber: web3-eth
• getPastLogs: web3-eth
• isSyncing: web3-eth
• sendTransaction: web3-eth
• getBlock: web3-eth
• getUncle: web3-eth
• getTransaction: web3-eth
• getTransactionFromBlock: web3-eth
• getTransactionReceipt: web3-eth
• estimateGas: web3-eth
• getPastLogs: web3-eth
• getTransactionCount: web3-eth
• subscribe('newBlockHeaders'): web3-eth
• subscribe('logs'): web3-eth
• subscribe('syncing'): web3-eth
• contract-events: web3-eth-contract
• contract-methods: web3-eth-contract
• getPastEvents: web3-eth-contract

Affected Interfaces

• Transaction
• TransactionReceipt
• EventLog
• Log
• Syncing
• BlockHeader
• BlockTransactionBase
• Block
• BlockTransactionObject
• BlockTransactionString

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• I have selected the correct base branch.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran npm run test with success and extended the tests if necessary.
• I ran npm run build and tested the resulting file from dist folder in a browser.
• I have tested my code on the live network.

[coveralls]

Coverage decreased (-0.2%) to 84.308% when pulling 7db55b9 on issue/number-overflows into 73ef7a9 on 1.x.

[nivida]

@cgewecke As mentioned in issue #3227 are the max. values defined in EIP-1985 bigger than the Number.MAX_SAFE_INTEGER of JS.

That’s why I think adding a numbers options property is something we definitely should do. Do you agree?

Example:

{
  numbers: 'default' | 'string' | 'BN' | Function
}

I've added Function as a possible type because it could be interesting to handle this more dynamically (another BigInt lib as BN, BigInt feature detection, etc.).

[cgewecke]

@nivida I kind of like the interface proposed in this comment at 3227 and think extend, is a good pattern for this case as well.

To me it seems like there are many different clients who are either returning large values for a specific field (like timestamp) or not. And the example of jpmorganchase/quorum.js suggests that fork developers have tended to map outputs to big number on a field-by-field basis.

On the client side, EIP-1985 is less important than it seems because

• Ethereum clients are not returning these values (no change is necessary, or desire-able)
• Non-ethereum clients are not required to respect the EIPs (any field could be different & and users could have any set of expectations)

My instinct is to look at this question (on 1.x anyway) more from a practical standpoint and worry less about conforming to the clients' formal specification.

@nivida What is you opinion of providing the ability to specify the format of number outputs as a separate extension lib published to the web3-js org?

[nivida]

I kind of like the interface proposed in this comment at 3227 and think extend, is a good pattern for this case as well.

Yes, the extend method is great it does give our consumers the opportunity to customize such stuff in their own way. I'm not a big fan of the interface proposed in the mentioned issue. This because all option properties we have currently are defined as simple properties and only the setProvider function does exist which also does have a bit more logic behind as only to set a value from false to true. I would provide such a configuration possibility the same way as all other configuration properties we have and we see the provider as "golden exception". :)

@nivida What is you opinion of providing the ability to specify the format of number outputs as a separate extension lib published to the web3-js org?

I think this will confuse our customers more than necessary. I would add such a configuration property with four options or we close this PR and do provide this possibility in a clean way for 2.x

have tended to map outputs to big number on a field-by-field basis.

Yes, this is something we wouldn’t provide. The idea I had was to return all numbers on all places as string, BN, the current mixed types, or whatever the coerce function is returning. This would be a global setting applied to all numbers to improve the current situation for a lot of custom Ethereum chains.

After all:
Do you agree to set this on-hold for now and to put our focus on bugs and features with a higher priority?

[cgewecke]

I think this will confuse our customers more than necessary. I would add such a configuration property with four options or we close this PR and do provide this possibility in a clean way for 2.x

Yeah, I don't think an extension lib is ideal either. And it seems like people will want it to be configurable?

Do you agree to set this on-hold for now and to put our focus on bugs and features with a higher priority?

Yes, absolutely! We can always come back to this and there are several good solutions...

[nivida]

I've closed this PR now and we can later re-open it again if required.