search

xJonathanLEI / starknet-rs

Complete Starknet library in Rust™
https://starknet.rs
Apache License 2.0
281 stars 99 forks source link

Incorrect error handling #454

Closed satoshiotomakan closed 1 year ago

satoshiotomakan commented 1 year ago

Unwraps: https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/fe_utils.rs#L43 https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/fe_utils.rs#L66 https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/rfc6979.rs#L52 https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-curve/src/ec_point.rs#L41-L56 https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-ff/src/lib.rs#L144-L170

Panics: https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/fe_utils.rs#L46-L67 https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-ff/src/lib.rs#L288-L291

xJonathanLEI commented 1 year ago

Sorry but some of these have already been answered in #446, an issue submitted by yourself. I prefer not to waste my time answering in this case.

satoshiotomakan commented 1 year ago

@xJonathanLEI sorry for the duplicates, my mistake.

xJonathanLEI commented 1 year ago

Unwraps:

https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/fe_utils.rs#L43

modulus is a FieldElement, and result is mod modulus. So result is always a valid FieldElement.

https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-crypto/src/fe_utils.rs#L66

result is always smaller than modulus, which is a valid FieldElement itself.

https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-curve/src/ec_point.rs#L41-L56

I'm actually not sure about this. This seems to panic when self.y is zero.

https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-ff/src/lib.rs#L144-L170

We already checked for is_some before unwrapping. It's safe.

Panics:

https://github.com/xJonathanLEI/starknet-rs/blob/91425ad9c20cceaf87f5ae179341f7e1f7958507/starknet-ff/src/lib.rs#L288-L291

As mentioned in the comment, panicking on division by zero is the expected behavior, similar to how division by zero panics for any other types.