From fc2c2ca008efb26a6c910ba78b29f741e8b47db3 Mon Sep 17 00:00:00 2001 From: Francisco Gindre Date: Mon, 1 Jan 2024 16:00:52 -0300 Subject: [PATCH] [#35] Use BigDecimal instead of Decimal closes #35 fixes a problem where tests are failing because of a Swift compiler issue where Decimal from a literal value is converted from literal to Double and then initialized as Decimal, causing a precision problem where Decimal("123.456") != Decimal(123.456) CHANGELOG.md --- CHANGELOG.md | 10 ++ Package.resolved | 27 ++++ Package.swift | 8 +- Sources/zcash-swift-payment-uri/Amount.swift | 120 ++++++++++++++++++ Sources/zcash-swift-payment-uri/Model.swift | 73 +---------- Sources/zcash-swift-payment-uri/Parser.swift | 2 +- .../AmountTests.swift | 7 + .../ParsingTests.swift | 6 +- .../RendererTests.swift | 4 +- .../ZIP321Tests.swift | 46 +++++++ 10 files changed, 225 insertions(+), 78 deletions(-) create mode 100644 Sources/zcash-swift-payment-uri/Amount.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 72d5a04..d0dece3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,16 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.1.0-beta.1] - 2024-01-01 +- Fixed [problem with literal Decimals](https://github.com/pacu/zcash-swift-payment-uri/issues/35) +- Always favor using `BigDecimal` to avoid misrepresentations of Decimal from +implicit conversion from `Double`. + +### additions +- BigDecimal library that handles the internals of `Amount` +- `init(decimal:)` uses BigDecimal +- `init(value:)` uses Swift's Double + ## [0.1.0-beta] - 2023-12-23 - CI had to be disabled because of swift 5.9 issue with SwiftFormat - `Parser` API diff --git a/Package.resolved b/Package.resolved index e01571f..e56a350 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,23 @@ { "pins" : [ + { + "identity" : "bigdecimal", + "kind" : "remoteSourceControl", + "location" : "https://github.com/mgriebling/BigDecimal.git", + "state" : { + "revision" : "c4e8348c7fbc90f29225d5f8681ce33a16ab33a2", + "version" : "2.2.3" + } + }, + { + "identity" : "bigint", + "kind" : "remoteSourceControl", + "location" : "https://github.com/mgriebling/BigInt.git", + "state" : { + "revision" : "498d4d290658d7c43a24b9e309c321592dc294f2", + "version" : "2.0.10" + } + }, { "identity" : "collectionconcurrencykit", "kind" : "remoteSourceControl", @@ -90,6 +108,15 @@ "version" : "7.0.2" } }, + { + "identity" : "uint128", + "kind" : "remoteSourceControl", + "location" : "https://github.com/mgriebling/UInt128.git", + "state" : { + "revision" : "59dac4f14d657fd60bacfdfb7398d38b450af74f", + "version" : "3.1.5" + } + }, { "identity" : "xctest-dynamic-overlay", "kind" : "remoteSourceControl", diff --git a/Package.swift b/Package.swift index f85fda0..30db68f 100644 --- a/Package.swift +++ b/Package.swift @@ -7,6 +7,7 @@ let dependencies: [Package.Dependency] = [ .package(url: "https://github.com/realm/SwiftLint.git", from: "0.54.0"), .package(url: "https://github.com/pointfreeco/swift-parsing", from: "0.13.0"), .package(url: "https://github.com/pointfreeco/swift-case-paths", exact: Version(stringLiteral: "1.0.0")), + .package(url: "https://github.com/mgriebling/BigDecimal.git", from: "2.0.0") ] let targets: [Target] = [ @@ -14,7 +15,10 @@ let targets: [Target] = [ // Targets can depend on other targets in this package and products from dependencies. .target( name: "zcash-swift-payment-uri", - dependencies: [.product(name: "Parsing", package: "swift-parsing")], + dependencies: [ + .product(name: "Parsing", package: "swift-parsing"), + .product(name: "BigDecimal", package: "BigDecimal"), + ], plugins: [.plugin(name: "SwiftLintPlugin", package: "SwiftLint")]), .testTarget( name: "zcash-swift-payment-uriTests", @@ -25,6 +29,7 @@ let targets: [Target] = [ let dependencies: [Package.Dependency] = [ .package(url: "https://github.com/pointfreeco/swift-parsing", from: "0.13.0"), .package(url: "https://github.com/pointfreeco/swift-case-paths", exact: Version(stringLiteral: "1.0.0")), + .package(url: "https://github.com/mgriebling/BigDecimal.git", from: "2.0.0") ] let targets: [Target] = [ @@ -34,6 +39,7 @@ let targets: [Target] = [ name: "zcash-swift-payment-uri", dependencies: [ .product(name: "Parsing", package: "swift-parsing"), + .product(name: "BigDecimal", package: "BigDecimal"), ] ), .testTarget( diff --git a/Sources/zcash-swift-payment-uri/Amount.swift b/Sources/zcash-swift-payment-uri/Amount.swift new file mode 100644 index 0000000..457acfc --- /dev/null +++ b/Sources/zcash-swift-payment-uri/Amount.swift @@ -0,0 +1,120 @@ +// +// Amount.swift +// +// +// Created by Pacu on 2024-01-01. +// + +import Foundation +import BigDecimal +/// An *non-negative* decimal ZEC amount represented as specified in ZIP-321. +/// Amount can be from 1 zatoshi (0.00000001) to the `maxSupply` of 21M ZEC (`21_000_000`) +public struct Amount: Equatable { + public enum AmountError: Error { + case negativeAmount + case greaterThanSupply + case tooManyFractionalDigits + case invalidTextInput + } + + static let maxFractionalDecimalDigits: Int = 8 + + static let zecRounding = Rounding(.toNearestOrEven, maxFractionalDecimalDigits) + + static let decimalHandler = NSDecimalNumberHandler( + roundingMode: NSDecimalNumber.RoundingMode.bankers, + scale: Int16(Self.maxFractionalDecimalDigits), + raiseOnExactness: true, + raiseOnOverflow: true, + raiseOnUnderflow: true, + raiseOnDivideByZero: true + ) + + static let maxSupply: BigDecimal = 21_000_000 + + static let zero = Amount(unchecked: 0) + + let value: BigDecimal + + /// Initializes an Amount from a `Decimal` number + /// - parameter value: decimal representation of the desired amount. **Important:** `Decimal` values with more than 8 fractional digits ** will be rounded** using bankers rounding. + /// - returns A valid ZEC amount + /// - throws `Amount.AmountError` then the provided value can't represent or can't be rounded to a non-negative ZEC decimal amount. + public init(value: Double) throws { + guard value >= 0 else { throw AmountError.negativeAmount } + + guard value <= Self.maxSupply.asDouble() else { throw AmountError.greaterThanSupply } + + try self.init(decimal: BigDecimal(value).round(Self.zecRounding)) + } + + /// Initializes an Amount from a `BigDecimal` number + /// - parameter decimal: decimal representation of the desired amount. **Important:** `Decimal` values with more than 8 fractional digits ** will be rounded** using bankers rounding. + /// - returns A valid ZEC amount + /// - throws `Amount.AmountError` then the provided value can't represent or can't be rounded to a non-negative ZEC decimal amount. + public init(decimal: BigDecimal) throws { + guard decimal >= 0 else { throw AmountError.negativeAmount } + + guard decimal <= Self.maxSupply else { throw AmountError.greaterThanSupply } + + guard decimal.significantFractionalDecimalDigits <= Self.maxFractionalDecimalDigits else { + throw AmountError.tooManyFractionalDigits + } + + guard decimal <= Self.maxSupply else { throw AmountError.greaterThanSupply } + + self.value = decimal + } + + /// Initializes an Amount from a `BigDecimal` number + /// - parameter decimal: decimal representation of the desired amount. **Important:** `Decimal` values with more than 8 fractional digits ** will be rounded** using bankers rounding. + /// - returns A valid ZEC amount + /// - throws `Amount.AmountError` then the provided value can't represent or can't be rounded to a non-negative ZEC decimal amount. + public init(decimal: Decimal) throws { + guard decimal >= 0 else { throw AmountError.negativeAmount } + + guard decimal.significantFractionalDecimalDigits <= Self.maxFractionalDecimalDigits else { + throw AmountError.tooManyFractionalDigits + } + + guard decimal <= Self.maxSupply.asDecimal() else { throw AmountError.greaterThanSupply } + + self.value = BigDecimal(decimal).round(Self.zecRounding) + } + + public init(string: String) throws { + let decimalAmount = BigDecimal(string).trim + + guard !decimalAmount.isNaN else { + throw AmountError.invalidTextInput + } + + guard decimalAmount.significantFractionalDecimalDigits <= Self.maxFractionalDecimalDigits else { + throw AmountError.tooManyFractionalDigits + } + + try self.init(decimal: decimalAmount) + } + + init(unchecked: BigDecimal) { + self.value = unchecked + } + + public func toString() -> String { + let decimal = value.round(Rounding(.toNearestOrEven, Self.maxFractionalDecimalDigits)).trim + + return decimal.asString(.plain) // this value is already validated. + } +} + +extension BigDecimal { + var significantFractionalDecimalDigits: Int { + return max(-exponent, 0) + } +} + +extension Decimal { + var significantFractionalDecimalDigits: Int { + return max(-exponent, 0) + } +} diff --git a/Sources/zcash-swift-payment-uri/Model.swift b/Sources/zcash-swift-payment-uri/Model.swift index 6a52d54..7003f4b 100644 --- a/Sources/zcash-swift-payment-uri/Model.swift +++ b/Sources/zcash-swift-payment-uri/Model.swift @@ -49,70 +49,6 @@ public struct OtherParam: Equatable { public let value: String } -/// An *non-negative* decimal ZEC amount represented as specified in ZIP-321. -/// Amount can be from 1 zatoshi (0.00000001) to the `maxSupply` of 21M ZEC (`21_000_000`) -public struct Amount: Equatable { - public enum AmountError: Error { - case negativeAmount - case greaterThanSupply - case tooManyFractionalDigits - case invalidTextInput - } - - static let maxFractionalDecimalDigits: Int16 = 8 - static let decimalHandler = NSDecimalNumberHandler( - roundingMode: NSDecimalNumber.RoundingMode.bankers, - scale: Self.maxFractionalDecimalDigits, - raiseOnExactness: true, - raiseOnOverflow: true, - raiseOnUnderflow: true, - raiseOnDivideByZero: true - ) - - static let maxSupply: Decimal = 21_000_000 - - static let zero = Amount(unchecked: 0) - - let value: Decimal - /// Initializes an Amount from a `Decimal` number - /// - parameter value: decimal representation of the desired amount. **Important:** `Decimal` values with more than 8 fractional digits ** will be rounded** using bankers rounding. - /// - returns A valid ZEC amount - /// - throws `Amount.AmountError` then the provided value can't represent or can't be rounded to a non-negative ZEC decimal amount. - public init(value: Decimal) throws { - guard value >= 0 else { throw AmountError.negativeAmount } - - guard value <= Self.maxSupply else { throw AmountError.greaterThanSupply } - - self.value = value - } - - public init(string: String) throws { - let formatter = NumberFormatter.zcashNumberFormatter - - guard let decimalAmount = formatter.number(from: string)?.decimalValue else { - throw AmountError.invalidTextInput - } - - guard decimalAmount.significantFractionalDecimalDigits <= Self.maxFractionalDecimalDigits else { - throw AmountError.tooManyFractionalDigits - } - - try self.init(value: decimalAmount) - } - - init(unchecked: Decimal) { - self.value = unchecked - } - - public func toString() -> String { - let formatter = NumberFormatter.zcashNumberFormatter - - let decimal = NSDecimalNumber(decimal: self.value) - - return formatter.string(from: decimal.rounding(accordingToBehavior: Self.decimalHandler)) ?? "" // this value is already validated. - } -} - public struct MemoBytes: Equatable { public enum MemoError: Error { case memoTooLong @@ -185,17 +121,12 @@ extension NumberFormatter { formatter.numberStyle = .decimal formatter.usesGroupingSeparator = false formatter.decimalSeparator = "." - + formatter.roundingMode = .halfUp + return formatter }() } -extension Decimal { - var significantFractionalDecimalDigits: Int { - return max(-exponent, 0) - } -} - extension String.StringInterpolation { mutating func appendInterpolation(_ value: Amount) { appendLiteral(value.toString()) diff --git a/Sources/zcash-swift-payment-uri/Parser.swift b/Sources/zcash-swift-payment-uri/Parser.swift index 72de11e..79eedfc 100644 --- a/Sources/zcash-swift-payment-uri/Parser.swift +++ b/Sources/zcash-swift-payment-uri/Parser.swift @@ -238,7 +238,7 @@ enum Parser { } } - return try paramsByIndex.compactMap { + return try paramsByIndex.map { try Payment.uniqueIndexedParameters(index: $0, parameters: $1) } } diff --git a/Tests/zcash-swift-payment-uriTests/AmountTests.swift b/Tests/zcash-swift-payment-uriTests/AmountTests.swift index f9911a3..c62ba46 100644 --- a/Tests/zcash-swift-payment-uriTests/AmountTests.swift +++ b/Tests/zcash-swift-payment-uriTests/AmountTests.swift @@ -6,12 +6,17 @@ // import XCTest +import BigDecimal @testable import zcash_swift_payment_uri final class AmountTests: XCTestCase { func testAmountStringDecimals() throws { XCTAssertEqual(try Amount(value: 123.456).toString(), "123.456") XCTAssertEqual("\(try Amount(value: 123.456))", "123.456") + + let stringDecimal = try Amount(string: "123.456") + let literalDecimal = try Amount(value: 123.456) + XCTAssertEqual(stringDecimal, literalDecimal) } func testAmountTrailing() throws { @@ -27,7 +32,9 @@ final class AmountTests: XCTestCase { } func testAmountThrowsIfMaxSupply() throws { + XCTAssertThrowsError(try Amount(decimal: BigDecimal(21_000_000.00000001)).toString()) XCTAssertThrowsError(try Amount(value: 21_000_000.00000001).toString()) + XCTAssertThrowsError(try Amount(string: "21_000_000.00000001").toString()) } func testAmountThrowsIfNegativeAmount() throws { diff --git a/Tests/zcash-swift-payment-uriTests/ParsingTests.swift b/Tests/zcash-swift-payment-uriTests/ParsingTests.swift index 2d4f967..2669dbd 100644 --- a/Tests/zcash-swift-payment-uriTests/ParsingTests.swift +++ b/Tests/zcash-swift-payment-uriTests/ParsingTests.swift @@ -44,7 +44,7 @@ final class ParsingTests: XCTestCase { payments: [ Payment( recipientAddress: recipient, - amount: Amount(unchecked: 1.0001), + amount: try Amount(string:"1.0001"), memo: nil, label: nil, message: "lunch", @@ -560,7 +560,7 @@ final class ParsingTests: XCTestCase { let value = "1.00020112"[...] XCTAssertEqual( - IndexedParameter(index: 0, param: .amount(try Amount(value: 1.00020112))), + IndexedParameter(index: 0, param: .amount(try Amount(string: String(value)))), try Parser.zcashParameter((query, nil, value), validating: Parser.onlyCharsetValidation) ) } @@ -944,4 +944,4 @@ final class ParsingTests: XCTestCase { } } } -// swiftlint:enable line_length + diff --git a/Tests/zcash-swift-payment-uriTests/RendererTests.swift b/Tests/zcash-swift-payment-uriTests/RendererTests.swift index 6b11ba4..7352000 100644 --- a/Tests/zcash-swift-payment-uriTests/RendererTests.swift +++ b/Tests/zcash-swift-payment-uriTests/RendererTests.swift @@ -11,7 +11,7 @@ final class RendererTests: XCTestCase { func testAmountRendersNoParamIndex() throws { let expected = "amount=123.456" - let amount = try Amount(value: Decimal(123.456)) + let amount = try Amount(string: "123.456") XCTAssertEqual(Render.parameter(amount, index: nil), expected) @@ -21,7 +21,7 @@ final class RendererTests: XCTestCase { func testAmountRendersWithParamIndex() throws { let expected = "amount.1=123.456" - let amount = try Amount(value: Decimal(123.456)) + let amount = try Amount(string: "123.456") XCTAssertEqual(Render.parameter(amount, index: 1), expected) } diff --git a/Tests/zcash-swift-payment-uriTests/ZIP321Tests.swift b/Tests/zcash-swift-payment-uriTests/ZIP321Tests.swift index 51fda24..e3c38b3 100644 --- a/Tests/zcash-swift-payment-uriTests/ZIP321Tests.swift +++ b/Tests/zcash-swift-payment-uriTests/ZIP321Tests.swift @@ -91,4 +91,50 @@ final class ZcashSwiftPaymentUriTests: XCTestCase { XCTAssertEqual(ZIP321.uriString(from: paymentRequest, formattingOptions: .useEmptyParamIndex(omitAddressLabel: false)), expected) } + + func testParsingMultiplePaymentsRequestStartingWithNoParamIndex() throws { + let uriString = "zcash:?address=tmEZhbWHTpdKMw5it8YDspUXSMGQyFwovpU&amount=123.456&address.1=ztestsapling10yy2ex5dcqkclhc7z7yrnjq2z6feyjad56ptwlfgmy77dmaqqrl9gyhprdx59qgmsnyfska2kez&amount.1=0.789&memo.1=VGhpcyBpcyBhIHVuaWNvZGUgbWVtbyDinKjwn6aE8J-PhvCfjok" + + let address0 = "tmEZhbWHTpdKMw5it8YDspUXSMGQyFwovpU" + + guard let recipient0 = RecipientAddress(value: address0) else { + XCTFail("failed to create recipient without validation for address: \(address0)") + return + } + + let payment0 = Payment( + recipientAddress: recipient0, + amount: try Amount(value: 123.456), + memo: nil, + label: nil, + message: nil, + otherParams: nil + ) + + let address1 = "ztestsapling10yy2ex5dcqkclhc7z7yrnjq2z6feyjad56ptwlfgmy77dmaqqrl9gyhprdx59qgmsnyfska2kez" + + guard let recipient1 = RecipientAddress(value: address1) else { + XCTFail("failed to create recipient without validation for address: \(address1)") + return + } + + let payment1 = Payment( + recipientAddress: recipient1, + amount: try Amount(value: 0.789), + memo: try MemoBytes(utf8String: "This is a unicode memo ✨🦄🏆🎉"), + label: nil, + message: nil, + otherParams: nil + ) + + let paymentRequest = PaymentRequest(payments: [payment0, payment1]) + + let result = try ZIP321.request(from: uriString) + + + XCTAssertEqual( + result, + ParserResult.request(paymentRequest) + ) + } }