Fix oracle price validation and add math overflow saturation tests - #489
Open
Uchechukwu-Ekezie wants to merge 1 commit into
Open
Fix oracle price validation and add math overflow saturation tests#489Uchechukwu-Ekezie wants to merge 1 commit into
Uchechukwu-Ekezie wants to merge 1 commit into
Conversation
Closes SO4-Markets#384 Issue: mul_div_wide and mul_div_wide_up compute product/ceiling-division in a wider integer type and then convert back with a fallback that silently saturates to i128::MAX/MIN when the true result doesn't fit. None of the existing property tests pushed both operands large enough simultaneously to exercise this fallback. Fix: Add comprehensive tests for overflow saturation behavior: - mul_div_wide_overflow_saturates_to_max: Tests i128::MAX * i128::MAX / 1 - mul_div_wide_negative_overflow_saturates_to_min: Tests i128::MIN * i128::MAX / 1 - mul_div_wide_up_overflow_saturates_to_max: Ceiling division overflow to MAX - mul_div_wide_up_negative_overflow_saturates_to_min: Ceiling division overflow to MIN - mul_div_wide_large_but_safe_values: Ensures large non-overflowing cases work correctly These tests pin down the saturation behavior, ensuring regressions would be caught.
|
@Uchechukwu-Ekezie Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #383, Closes #384
Summary
This PR addresses two issues: oracle price validation (HIGH priority) and untested math overflow saturation behavior (LOW priority).
Issue #383: Oracle - validate u128-to-i128 cast
Problem:
get_stable_priceandget_price_with_stable_fallbackcast stored u128 directly to i128 without validation. A u128 value >= 2^127 reinterprets as a negative i128, bypassing safety checks.Fix:
This matches validation applied to keeper-submitted prices.
Issue #384: Math - test overflow saturation fallback
Problem:
mul_div_wideandmul_div_wide_uphave untested overflow saturation fallback. The comment admits "shouldn't happen in normal protocol use" — an unverified assumption rather than enforced invariant.Fix: Add 5 comprehensive tests covering:
These tests pin down the saturation behavior and catch regressions.
Issue #385: Position manager lookup reversal
Status: Requires further work beyond this PR.
The
create_orderfunction has the position manager lookup reversed:get_position_manager(\&caller, market)looks up "who is caller's own manager"get_position_manager(\&owner, market) == Some(caller)to verify caller IS a manager for ownerThis requires adding an
on_behalf_of: Option<Address>field to CreateOrderParams and updating the authorization logic. Deferred to follow-up PR for thorough testing.Testing