Comparisons and overflow/underflow on `Felt`

[PhilippGackstatter]

Felt Comparison

https://docs.miden.xyz/builder/tutorials/rust-compiler/miden-bank/constants-constraints#safe-felt-comparisons says:

Never use <, >, <=, or >= operators directly on Felt values. They produce incorrect results due to field element ordering.

Is this still accurate? It looks like miden_field::Felt properly uses the built-in instructions for this which shouldn't produce "incorrect" results (I'm not sure what exactly we mean by that).

Safe Arithmetic

Similarly, the miden bank tutorial has a large section on the dangers of underflows: https://docs.miden.xyz/builder/tutorials/rust-compiler/miden-bank/asset-management#step-2-add-the-withdraw-method-skeleton.

Ideally this would be easy for users to avoid, e.g. by implementing checked* arithmetic for the miden_field::Felt type , so users can easily do things like current_balance.checked_sub(withdraw_amount).expect("insufficient balance").

If correct/agreed, we'd have to open an issue in miden-crypto.

An even safer alternative, at least for on-chain users, would be to let impl Add for Felt (and all such related impls) panic instead of wrap around. We'd implement wrapping_add and such instead for explicit use. In other words, it'd be ideal to follow the safe design of arithmetic functionality on the Rust primitive types and have Felt::{wrapping_add, checked_add, overflowing_add, ...}.

Maybe we could selectively do this only for the miden_field::Felt type in wasm32, which would be ideal to not have to touch the non-wasm parts, though even that might be doable.

cc @greenhat for thoughts

[greenhat]

Linking 0xMiden/compiler#999.

Safe Arithmetic

Similarly, the miden bank tutorial has a large section on the dangers of underflows: https://docs.miden.xyz/builder/tutorials/rust-compiler/miden-bank/asset-management#step-2-add-the-withdraw-method-skeleton.

Ideally this would be easy for users to avoid, e.g. by implementing checked* arithmetic for the miden_field::Felt type , so users can easily do things like current_balance.checked_sub(withdraw_amount).expect("insufficient balance").

This feels like "bending" the Felt ops to be more like integer ops which might complicate things even more. See below.

If correct/agreed, we'd have to open an issue in miden-crypto.

An even safer alternative, at least for on-chain users, would be to let impl Add for Felt (and all such related impls) panic instead of wrap around. We'd implement wrapping_add and such instead for explicit use. In other words, it'd be ideal to follow the safe design of arithmetic functionality on the Rust primitive types and have Felt::{wrapping_add, checked_add, overflowing_add, ...}.

Maybe we could selectively do this only for the miden_field::Felt type in wasm32, which would be ideal to not have to touch the non-wasm parts, though even that might be doable.

This would blur the lines between felt and integer ops which might make things worse. I think we have two categories of users:

1. Ones that don't care/know about the felt arithmetic nature (wrapping, inversion instead of division, etc.) and think that it's just some "fancy"/fast integer arithmetic.
2. Ones that know what felt arithmetics are and want to use them (cryptography?).

We should nudge the first group toward the integer types (u64, etc.) and expose "pure" felt arithmetics for the second group.