Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wallet signer doesn't accept a BigNumber for nonce. #301

Closed
MicahZoltu opened this issue Oct 10, 2018 · 2 comments
Closed

Wallet signer doesn't accept a BigNumber for nonce. #301

MicahZoltu opened this issue Oct 10, 2018 · 2 comments
Assignees
Labels
enhancement New feature or improvement.

Comments

@MicahZoltu
Copy link

The Ethereum protocol doesn't have a concept of small numbers vs big numbers, in its eyes everything is a big number, even if there is no reasonable expectation for a number to grow past 2^52. An example of this is the nonce, which is derived from https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_gettransactioncount which returns a QUANTITY.

While I think this is stupid, ethers currently is mostly consistent about accepting BigNumber for any numeric parameter. The one exception I have found is in TransactionRequest when it asks for a nonce https://github.com/ethers-io/ethers.js/blob/master/src.ts/providers/abstract-provider.ts#L81.

For consistency, I recommend making this accept a BigNumber.

Alternatively, I could see value in doing a large scale refactoring of ethers that does ab etter job than the wire protocol at differentiating between different types, including small numbers, big numbers, addresses, hashes, byte arrays, etc. I have tackled this previously and it is a lot of work but I personally found it really paid off in making my code easier to maintain by giving me much more robust type checking (no longer accidentally passing an address to something that wanted a hash, or passing a hash to something that wanted a big number, or passing a big number to something that wanted a small number). This would be a very large undertaking, so for now I would be fine with the library simply being consistent and accepting BigNumber everywhere.

@ricmoo
Copy link
Member

ricmoo commented Oct 10, 2018

Ethers will already, for the most part, perform run-time checks on most parameters passed in, but you are correct. It makes sense to allow BigNumber as a parameter in a request. It will ensure it is a valid ieee 764 compact integer, but should be allowed.

@ricmoo ricmoo self-assigned this Oct 10, 2018
@ricmoo ricmoo added enhancement New feature or improvement. on-deck This Enhancement or Bug is currently being worked on. labels Oct 10, 2018
@ricmoo
Copy link
Member

ricmoo commented Dec 14, 2018

This should work now in 4.0.19.

Thanks! :)

@ricmoo ricmoo closed this as completed Dec 14, 2018
@ricmoo ricmoo removed the on-deck This Enhancement or Bug is currently being worked on. label Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement.
Projects
None yet
Development

No branches or pull requests

2 participants