Repository navigation
fix(common_utils): return an error instead of panicking when a major-unit amount overflows Decimal - #14070
Open
edubraqd wants to merge 1 commit into
Open
fix(common_utils): return an error instead of panicking when a major-unit amount overflows Decimal#14070edubraqd wants to merge 1 commit into
Decimal#14070edubraqd wants to merge 1 commit into
Conversation
This branch has not been deployed
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
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.
Type of Change
Description
FloatMajorUnit::to_minor_unit_as_i64andStringMajorUnit::to_minor_unit_as_i64scale theparsed amount with
amount_decimal * Decimal::from(100)(or1000for three-decimalcurrencies).
rust_decimal'sMulimplementation panics on overflow(
Multiplication overflowed), so any amount in the rangeDecimal::MAX / 1000 .. Decimal::MAX(roughly
7.9e25 .. 7.9e28) — whichDecimal::from_f64/Decimal::from_strhappily accept —takes the handler down instead of surfacing as a conversion error. The
i64overflow rightafter it is already handled through
ParsingError::DecimalToI64ConversionFailure, so the twooverflow paths were inconsistent.
These conversions run through
AmountConvertor::convert_backon connector responses(
convert_back_amount_to_minor_unitsinhyperswitch_connectors, used for theauthorize / sync / capture integrity objects), so a malformed or absurd amount coming back from a
connector should produce
ConnectorError::AmountConversionFailed, not a panic.This PR moves the scaling into a small
scale_major_to_minorhelper that useschecked_mulandmaps the overflow to the existing
DecimalToI64ConversionFailure(the result would not fit in ani64either), and adds a unit test for both theDecimaloverflow and thei64overflow.Additional Changes
Motivation and Context
Found while running a small property-style harness over the pure helpers in
common_utilswith extreme inputs. Reproduction on
main:The same happens for
StringMajorUnitwith"792281625142643375935439503.36".How did you test it?
cargo test -p common_utils --lib types::amount_conversion_tests— the newamount_conversion_back_from_major_unit_overflow_is_an_errorpanics onmainand passes withthis change; the existing round-trip tests are unchanged.
cargo +nightly fmt --allandcargo clippy -p common_utils --lib --testsare clean.Checklist
cargo +nightly fmt --allcargo clippyCloses #14199