Skip to content

accounts/abi/abigen: use golang uint256.Int for solidity uint256 params #31607

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mirokuratczyk
Copy link
Contributor

Currently, abigen generates a big.Int binding in Golang for uint256 Solidity parameters.

The danger is that negative big.Int values are serialized into very large uint256 values due to two’s complement. For example, big.NewInt(-1) is serialized to the maximum uint256 value.

Consider this binding where the type of amount in Solidity is uint256, but the generated binding uses big.Int.

// Solidity: function mint(address to, uint256 amount) returns()
func (_Token *TokenTransactor) Mint(opts *bind.TransactOpts, to common.Address, amount *big.Int) (*types.Transaction, error)

The risk is that, due to programmer error, an accidental call like erc20Token.Mint(..., big.NewInt(-1)) is made. Unexpectedly, the call will not return an error, but instead serializes a transaction where amount=0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff due to

// U256 encodes x as a 256 bit two's complement number. This operation is destructive.

Another similar scenario is demonstrated in this test mirokuratczyk@170d120, which fails with

=== RUN   TestToken
    token_test.go:53: Expect token balance to equal transfer value: 115792089237316195423570985008687907853269984665640564039457584007913129639935 != -1
--- FAIL: TestToken (0.04s)

Generating a uint256.Int binding in Golang for uint256 Solidity parameters ensures proper type safety and makes this unexpected behavior impossible.

@jwasinger
Copy link
Contributor

I would say overall, it makes sense to add this.

@mirokuratczyk
Copy link
Contributor Author

Rebased and force-pushed to include a9444ea, which is required for the AppVeyor tests to pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants