fix(tholos-v2): use checked arithmetic in register() - #153
Conversation
Replaces unchecked previous_amount + amount and �ligible_total - previous_amount + new_amount with checked_add/checked_sub, returning Error::SettlementArithmeticOverflow on overflow. Closes drydocs#131
| assert_eq!(result, Err(Ok(Error::CommitmentMismatch))); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
There's a double blank line above this function. cargo fmt --check will fail CI's formatting gate on this. Please run cargo fmt before pushing.
| .get(&DataKey::Resolution(id)) | ||
| .unwrap(); | ||
| resolution.eligible_total = i128::MAX; | ||
| env.storage().persistent().set(&DataKey::Resolution(id), &resolution); |
There was a problem hiding this comment.
This line is unwrapped and exceeds rustfmt's line width, another cargo fmt --check failure. Run cargo fmt to reformat it.
| DEFAULT_MAX_POSITION, | ||
| DEFAULT_MAX_TOTAL_WEIGHT, | ||
| ); | ||
| assert_eq!(result, Ok(Ok(()))); |
There was a problem hiding this comment.
Missing trailing newline at end of file, also flagged by cargo fmt --check.
initialize() rejects max_position/max_total_weight above MAX_SETTLEMENT_TOTAL_WEIGHT with InvalidMaxTotalWeight, so the two register() overflow tests passed i128::MAX as caps and died at init instead of exercising the checked arithmetic. Use MAX_SETTLEMENT_TOTAL_WEIGHT as the caps and fund the voter at the min-resolution-bond level so register() passes the bond gate and the overflow paths in checked_add/checked_sub are actually reached. Also drop the extra blank line flagged by cargo fmt --check.
- Run cargo fmt (double blank line, line width, trailing newline) - Fix overflow test setup: use MAX_SETTLEMENT_TOTAL_WEIGHT for init params (i128::MAX is rejected by initialize), mint enough for voter bond, and register with DEFAULT_BOND so the overflow path is actually hit
|
Pushed fixes for the review feedback:
cargo test --package tholos-v2 passes (109 tests). cargo fmt --all -- --check and cargo clippy --package tholos-v2 --lib -- -D warnings are clean. |
|
Hi @collinsezedike, thanks for the review. I ran |
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
@ZacLou thank you for the contribution. This looks good, there is nothing to flag.
3 participants
Closes #131
Replaces the unchecked previous_amount + amount and �ligible_total - previous_amount + new_amount calculations in
egister() with checked_add/checked_sub, returning the existing Error::SettlementArithmeticOverflow on overflow.
Also adds two unit tests that seed storage near i128::MAX to confirm both error paths are reachable.
egister() arithmetic is now checked