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

Added uint256 type as valid type for price and sku #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Alexpinilla
Copy link

uint is an alias for uint256, and many people use uint256 instead the alias because they prefer uint256 for some cases or for readability.

OpenZeppelin uses uint256 instead of uint:

Example when uint256 could be better:

  1. bytes4(keccak('transfer(address, uint)'))
    you'll get a different method sig ID than
  2. bytes4(keccak('transfer(address, uint256)'))

I thing that an smart contract only understands the second (uint256) case if it is comparing meth. signature Ids

both, uint and uint256 are valids, so I thing both of types should be valid in the test cases

uint is an alias for uint256, and many people use uint256 instead the alias because they prefer uint256 for some cases or for readability.

OpenZeppelin uses uint256 instead of uint:
- ERC20 OpenZeppelin: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol
- ERC721 OpenZeppelin https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol

Example when uint256 could be better:
1. bytes4(keccak('transfer(address, uint)'))
you'll get a different method sig ID than
2. bytes4(keccak('transfer(address, uint256)'))

I thing that an smart contract only understands the second (uint256) case if it is comparing meth. signature Ids

both, uint and uint256 are valids, so I thing both of types should be valid in the test cases
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.

1 participant