-
Notifications
You must be signed in to change notification settings - Fork 88
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
Deny clippy::arithmetic_side_effects #725
Conversation
fuel-tx/src/transaction/metadata.rs
Outdated
@@ -84,7 +84,7 @@ impl CommonMetadata { | |||
.iter() | |||
.map(|input| { | |||
let i = offset; | |||
offset += input.size(); | |||
offset = offset.saturating_add(input.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added validation in 042db35. Might still be a breaking change, since I created new ValidityError
variants for these. The errors didn't seem to be a good match with this.
Work towards #170
This does the changes for all crates other than
fuel-merkle
, which will be done in a separate PR.Denies the following lints:
clippy::arithmetic_side_effects
clippy::cast_sign_loss
clippy::cast_possible_truncation
clippy::cast_possible_wrap
The following bugs were fixed:
UtxoId::from_str
now rejects inputs with multiple0x
prefixesAssetId
andBlockHeight
now reject extra bytes in theirfrom_str
implementation (these were ignored before)Some improvements were also done:
from_str
error messages now indicate which type caused the errorSome changes introduce Rust-level panics where normally an overflow would have only panicked on debug mode.
Memory offsets of several types were previously calculated using unchecked operations. This PR changes those to use
saturating_*
operations instead. This means that given incorrect consensus parameters, some operations that previously overflowed silently know instead give memory offsets outside VM ram, causing a VM-level panic on access. This is strictly an improvement over previous behavior, but still leaves a lot to be desired. I'm not sure if it's actually possible to do much better, though. Maybe with custom types? The current approach feels like the most sensible tradeoff.Checklist
Before requesting review