checked variants of I256 arithmetic functions

[ozgunozerk]

Quoting from stellar/rs-soroban-sdk#1659:

What problem does your feature solve?

Right now, we don't have checked_sub, checked_mul, etc. for I256 custom type. This hinders the developers to write additional math libraries that may require the checked versions.

In our case, we are writing a fixed_point_math library, and a WAD equivalent for Soroban. However, especially the error-handling part becomes a bit inconsistent due to lack of checked variants.

Here is a code piece:

pub(crate) fn checked_mul_div_floor(env: &Env, x: &I256, y: &I256, z: &I256) -> Option<I256> {
    let zero = I256::from_i32(env, 0);
    let r = x.mul(y);

    // we handle `DivisionByZero` case, and return `None` as expected by the `checked` variant
    if *z == zero {
        return None;
    }

    if (r < zero && *z > zero) || (r > zero && *z < zero) {
        // ceil is taken by default for a negative result

        // if `rem_euclid` fails, this will panic, but it should have returned `None` instead...
        let remainder = r.rem_euclid(z);
        let one = I256::from_i32(env, 1);

        // similarly, if `div` or `sub` fails, this will panic, but it should have returned `None` instead...
        Some(r.div(z).sub(if remainder > zero { &one } else { &zero }))
    } else {
        // floor is taken by default for a positive or zero result
        Some(r.div(z))
    }
}

What would you like to see?

checked variants of all the arithmetic functions

What alternatives are there?

I saw that, behind the scenes, env.i256_{operation} is used. For example:

    pub fn mul(&self, other: &I256) -> I256 {
        let val = self.env.i256_mul(self.val, other.val).unwrap_infallible();
        I256 {
            env: self.env.clone(),
            val,
        }
    }

However, it is quite unintuitive, and even impossible to use the underlying i256_{operation} functions as alternative.

Impossible, due to:

• these functions are private, not public

Unintuitive, due to:

• we have to import i256Val type
• it is returning Result, and we have to map the Result into Option
• it is not named as checked, which creates confusion
• the required argument type I256Val, is reachable only through self.to_val_type(), which adds additional friction

[tupui]

Related to this, I am wondering why we even have non checked versions? By default we should have safe operations and only if explicitly requested we should have non-safe versions. That would be a better DX and follow the general idea of Stellar Smart contracts being a battery included platform with sane and sensible defaults.

[tupui]

A great, thanks for the clarifications 😃

[tupui]

I don't think we have much recommendations in terms of security in the docs. We have this https://developers.stellar.org/docs/learn/migrate/evm/solidity-and-rust-advanced-concepts#writing-safe-and-efficient-rust-code-for-smart-contracts and https://developers.stellar.org/docs/build/smart-contracts/example-contracts/cross-contract-call#contract-a-the-contract-to-be-called but I could not find overflow-checks = true and pros-cons of approaches.

[leighmcculloch]

That's a good call out. The docs you linked to demonstrate using .checked_ variants of the operations, where the code can handle the failed check.

The examples always have the release profile setup so that if you use standard operations they will be checked and panic:
https://developers.stellar.org/docs/build/smart-contracts/getting-started/hello-world#release-profile

But this should be discussed in the safe and efficient sections.

[tupui]

handle when the check fails

On that point, if the goal in the end is to have an Err instead of panic, wouldn't we be better off having a generic way to catch panic like in std? Or maybe that's technically difficult to do and the reason it is not there?

Also, now in the API we have things like try_ and checked_. If really needed, maybe we should only have a single way to prefix functions. I am saying this as someone who is not a native Rust dev so that's confusing to me to have these 2.

[leighmcculloch]

we have things like try_ and checked_

In the Rust libstd:

The checked_ fn pattern is really only used for arithmetic overflow and typically returns Option<> and None on overflow. For example: https://doc.rust-lang.org/std/index.html?search=checked_

The try_ fn pattern usually returns a Result for handling some error that has occurred. For example: https://doc.rust-lang.org/std/index.html?search=try_

[leighmcculloch]

I think @ozgunozerk's proposal at the top would require adding the following host functions, that mirror the existing u/i256_ functions:
https://github.com/stellar/rs-soroban-env/blob/97546e04908701935dcf81d36bb0a69ac761430e/soroban-env-common/env.json#L443-L698

For overflow-safe arithmetic, the new checked host functions would mirror the existing math operations but return Val where the value is either the result or Void (representing None on overflow/error).

Existing I256 Functions:

Function	Parameters	Return
i256_add	lhs: I256Val, rhs: I256Val	I256Val
i256_sub	lhs: I256Val, rhs: I256Val	I256Val
i256_mul	lhs: I256Val, rhs: I256Val	I256Val
i256_div	lhs: I256Val, rhs: I256Val	I256Val
i256_rem_euclid	lhs: I256Val, rhs: I256Val	I256Val
i256_pow	lhs: I256Val, rhs: U32Val	I256Val
i256_shl	lhs: I256Val, rhs: U32Val	I256Val
i256_shr	lhs: I256Val, rhs: U32Val	I256Val

Existing U256 Functions:

Function	Parameters	Return
u256_add	lhs: U256Val, rhs: U256Val	U256Val
u256_sub	lhs: U256Val, rhs: U256Val	U256Val
u256_mul	lhs: U256Val, rhs: U256Val	U256Val
u256_div	lhs: U256Val, rhs: U256Val	U256Val
u256_rem_euclid	lhs: U256Val, rhs: U256Val	U256Val
u256_pow	lhs: U256Val, rhs: U32Val	U256Val
u256_shl	lhs: U256Val, rhs: U32Val	U256Val
u256_shr	lhs: U256Val, rhs: U32Val	U256Val

Proposed I256 Checked Functions:

Function	Parameters	Return
i256_checked_add	lhs: I256Val, rhs: I256Val	Val
i256_checked_sub	lhs: I256Val, rhs: I256Val	Val
i256_checked_mul	lhs: I256Val, rhs: I256Val	Val
i256_checked_div	lhs: I256Val, rhs: I256Val	Val
i256_checked_rem_euclid	lhs: I256Val, rhs: I256Val	Val
i256_checked_pow	lhs: I256Val, rhs: U32Val	Val
i256_checked_shl	lhs: I256Val, rhs: U32Val	Val
i256_checked_shr	lhs: I256Val, rhs: U32Val	Val

Proposed U256 Checked Functions:

Function	Parameters	Return
u256_checked_add	lhs: U256Val, rhs: U256Val	Val
u256_checked_sub	lhs: U256Val, rhs: U256Val	Val
u256_checked_mul	lhs: U256Val, rhs: U256Val	Val
u256_checked_div	lhs: U256Val, rhs: U256Val	Val
u256_checked_rem_euclid	lhs: U256Val, rhs: U256Val	Val
u256_checked_pow	lhs: U256Val, rhs: U32Val	Val
u256_checked_shl	lhs: U256Val, rhs: U32Val	Val
u256_checked_shr	lhs: U256Val, rhs: U32Val	Val

Total: 16 new host functions (8 for each type)

[jayz22]

Thanks @ozgunozerk and @leighmcculloch for the proposal. Yeah this is better than the original.
Just thinking about future proofing, would we ever need a saturating/wrapping version of the same functions? If so would be worth consider adding a toggle via a Symbol argument.

[leighmcculloch]

With regards to a host function versus a symbol parameter, is there a benefit to one or the other? .