Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix various conversion functions from string to Decimal #439

Merged
merged 3 commits into from Apr 14, 2019

Conversation

Projects
None yet
2 participants
@francesco-da
Copy link
Contributor

commented Apr 12, 2019

Fixes #399.

This fixes a critical bug -- since:

  • The DAML-LF spec specifies that the scale of Decimal is 10 --
    that is, there are at most 10 digits past the decimal point:
    https://github.com/digital-asset/daml/blob/79bbf5c794530ed7e29e61fcc2d98221cdb221e3/daml-lf/spec/value.rst#field-decimal.

  • However, the code converting from the string representation that
    we get on the wire was not performing this check. This is due
    to two reasons:

    • Decimal.check is a function that checks whether a given
      Decimal is within the upper and lower bounds of what the
      DAML-LF spec specifies, but crucially it does not check that
      the scale is not exceeded:

      def check(x0: Decimal): Either[String, Decimal] = {
      .
      This behavior is correct in some cases (more on that later),
      but not in others. Crucially, Decimal.fromString, which was
      supposed to check if a decimal string literal is valid, used
      this function, which means that it accepted string literals
      containing numbers out of the required scale, rounding them to
      make them fit within the scale. This function has been renamed
      to Decimal.checkWithinBoundsAndRound, and a new function
      Decimal.checkWithinBoundsAndWithinScale has been added, which
      fails if the number provided has data not within the scale.
      Decimal.fromString now uses
      Decimal.checkWithinBoundsAndWithinScale.

    • ApiToLfEngine.parseDecimal assumed that Decimal.fromString did
      fail when passed numbers that were in any way invalid, and
      moreover did not use the result of Decimal.fromString, but rather
      re-parsed the string into an unconstrained BigDecimal:

      .
      The reason for the code to work this way is that in the past
      it was responsible for converting decimal strings both for the
      current engine but also for an older version of the engine which
      handled decimals of a different type. Both issues have been fixed.

  • Therefore, Decimals with scale exceeding the specified scale
    made it into the engine, and contracts could be created containing
    this invalid value.

  • Once on the ledger, these bad numbers can be used to produce extremely
    surprising results, due to how Decimal operations are
    implemented. Generally speaking, all operations on Decimal
    first compute the result and then run the output through
    Decimal.checkWithinBoundsAndRound. The reason for this behavior
    is that we specify multiplication and division as rounding their
    output. Consider the case where the bad value 0.00000000005 made
    it to the engine, and is then added to 100. The full-precision
    result will be 100.00000000005, which after rounding becomes 100.
    Therefore, on a ledger where such invalid values exist, it is not
    the case that y > 0 ==> x + y != x, and so on.

Thanks a bunch to @briandbecker for the excellent bug report.

Pull Request Checklist

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@francesco-da francesco-da requested a review from remyhaemmerle-da Apr 12, 2019

@francesco-da francesco-da changed the title fix various conversion functions from string to Decima fix various conversion functions from string to Decimal Apr 12, 2019

@francesco-da francesco-da force-pushed the 399-convert-decimal-correctly branch 2 times, most recently from 38a9c0b to 249077c Apr 12, 2019

@remyhaemmerle-da
Copy link
Contributor

left a comment

just minor comments

x1 <- checkWithinBoundsAndRound(x0)
// if we've lost any data at all, it means that we weren't within the
// scale.
x2 <- if (x0 == x1) {

This comment has been minimized.

Copy link
@remyhaemmerle-da

remyhaemmerle-da Apr 12, 2019

Contributor
Suggested change
x2 <- if (x0 == x1) {
x2 <- Either.cond(x0 == x1)(s"out-of-bounds Decimal $x0")(x1)
// to have equal precision set as with damle core
// remove this when removing old engine
Decimal.fromString(d).map(_ => BigDecimal(d)))
.fold(notValidErr)(_ => Decimal.fromString(d))

This comment has been minimized.

Copy link
@remyhaemmerle-da
@@ -135,5 +135,13 @@ class ApiToLfEngineSpec extends WordSpec with Matchers {
}
}

"handle Decimals exceeding scale correctly" in {
ApiToLfEngine.parseDecimal("0.00000000005") shouldBe 'left
}

This comment has been minimized.

Copy link
@remyhaemmerle-da

remyhaemmerle-da Apr 12, 2019

Contributor
Suggested change
}
ApiToLfEngine.parseDecimal("0.0000000001") shouldBe Right(BigDecimal("0.0000000001"))
}
if (x0 > max || x0 < min) {
Left(s"out-of-bounds Decimal $x0")

This comment has been minimized.

Copy link
@remyhaemmerle-da

remyhaemmerle-da Apr 12, 2019

Contributor

I will have let the creation of the left inside the if as it is not use anywhere else

@francesco-da francesco-da force-pushed the 399-convert-decimal-correctly branch 3 times, most recently from 4a1895d to 3b8ff5e Apr 12, 2019

fix various conversion functions from string to Decimal
Fixes #399.

This fixes a critical bug -- since:

* The DAML-LF spec specifies that the scale of `Decimal` is 10 --
    that is, there are at most 10 digits past the decimal point:
    <https://github.com/digital-asset/daml/blob/79bbf5c794530ed7e29e61fcc2d98221cdb221e3/daml-lf/spec/value.rst#field-decimal>.
* However, the code converting from the string representation that
    we get on the wire was _not_ performing this check. This is due
    to two reasons:

    - `Decimal.check` is a function that checks whether a given
        `Decimal` is within the upper and lower bounds of what the
        DAML-LF spec specifies, but crucially it does _not_ check that
        the scale is not exceeded:
        <https://github.com/digital-asset/daml/blob/79bbf5c794530ed7e29e61fcc2d98221cdb221e3/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/Decimal.scala#L31>.
        This behavior is correct in some cases (more on that later),
        but not in others. Crucially, `Decimal.fromString`, which was
        supposed to check if a decimal string literal is valid, used
        this function, which means that it accepted string literals
        containing numbers out of the required scale, rounding them to
        make them fit within the scale. This function has been renamed
        to `Decimal.checkWithinBoundsAndRound`, and a new function
        `Decimal.checkWithinBoundsAndWithinScale` has been added, which
        fails if the number provided has data not within the scale.
        `Decimal.fromString` now uses
        `Decimal.checkWithinBoundsAndWithinScale`.

    - `ApiToLfEngine.parseDecimal` assumed that `Decimal.fromString` _did_
        fail when passed numbers that were in any way invalid, and
        moreover did _not_ use the result of `Decimal.fromString`, but rather
        re-parsed the string into an unconstrained `BigDecimal`:
        <https://github.com/digital-asset/daml/blob/79bbf5c794530ed7e29e61fcc2d98221cdb221e3/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/participant/util/ApiToLfEngine.scala#L96>.
        The reason for the code to work this way is that in the past
        it was responsible for converting decimal strings both for the
        current engine but also for an older version of the engine which
        handled decimals of a different type. Both issues have been fixed.

* Therefore, `Decimal`s with scale exceeding the specified scale
    made it into the engine, and contracts could be created containing
    this invalid value.

* Once on the ledger, these bad numbers can be used to produce extremely
    surprising results, due to how `Decimal` operations are
    implemented. Generally speaking, all operations on `Decimal`
    first compute the result and then run the output through
    `Decimal.checkWithinBoundsAndRound`. The reason for this behavior
    is that we specify multiplication and division as rounding their
    output. Consider the case where the bad value 0.00000000005 made
    it to the engine, and is then added to 100. The full-precision
    result will be 100.00000000005, which after rounding becomes 100.
    Therefore, on a ledger where such invalid values exist, it is not
    the case that `y > 0 ==> x + y != x`, and so on.

Thanks a bunch to @briandbecker for the excellent bug report.

@francesco-da francesco-da force-pushed the 399-convert-decimal-correctly branch from 3b8ff5e to 97409c1 Apr 12, 2019

@francesco-da

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

retest this please

francesco-da and others added some commits Apr 12, 2019

@francesco-da francesco-da merged commit 6af8405 into master Apr 14, 2019

5 checks passed

digital-asset.daml Build #20190412.157 succeeded
Details
digital-asset.daml (Linux) Linux succeeded
Details
digital-asset.daml (Windows) Windows succeeded
Details
digital-asset.daml (macOS) macOS succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@garyverhaegen-da garyverhaegen-da deleted the 399-convert-decimal-correctly branch Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.