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

[Code]: Unnecessary conversions from u64 to u64 #156

Closed
0xOutOfGas opened this issue Sep 26, 2022 · 3 comments · Fixed by #191
Closed

[Code]: Unnecessary conversions from u64 to u64 #156

0xOutOfGas opened this issue Sep 26, 2022 · 3 comments · Fixed by #191
Labels
enhancement New feature or request
Milestone

Comments

@0xOutOfGas
Copy link

Framework version: v11
Description: Conversions from u64 to u64 are found in both files SignedInteger64.move and Bitwise.move. It's unnecessary and it consumes gas.
Code Location: sources/SignedInteger64.move, line 19,25,32; sources/Bitwise.move

public fun multiply_u64(num: u64, multiplier: SignedInteger64): SignedInteger64 {
    let product = multiplier.value * num;
    SignedInteger64 { value: (product as u64), is_negative: multiplier.is_negative }
}

/// Divide a u64 integer by a signed integer number.
public fun divide_u64(num: u64, divisor: SignedInteger64): SignedInteger64 {
    let quotient = num / divisor.value;
    SignedInteger64 { value: (quotient as u64), is_negative: divisor.is_negative }
}

/// Sub: `num - minus`
public fun sub_u64(num: u64, minus: SignedInteger64): SignedInteger64 {
    if (minus.is_negative) {
        let result = num + minus.value;
        SignedInteger64 { value: (result as u64), is_negative: false }
    } else {
        if (num > minus.value)  {
            let result = num - minus.value;
            SignedInteger64 { value: (result as u64), is_negative: false }
        }else {
            let result = minus.value - num;
            SignedInteger64 { value: (result as u64), is_negative: true }
        }
    }
}

Recommendation: Remove unnecessary conversions in both files SignedInteger64.move and Bitwise.move.

@0xOutOfGas 0xOutOfGas added the enhancement New feature or request label Sep 26, 2022
@jolestar jolestar added this to the v12 milestone Oct 11, 2022
@JerryKwan
Copy link
Contributor

@jolestar
type cast is necessary, but the problem of the codes is that regarding add (including minus a negative value) and multiply operations it should be use u128 to hold the result first and check if the result exceed the max of u64 and then cast it to u64 and return. Or it may fail with overflow and output something like

Execution failed because of an arithmetic error (i.e., integer overflow/underflow, div/mod by zero, or invalid shift)

here is some sample codes

public fun add_u64(m: u64, n: u64): u64 {
        let sum = (m as u128) + (n as u128);
        debug::print(&sum);
        assert!(sum <= MAX_U64, E_OUT_OF_RANGE);
        (sum as u64)
    }

@pause125
Copy link
Collaborator

@jolestar type cast is necessary, but the problem of the codes is that regarding add (including minus a negative value) and multiply operations it should be use u128 to hold the result first and check if the result exceed the max of u64 and then cast it to u64 and return. Or it may fail with overflow and output something like

Execution failed because of an arithmetic error (i.e., integer overflow/underflow, div/mod by zero, or invalid shift)

here is some sample codes

public fun add_u64(m: u64, n: u64): u64 {
        let sum = (m as u128) + (n as u128);
        debug::print(&sum);
        assert!(sum <= MAX_U64, E_OUT_OF_RANGE);
        (sum as u64)
    }

May be it's also OK to get the exception error from MoveVM ?

@jolestar
Copy link
Member

May be it's also OK to get the exception error from MoveVM?

yes, Move VM can check the overflow.

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

Successfully merging a pull request may close this issue.

4 participants