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

[Feature] Make UInt panic on overflow if overflow_checks is enabled #828

Open
u59149403 opened this issue Dec 19, 2024 · 1 comment
Open
Labels
enhancement New feature or request

Comments

@u59149403
Copy link

Component

primitives

Describe the feature you would like

Currently UInt wraps on overflow. Please, make it panic on overflow if cfg(overflow_checks) is enabled, i. e. exactly same thing rustc does with standard numbers. Here is why.

First and foremost, UInt is used mainly for representing monetary values. Wrapping behavior is unnatural for monetary values. So, UInt should panic on overflow. But still some users may want to skip checks for speed reasons. So, we should provide way to configure behavior. The most natural thing to do is to make UInt panic when standard numbers panic and wrap when standard types wrap. So, we should use cfg(overflow_checks).

Of course, this is breaking change, so it should be done in next major version.

I use alloy for managing my personal cryptocurrency, i. e. for transferring it around. Unnoticed overflows may cause loss of money, I absolutely don't want this. So, I plan to create my wrapper around U256, which will panic on overflow. And I also will use clippy::arithmetic_side_effects to make sure that I will not use arithmetic directly on ruint/alloy's U256. But ideally this should be fixed in alloy itself.

Also, currently Solidity throws exception on integer overflow by default ( https://docs.soliditylang.org/en/v0.8.28/types.html#integers ), so currently alloy is less safe than Solidity!

I already reported this problem to ruint ( recmo/uint#408 ), but they rejected my request. So, I propose to create alloy's wrapper, which will panic on overflow (if cfg(overflow_checks)).

If you don't want to replace UInt everywhere, then at least create panicking wrapper and use it in places, where we clearly know that we are dealing with money. For example, Provider::get_balance. This method clearly returns monetary value, there is no any sense for wrapping behavior for this value (but I still suggest checking cfg(overflow_checks) for consistency with standard types)

Additional context

No response

@u59149403 u59149403 added the enhancement New feature or request label Dec 19, 2024
@nadtech-hub
Copy link

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

No branches or pull requests

2 participants