fix various conversion functions from string to Decimal (#439)

* 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.

* fix failing test using to much precision

daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/Decimal.scala

@@ -11,7 +11,6 @@ import scala.math.BigDecimal
   *  These are numbers of precision 38 (38 decimal digits), and scale 10 (10 digits after the comma)
   */
 object Decimal {
-
   type Decimal = BigDecimal
 
   val scale: Int = 10
@@ -28,7 +27,11 @@ object Decimal {
 
   val min: Decimal = unlimitedBigDecimal("-9999999999999999999999999999.9999999999")
 
-  def check(x0: Decimal): Either[String, Decimal] = {
+  /** Checks that a `Decimal` falls between `min` and `max`, and
+    * round the number according to `scale`. Note that it does _not_
+    * fail if the number contains data beyond `scale`.
+    */
+  def checkWithinBoundsAndRound(x0: Decimal): Either[String, Decimal] = {
     if (x0 > max || x0 < min) {
       Left(s"out-of-bounds Decimal $x0")
     } else {
@@ -38,14 +41,26 @@ object Decimal {
     }
   }
 
-  def add(x: Decimal, y: Decimal): Either[String, Decimal] = check(x + y)
-  def div(x: Decimal, y: Decimal): Either[String, Decimal] = check(x / y)
-  def mult(x: Decimal, y: Decimal): Either[String, Decimal] = check(x * y)
-  def sub(x: Decimal, y: Decimal): Either[String, Decimal] = check(x - y)
+  /** Like `checkWithinBoundsAndRound`, but _fails_ if the given number contains
+    * any data beyond `scale`.
+    */
+  def checkWithinBoundsAndWithinScale(x0: Decimal): Either[String, Decimal] = {
+    for {
+      x1 <- checkWithinBoundsAndRound(x0)
+      // if we've lost any data at all, it means that we weren't within the
+      // scale.
+      x2 <- Either.cond(x0 == x1, x1, s"out-of-bounds Decimal $x0")
+    } yield x2
+  }
+
+  def add(x: Decimal, y: Decimal): Either[String, Decimal] = checkWithinBoundsAndRound(x + y)
+  def div(x: Decimal, y: Decimal): Either[String, Decimal] = checkWithinBoundsAndRound(x / y)
+  def mult(x: Decimal, y: Decimal): Either[String, Decimal] = checkWithinBoundsAndRound(x * y)
+  def sub(x: Decimal, y: Decimal): Either[String, Decimal] = checkWithinBoundsAndRound(x - y)
 
   def round(newScale: Long, x0: Decimal): Either[String, Decimal] =
     // check to make sure the rounding mode is OK
-    check(x0).flatMap(x =>
+    checkWithinBoundsAndRound(x0).flatMap(x =>
       if (newScale > scale || newScale < -27) {
         Left(s"Bad scale $newScale, must be between -27 and $scale")
       } else {
@@ -60,7 +75,7 @@ object Decimal {
   def fromString(s: String): Either[String, Decimal] =
     // parse with infinite precision, then check
     try {
-      check(unlimitedBigDecimal(s))
+      checkWithinBoundsAndWithinScale(unlimitedBigDecimal(s))
     } catch {
       case err: NumberFormatException =>
         Left(s"Could not read Decimal string: $err")

daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/SBuiltinTest.scala

@@ -248,10 +248,13 @@ class SBuiltinTest extends FreeSpec with Matchers with TableDrivenPropertyChecks
 
       val testCases = Table[String, (BigDecimal, BigDecimal) => Either[Any, SValue]](
         ("builtin", "reference"),
-        ("ADD_DECIMAL", (a, b) => Decimal.check(a + b).map(SDecimal)),
-        ("SUB_DECIMAL", (a, b) => Decimal.check(a - b).map(SDecimal)),
-        ("MUL_DECIMAL", (a, b) => Decimal.check(a * b).map(SDecimal)),
-        ("DIV_DECIMAL", (a, b) => if (b == 0) Left(()) else Decimal.check(a / b).map(SDecimal)),
+        ("ADD_DECIMAL", (a, b) => Decimal.checkWithinBoundsAndRound(a + b).map(SDecimal)),
+        ("SUB_DECIMAL", (a, b) => Decimal.checkWithinBoundsAndRound(a - b).map(SDecimal)),
+        ("MUL_DECIMAL", (a, b) => Decimal.checkWithinBoundsAndRound(a * b).map(SDecimal)),
+        (
+          "DIV_DECIMAL",
+          (a, b) =>
+            if (b == 0) Left(()) else Decimal.checkWithinBoundsAndRound(a / b).map(SDecimal)),
         ("LESS_EQ_DECIMAL", (a, b) => Right(SBool(a <= b))),
         ("GREATER_EQ_DECIMAL", (a, b) => Right(SBool(a >= b))),
         ("LESS_DECIMAL", (a, b) => Right(SBool(a < b))),

daml-lf/validation/src/test/scala/com/digitalasset/daml/lf/validation/TypingSpec.scala

@@ -113,7 +113,7 @@ class TypingSpec extends WordSpec with TableDrivenPropertyChecks with Matchers {
         // ExpLitInt64
         E"(( 42 ))" -> T"Int64",
         // ExpLitDecimal
-        E"(( 3.14159265359 ))" -> T"(( Decimal ))",
+        E"(( 3.1415926536 ))" -> T"(( Decimal ))",
         //ExpLitText
         E"""(( "text" ))""" -> T"(( Text ))",
         //ExpLitDate

docs/source/support/release-notes.rst

@@ -38,6 +38,8 @@ HEAD — ongoing
 - **BREAKING** Remove support for DAML 1.0 packages in the engine, and thus the
   sandbox. Note that the SDK has removed support for _compiling_ DAML 1.0
   months ago.
+- Fix critical bug related to the conversion of decimal numbers from Ledger API
+  strings, see #439.
 
 0.12.1 — 2019-04-04
 -------------------

ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/participant/util/ApiToLfEngine.scala

@@ -92,12 +92,7 @@ object ApiToLfEngine {
       Left(s"Failed to parse as decimal. ($d). Expected format is [+-]?\\d+(\\.\\d+)?")
     expectedDecimalFormat
       .findFirstIn(d)
-      .fold(notValidErr)(
-        (_) =>
-          // note: using big decimal constructor
-          // 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))
   }
 
   def toLfIdentifier(

ledger/ledger-api-common/src/test/suite/scala/com/digitalasset/platform/participant/util/ApiToLfEngineSpec.scala

@@ -135,5 +135,14 @@ class ApiToLfEngineSpec extends WordSpec with Matchers {
       }
     }
 
+    "handle Decimals exceeding scale correctly" in {
+      ApiToLfEngine.parseDecimal("0.0000000001") shouldBe Right(BigDecimal("0.0000000001"))
+      ApiToLfEngine.parseDecimal("0.00000000005") shouldBe 'left
+    }
+
+    "handle Decimals exceeding bounds" in {
+      ApiToLfEngine.parseDecimal("10000000000000000000000000000.0000000000") shouldBe 'left
+      ApiToLfEngine.parseDecimal("-10000000000000000000000000000.0000000000") shouldBe 'left
+    }
   }
 }

ledger/ledger-api-integration-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/CommandTransactionChecks.scala

@@ -708,6 +708,40 @@ abstract class CommandTransactionChecks
         }
       }
 
+      "handle bad Decimals correctly" in allFixtures { ctx =>
+        val alice = "Alice"
+        for {
+          _ <- failingCreate(
+            ctx,
+            "Decimal-scale",
+            alice,
+            templateIds.decimalRounding,
+            Record(fields = List(RecordField(value = Some(alice.asParty)), RecordField(value = Some("0.00000000005".asDecimal)))),
+            Code.INVALID_ARGUMENT,
+            "out-of-bounds Decimal"
+          )
+          _ <- failingCreate(
+            ctx,
+            "Decimal-bounds-positive",
+            alice,
+            templateIds.decimalRounding,
+            Record(fields = List(RecordField(value = Some(alice.asParty)), RecordField(value = Some("10000000000000000000000000000.0000000000".asDecimal)))),
+            Code.INVALID_ARGUMENT,
+            "out-of-bounds Decimal"
+          )
+          _ <- failingCreate(
+            ctx,
+            "Decimal-bounds-negative",
+            alice,
+            templateIds.decimalRounding,
+            Record(fields = List(RecordField(value = Some(alice.asParty)), RecordField(value = Some("-10000000000000000000000000000.0000000000".asDecimal)))),
+            Code.INVALID_ARGUMENT,
+            "out-of-bounds Decimal"
+          )
+        } yield {
+          succeed
+        }
+      }
     }
   }
 

ledger/ledger-api-integration-tests/src/test/lib/scala/com/digitalasset/platform/apitesting/TestTemplateIdentifiers.scala

@@ -99,6 +99,8 @@ final case class TestTemplateIdentifiers(testPackageId: String) {
     Identifier(testPackageId, "Test.Divulgence1", "Test", "Divulgence1")
   val divulgence2 =
     Identifier(testPackageId, "Test.Divulgence2", "Test", "Divulgence2")
+  val decimalRounding =
+    Identifier(testPackageId, "Test.DecimalRounding", "Test", "DecimalRounding")
   val allTemplates =
     List(
       dummy,

ledger/sandbox/src/test/resources/damls/Test.daml

@@ -379,6 +379,13 @@ template Divulgence2
         do
           archive div1ToArchive
 
+template DecimalRounding
+  with
+    party: Party
+    number: Decimal
+  where
+    signatory party
+
 {-
 testDivulgence : Scenario ()
 testDivulgence = test do