ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.48k stars 5.81k forks source link

Remove `uint` as alias for `uint256` #14026

Open pcaversaccio opened 1 year ago

pcaversaccio commented 1 year ago

Abstract

I would like to propose removing uint as an alias for uint256 for the upcoming breaking >=0.9.0 versions. The reason is that global aliases in the area of user-defined types and operators decrease readability.

Motivation

Consider the following example:

// SPDX-License-Identifier: WTFPL
pragma solidity 0.8.19;

type Uint is uint8; /// @dev Use `Uint` for `uint8` type. 
using {safeAddUint256 as +} for Uint global;

/// @dev The function name is chosen on purpose like that to increase the confusion.
function unsafeAddUint256(Uint a, Uint b) pure returns (Uint) {
    unchecked {
        return Uint.wrap(Uint.unwrap(a) + Uint.unwrap(b));
    }
}

contract evil {
    function add(Uint a, Uint b) external pure returns (Uint) {
        return a + b;
    }
}

The example is on purpose like that to indicate that uint and Uint can mean completely different things and I feel that for the reason of auditability and readability, we should discourage the use of uint as an alias for uint256. Explicitness of the type range is a good language feature to have.

[EDIT]: This example was on purpose rather provocative and please take a look at the examples of @Amxx here for more spot-on examples.

Specification

I would like to propose the following specification:

Backwards Compatibility

This change is not backward-compatible with <0.9.0 versions.

Amxx commented 1 year ago

While I do personally agree that uint as an alias to uint256 creates more confusion than good, I'm not sure your example is really about that. I think you are pointing out the danger of terribly named UDVT.

You would have the same issue with

type Uint256 is int256;
type Int256 is uint256;

or any other stupid thing like that.


One good reason to deprecate uint is that some people thing its a type of its own, and use it (instead of uint256) when trying to compute function selector or EIP-712 typehash. I can't recall all the times users came to me with difficulty to get a signature recognized because they did

struct MyStruct {
    address a;
    uint b;
}
bytes32 constant MYSTRUCT_TYPE_HASH = keccak256("MyStruct(address a,uint b)");

When I told them to do keccak256("MyStruct(address a,uint256 b)") instead, they often insisted that I was stupid for not seeing that their struct uses a uint and not uint256.


I've also seen developers say that their code did not have overflow issues because they replaced uint256 with uint, and that as a consequence the 256 bits limit no longer applies.

pcaversaccio commented 1 year ago

Yes, I agree with you @Amxx - the examples you provide are much more spot-on. Let me give another example I've seen among developers. Some people assumed that uint is an alias for the lowest possible conversion type:

function fn(uint8 x) external {
    uint scale = x; /// Developer assumes it converts automatically to type `uint8` for `scale`.
    ....
}

I even had people assuming that uint is an alias for uint64 (I don't know the reasons tbh, and here I could argue people should read more carefully the documentation). Another motivation I want to share is cross-language consistency. In Vyper, for example, such an alias is not existent.

PS: Tbh my issue example was on purpose a little bit provocative wrt UDVT in order to highlight what can go wrong overall.

Amxx commented 1 year ago

I even had people assuming that uint is an alias for uint64

That tipically sounds like some C/C++ héritage. Int and uint are usually 32 bits, and long often 64 bits (even though that is not guaranteed, some people prefer long long to be sure, even though that is not garanteed to be 64 bits either). When I wrote c++ (10 years ago) I used to rely on uint32_t and uint64_t, but I was in a minority of devs that use these types.

pcaversaccio commented 1 year ago

I would like to add another (honeypot) example where the uint alias creates an issue:

contract B {
    function claim(uint amount) external payable {
        payable(msg.sender).transfer(amount * 2);
    }
    fallback() payable external {}
}

contract C {
    function claim(address b) external payable {
        uint amount = msg.value;
        require(amount > 0, "Zero deposit");
        // The following line will call the `fallback` function in `B`.
        (bool success, ) = b.call{value: amount}(abi.encodeWithSignature("claim(uint)", amount));
        require(success, "Claim failed");
        payable(msg.sender).transfer(address(this).balance);
    }
    receive() payable external {}
}

There are multiple issues with the example contracts above and it's more of an illustration of why the alias can create confusion. The issue with the above contract that I want to point out is that we have B.claim.selector != bytes4(keccak256(bytes("claim(uint)"))) and thus it will call the fallback function in the contract B.

github-actions[bot] commented 1 year ago

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

pcaversaccio commented 1 year ago

This issue is still relevant.

github-actions[bot] commented 1 year ago

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

pcaversaccio commented 1 year ago

Don't close as this issue is still relevant.

pcaversaccio commented 1 year ago

Any updates on the roadmap for this security feature @cameel?

nikola-matic commented 1 year ago

@pcaversaccio @Amxx I'm not personally gonna close this issue, since I'd prefer for @cameel and/or @ekpyron to get the blame instead of me :) The reason it'll most likely be closed is that we're currently reworking our type system (or rather writing a new one), which will have full on generics and type deduction; in addition, we're likely gonna implement all of the types in the standard library, whilst having the core language remain as small as possible, such that the standard library will be written in Solidity, which will in turn allow community peer review/contributions (at a larger scale than now), as well as auditing of said standard library. This means that your gripe with the uint256 -> uint alias (valid btw) will be addressed in Solidity 1.0.0. Of course, removing the alias now would mean a breaking release, which would target it for the 0.9 release, and given that we're already putting most of our focus (as far as language design and long term direction is concerned) into the new type system, it's very unlikely that we'll actually introduce this change.

pcaversaccio commented 1 year ago

@pcaversaccio @Amxx I'm not personally gonna close this issue, since I'd prefer for @cameel and/or @ekpyron to get the blame instead of me :) The reason it'll most likely be closed is that we're currently reworking our type system (or rather writing a new one), which will have full on generics and type deduction; in addition, we're likely gonna implement all of the types in the standard library, whilst having the core language remain as small as possible, such that the standard library will be written in Solidity, which will in turn allow community peer review/contributions (at a larger scale than now), as well as auditing of said standard library. This means that your gripe with the uint256 -> uint alias (valid btw) will be addressed in Solidity 1.0.0. Of course, removing the alias now would mean a breaking release, which would target it for the 0.9 release, and given that we're already putting most of our focus (as far as language design and long term direction is concerned) into the new type system, it's very unlikely that we'll actually introduce this change.

Do I understand correctly that there will be no syntactic changes at all before 2025 (link)? image

I.e., there will be no breaking syntax changes in version 0.9?

nikola-matic commented 1 year ago

@pcaversaccio @Amxx I'm not personally gonna close this issue, since I'd prefer for @cameel and/or @ekpyron to get the blame instead of me :) The reason it'll most likely be closed is that we're currently reworking our type system (or rather writing a new one), which will have full on generics and type deduction; in addition, we're likely gonna implement all of the types in the standard library, whilst having the core language remain as small as possible, such that the standard library will be written in Solidity, which will in turn allow community peer review/contributions (at a larger scale than now), as well as auditing of said standard library. This means that your gripe with the uint256 -> uint alias (valid btw) will be addressed in Solidity 1.0.0. Of course, removing the alias now would mean a breaking release, which would target it for the 0.9 release, and given that we're already putting most of our focus (as far as language design and long term direction is concerned) into the new type system, it's very unlikely that we'll actually introduce this change.

Do I understand correctly that there will be no syntactic changes at all before 2025 (link)? image

I.e., there will be no breaking syntax changes in version 0.9?

Nope, there will be - it's just a question of the quantity of such changes - since the focus for 0.9 is via-ir by default. That's why I didn't close the issue, but given that our main priority is via-ir and the new type system, it's unlikely that we'll be focusing on the syntax that much.

cameel commented 1 year ago

It's exactly as @nikola-matic said. This is one of the things that are much better off handed off to the community via the future stdlib process. It will take time and I know that the wait may be frustrating, but once it's in place, it will seriously boost our ability to address these things efficiently. Currently it's a whack-a-mole where, if we spend time to address one problem with syntax sugar, ten new ones crop up and the core roadmap work gets pushed more and more into the unspecified future.

The change itself is small in terms of implementation, so we'll consider it along with other breaking changes for 0.9 but I think that the chance of it getting in is pretty slim. It's quite opinionated and even if we may agree with the opinion, we'd much rather have it handled in a way that gives us more confidence that this is really the opinion of the wider community. We'd rather not rush it just because it's easy to change.

pcaversaccio commented 1 year ago

The change itself is small in terms of implementation, so we'll consider it along with other breaking changes for 0.9 but I think that the chance of it getting in is pretty slim. It's quite opinionated and even if we may agree with the opinion, we'd much rather have it handled in a way that gives us more confidence that this is really the opinion of the wider community. We'd rather not rush it just because it's easy to change.

What do you have in mind to provide you with more confidence? As two critical examples: EIPs or ABI-based function selectors use uint256 and not uint. Generally, having the alias uint for uint256 follows the principle of least astonishment, but there are still surprises where uint can create harm.

cameel commented 1 year ago

I was thinking about the stdlib process. Handing the decision off to the actual users of the language is the ultimate way to be sure that we're giving them what they want.

Also, it's not only about whether the alias itself is problematic, but also about whether deprecating it right now is really the best decision. We have much bigger changes to the type system ahead and doing it now may just create more confusion and upgrade friction than it's worth. In the short term there are still ways to address this without necessarily removing it - for example this could be flagged by linters - so we don't really see this as so urgent that it can't wait for a time where we have more clarity and better tools to handle it.

drortirosh commented 1 year ago

There is a distinction between "binary format" (uint256) and "native type". I suggest to keep the alias, but restrict its usage as storage field (or struct)

native type is what the compiler should handle the best. e.g. for (uint256 i=0,...) and for( uint32 i=0,...) seem equivalent, but AFAIK, the latter is far less optimize from Solidity's point of view, since it uses a non-native type

Czar102 commented 4 months ago

I also view uint as the "native" type for the VM, and I think it's natural for it to exist. uint256 can be considered an alias of this fundamental type, while all other types are implemented by the compiler on top of the fundamental type.

The confusion related to ABI encoding is understandable. ABI encoding differentiates the interpretation of the type, not its actual structure – for example, uint256 and bytes32 are NOT equivalent. It could be an explanation for why is there no "native" type allowed there.

Also, it's not a good experience to type seven characters just to declare the native EVM type. If uint is to be deprecated anyway, it would be great to have another native type introduced, hopefully a one that's shorter. Or, at least, doesn't contain numbers in its name, unlike uint256 and bytes32.