fix(contracts): add DivisionByZero error variant for checked_div sites - #671
Conversation
|
@cashmotor01 is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
packages/contracts/vault/src/lib.rs does not compile as committed. It looks like a GitHub web merge-conflict resolution got pasted in literally instead of resolved: line 215 has a raw GitHub conflict-resolution URL concatenated straight onto a closing brace, and the shares_to_mint computation right after it has two different versions of the same checked_mul/checked_div chain concatenated one after another, the first one already terminated with a semicolon and the second dangling off nothing.
This needs a clean rebase onto current main and a proper manual conflict resolution, not a patch on top of the corrupted file. Also note, same as the earlier round on this pattern in #644: DivisionByZero = 3 in blend-adapter collides with the existing NotInitialized = 3, and DivisionByZero = 19 in vault skips over MinAmountOutNotMet = 15 and NoPendingAdmin = 16 that already exist on main; after rebasing this should be 4 and 17 respectively.
| if total_shares > 0 && total_assets <= 0 { | ||
| return Err(ContractError::AdapterReportedNoAssets); | ||
| } | ||
| }https://github.com/drydocs/meridian/pull/671/conflict?name=packages%252Fcontracts%252Fvault%252Fsrc%252Flib.rs&ancestor_oid=5da2079f71a9c080d32edf55b78fafd79dbb877a&base_oid=9567e868a38dfec5c837a18cf35b890d6e172a59&head_oid=2da4054355f35c399d2f7f50c6c72e54bdc494b6 |
There was a problem hiding this comment.
This line has a literal GitHub merge-conflict-resolution URL pasted into the source (}https://github.com/.../pull/671/conflict?...), and the shares_to_mint block just below it concatenates two different versions of the same checked_mul/checked_div chain, one terminated with ; and a second one dangling off nothing right after. This file does not compile as committed. Please rebase onto current main and resolve the conflict properly rather than patching over this.
| /// Distinct from `Overflow`: this points to a degenerate adapter state | ||
| /// (e.g. a zero `b_rate` from a broken Blend pool) rather than a genuine | ||
| /// arithmetic overflow. | ||
| DivisionByZero = 3, |
There was a problem hiding this comment.
DivisionByZero = 3 collides with the existing NotInitialized = 3 two lines below, in the same ContractError enum. Rust rejects duplicate explicit discriminants, so this fails to compile with error[E0081]: discriminant value 3 assigned more than once. DivisionByZero needs a free discriminant, for example 4.
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
@cashmotor01 This doesn't compile in either crate it touches, and GitHub reports it as CONFLICTING/DIRTY. See inline comments.
| if total_shares > 0 && total_assets <= 0 { | ||
| return Err(ContractError::AdapterReportedNoAssets); | ||
| } | ||
| }https://github.com/drydocs/meridian/pull/671/conflict?name=packages%252Fcontracts%252Fvault%252Fsrc%252Flib.rs&ancestor_oid=5da2079f71a9c080d32edf55b78fafd79dbb877a&base_oid=9567e868a38dfec5c837a18cf35b890d6e172a59&head_oid=2da4054355f35c399d2f7f50c6c72e54bdc494b6 |
There was a problem hiding this comment.
@cashmotor01 A literal, unresolved merge-conflict artifact is concatenated directly onto the file here: a GitHub conflict-resolution URL string sits right after a closing brace with no separating token. This alone is a parse error.
| .ok_or(ContractError::Overflow)?, | ||
| ) | ||
| .ok_or(ContractError::Overflow)? | ||
| .checked_div(total_assets + OFFSET) |
There was a problem hiding this comment.
@cashmotor01 deposit()'s shares_to_mint calculation has two complete, conflicting .checked_div(...).ok_or(...)?; chains stacked back to back here, one new (DivisionByZero) immediately followed by the original (Overflow). Same unresolved-merge artifact as the line above.
| /// Distinct from `Overflow`: this points to a degenerate adapter state | ||
| /// (e.g. a zero `b_rate` from a broken Blend pool) rather than a genuine | ||
| /// arithmetic overflow. | ||
| DivisionByZero = 3, |
There was a problem hiding this comment.
@cashmotor01 DivisionByZero = 3 collides with the existing NotInitialized = 3 two lines below, in the same #[repr(u32)] enum. Two variants sharing a discriminant is a hard compile error (E0081). This should be 4, the next free value.
cb9b899 to
8780286
Compare
|
Thank you for the contribution, @cashmotor01. Verified: DivisionByZero is correctly distinguished from Overflow across all checked_div sites in both vault and blend-adapter, with no discriminant collisions. All required checks are green. Merging now. |
|
If you have a moment, a star on the repo would be appreciated. |
2 participants
Distinguish divide-by-zero from genuine arithmetic overflow. Previously all checked_div calls mapped None to ContractError::Overflow, making it impossible to tell from the error code alone whether the failure was a real i128 overflow or a degenerate zero divisor (e.g. broken adapter reporting zero total_assets).
Changes:
Overflow is now reserved exclusively for checked_mul/checked_add failures. No breaking change — new variants are appended.
Summary
Test plan
pnpm lint && pnpm typecheck && pnpm testpass locallyCloses #570