ralexstokes / ssz-rs

Implementation of ethereum's `ssz`
Apache License 2.0
102 stars 40 forks source link

Note possible checked_sub cases #57

Closed doubledup closed 1 year ago

doubledup commented 1 year ago

For #22, I've looked through the places we've used subtraction and haven't found any cases where checked_sub is strictly necessary. The constraints for each case are documented in comments before the subtraction. I've also added a few constraints where necessary.

~Opening as a draft since I'm not sure whether we want to use checked_sub anyway, just in case the constraints change and we miss something. There is a small performance cost to checked_sub and I'm fairly certain it's unnecessary, so it's down to a cautiousness vs performance tradeoff.~

ralexstokes commented 1 year ago

so it's down to a cautiousness vs performance tradeoff.

I think we are fine to just add comments like you have done; I would consider calling them out explicitly with a SAFETY: marker in the comments

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 95.74% and project coverage change: +30.97 :tada:

Comparison is base (2300ff8) 44.28% compared to head (8bde6b5) 75.26%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #57 +/- ## =========================================== + Coverage 44.28% 75.26% +30.97% =========================================== Files 20 18 -2 Lines 1409 857 -552 =========================================== + Hits 624 645 +21 + Misses 785 212 -573 ``` | [Impacted Files](https://app.codecov.io/gh/ralexstokes/ssz-rs/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alex+Stokes) | Coverage Δ | | |---|---|---| | [ssz-rs/src/bitvector.rs](https://app.codecov.io/gh/ralexstokes/ssz-rs/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alex+Stokes#diff-c3N6LXJzL3NyYy9iaXR2ZWN0b3IucnM=) | `82.02% <0.00%> (-0.94%)` | :arrow_down: | | [ssz-rs/src/bitlist.rs](https://app.codecov.io/gh/ralexstokes/ssz-rs/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alex+Stokes#diff-c3N6LXJzL3NyYy9iaXRsaXN0LnJz) | `79.20% <83.33%> (+0.26%)` | :arrow_up: | | [ssz-rs/src/de.rs](https://app.codecov.io/gh/ralexstokes/ssz-rs/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alex+Stokes#diff-c3N6LXJzL3NyYy9kZS5ycw==) | `80.00% <100.00%> (+7.11%)` | :arrow_up: | | [ssz-rs/src/merkleization/mod.rs](https://app.codecov.io/gh/ralexstokes/ssz-rs/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alex+Stokes#diff-c3N6LXJzL3NyYy9tZXJrbGVpemF0aW9uL21vZC5ycw==) | `92.66% <100.00%> (+0.82%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/ralexstokes/ssz-rs/pull/57/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alex+Stokes)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.