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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI: avoid coercing number arguments from strings to JS numbers #687

Merged
merged 1 commit into from Aug 6, 2019

Conversation

@sohkai
Copy link
Member

commented Aug 6, 2019

馃 Pull Request

dao act 0x510846E270759CC92131D3453c58e1073C191cD1 0xec67005c4e498ec7f55e092bd1d35cbc47c91892 "transfer(address,uint256)" 0x0d580ae50B58fe08514dEAB4e38c0DFdB0D30adC 1000000000000000000 --environment aragon:mainnet

Results in an

invalid number value (arg="", coderType="uint256", value=1000000000000000000)`

Error produced from web3-eth-abi.

馃毃 Test instructions

Run

dao act 0x510846E270759CC92131D3453c58e1073C191cD1 0xec67005c4e498ec7f55e092bd1d35cbc47c91892 "transfer(address,uint256)" 0x0d580ae50B58fe08514dEAB4e38c0DFdB0D30adC 1000000000000000000 --environment aragon:mainnet

鉁旓笍 PR Todo

  • Include links to related issues/PRs
  • Update unit tests for this change
  • Update the relevant documentation
  • Clear dependencies on other modules that have to be released before merging this

Note that web3-eth-abi does not like much else than a string; giving it a BN.js object or etc. does not work :(.

In general though, unless we need to do computation to a number, it's often best to keep it around as a string since Ethereum numbers are generally large.

@sohkai sohkai requested a review from 0xGabi Aug 6, 2019

@sohkai sohkai requested a review from 0x6431346e as a code owner Aug 6, 2019

@0x6431346e

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

I can see that:

dao act 0x510846E270759CC92131D3453c58e1073C191cD1 0xec67005c4e498ec7f55e092bd1d35cbc47c91892 "transfer(address,uint256)" 0x0d580ae50B58fe08514dEAB4e38c0DFdB0D30adC 9007199254740991 --environment aragon:mainnet --debug

does not throw an error, so perhaps we can do a check:

const number = Number(target)

if (isNaN(number) || number > Number.MAX_SAFE_INTEGER) {
  throw new Error(`Cannot parse ${target} as number`)
}

I imagine this utility could be useful in the future (outside the context of web3), but mainly I think parsing only some, and not all data types is not very intuitive. 馃 What do you think?

@sohkai

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

Mainly I think parsing only some, and not all data types is not very intuitive

I agree with this, but only parsing up to the the max safe int is also not very intuitive and you really have no way to know how to handle its output unless you do the same check before, or do a typeof check afterwards.

Perhaps the best strategy would be to parse into a BN.js object, or similar, and write another "coerceIntoWeb3" type of function? That way we separate parsing string arguments from coercing them into web3.

@0xGabi 0xGabi merged commit 695c5cc into master Aug 6, 2019

10 checks passed

build build
Details
install install
Details
lint lint
Details
test test
Details
test:e2e test:e2e
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.1%) to 4.646%
Details
license/cla Contributor License Agreement is signed.
Details

@delete-merged-branch delete-merged-branch bot deleted the fix-number-arg-coercion branch Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.