Skip to content

Remove unchecked arithmetic #1831

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.

Sign up for GitHub

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

Merged
athei merged 14 commits into from
Aug 28, 2023
Merged

Remove unchecked arithmetic #1831

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5ad9d0e
Remove unchecked arithmetic
athei Jul 1, 2023
9c4e59f
Remove from erc20
athei Jul 2, 2023
8ba8725
Remove from erc1155
athei Jul 2, 2023
9627c86
Remove from erc721
athei Jul 2, 2023
41bc133
Remove from incrementer
athei Jul 2, 2023
0b19225
Remove from payment-channel
athei Jul 2, 2023
526fdfd
Remove from trait-erc20
athei Jul 2, 2023
bea89ad
Remove from trait-incrementer
athei Jul 2, 2023
24da566
Remove from multi-contract-caller
athei Jul 2, 2023
ab9e040
Remove from multisig
athei Jul 2, 2023
b703b49
Remove from set-code-hash
athei Jul 2, 2023
c8e9875
cargo fmt
athei Jul 2, 2023
8675b2e
Merge branch 'master' into at/remove-unchecked-arithmetic
athei Aug 21, 2023
28175ea
Spellcheck
athei Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove from erc1155
  • Loading branch information
@athei
athei committed Jul 2, 2023
commit 8ba8725e2ca2f6c1eaac8ad56b0612cb3bb84e4c
13 changes: 10 additions & 3 deletions integration-tests/erc1155/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,10 @@ mod erc1155 {

// Given that TokenId is a `u128` the likelihood of this overflowing is pretty
// slim.
self.token_id_nonce += 1;
#[allow(clippy::arithmetic_side_effects)]
Copy link
Contributor

@SkymanOne SkymanOne Jul 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fun of a this. Is there a reason we can't do checked arithmetic operation here? If the operation is not implemented for the type, then we need to implement it first before introducing the change here and in cargo contract.

Otherwise we are breaking the arithmetic safety guarantees.

Copy link
Contributor Author

@athei athei Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can. But it is not necessary here. Overflowing a u128 is impossible by incrementing by 1 per extrinsic.

Copy link
Contributor

@SkymanOne SkymanOne Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that we won't be able to use arithmetic operators (i.e. +, -, /, *) on numbers at all? I am just thinking of what impact it will have on the developer experience if we have to tell everyone knows that they need to use checked_ operations specifically.

Copy link
Contributor Author

@athei athei Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that we won't be able to use arithmetic operators (i.e. +, -, /, *) on numbers at all?

Correct.

I am just thinking of what impact it will have on the developer experience if we have to tell everyone knows that they need to use checked_ operations specifically.

Or saturating or wrapping. Developers should express the overflow behaviour in-code instead of leaving it open to a compile time switch. We should have done that from the beginning. People have to think about this anyways.

Copy link

@koute koute Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or saturating or wrapping.

Or use Wrapping and Saturating, so then the arithmetic itself wouldn't become an unreadable soup of function calls, e.g. this compiles:

#![deny(clippy::arithmetic_side_effects)]
use core::num::Wrapping;

fn foo(a: Wrapping<u32>, b: Wrapping<u32>) -> Wrapping<u32> {
    a + b
}

(Saturating is still unstable though, but we could just provide our own wrapper for the time being.)

{
self.token_id_nonce += 1;
}
self.balances.insert((caller, self.token_id_nonce), &value);

// Emit transfer event but with mint semantics
Expand Down Expand Up @@ -329,11 +332,15 @@ mod erc1155 {
.balances
.get((from, token_id))
.expect("Caller should have ensured that `from` holds `token_id`.");
sender_balance -= value;
// checks that sender_balance >= value were performed by caller
#[allow(clippy::arithmetic_side_effects)]
{
sender_balance -= value;
}
self.balances.insert((from, token_id), &sender_balance);

let mut recipient_balance = self.balances.get((to, token_id)).unwrap_or(0);
recipient_balance += value;
recipient_balance = recipient_balance.checked_add(value).unwrap();
self.balances.insert((to, token_id), &recipient_balance);

let caller = self.env().caller();
Expand Down