1.5.0 Hot fixes #1070
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,8 @@ extension TokenPricesClient { | |
| @Dependency(\.jsonDecoder) var jsonDecoder | ||
| @Dependency(\.jsonEncoder) var jsonEncoder | ||
| @Dependency(\.cacheClient) var cacheClient | ||
|
There was a problem hiding this comment. Opt to always use prod token price service to facilitate testing. Dev endpoint might not have all prices available or latest ones. |
||
|
|
||
| let rootURL = URL(string: "https://token-price-service.radixdlt.com")! | ||
|
|
||
| @Sendable | ||
| func getTokenPrices(_ fetchRequest: FetchPricesRequest) async throws -> TokenPrices { | ||
|
|
@@ -43,12 +40,14 @@ extension TokenPricesClient { | |
| } | ||
|
|
||
| extension TokenPricesClient.TokenPrices { | ||
| public init(_ tokenPricesResponse: TokensPriceResponse) { | ||
| let formatter = NumberFormatter() | ||
|
There was a problem hiding this comment. Would be nice to have a RETDecimal initializer in Sargon to handle the case of providing a Double with more than 18 decimal places. |
||
| formatter.locale = Locale(identifier: "en_US_POSIX") // Just ignore the users locale | ||
| formatter.numberStyle = .decimal | ||
| formatter.maximumFractionDigits = Int(RETDecimal.maxDivisibility) | ||
| formatter.roundingMode = .down | ||
| formatter.decimalSeparator = "." // Enforce dot notation for RETDecimal | ||
| formatter.usesGroupingSeparator = false // No grouping separator for RETDecimal | ||
|
There was a problem hiding this comment. Not having this caused the prices > 1000 to be formatted with a thousand separator which then couldn't be converted to RETDecimal. There was a problem hiding this comment. there is another initialiser than There was a problem hiding this comment. It looks like we are rounding the Double that represents the price, by first converting the price Double to a string (using a fixed decimal separator) and then converting it back to RETDecimal?
There was a problem hiding this comment.
Presumably (hopefully), the latter respects the user's locale, and the former does not, and in this case we don't want to respect the locale, since we hard code the decimal separator. There was a problem hiding this comment. @kugel3 note that this whole shenanigans is not about rounding, it is about truncating to RETDecimal max divisibility, otherwise we cannot initialise the RETDecimal with a double that has more decimals than 18. The price is double as it comes from the TokenPrice service, here we do immediately convert it to RETDecimal. There was a problem hiding this comment. Alright, then I understand your other comment about an initialiser for RETDecimal that takes a Double with arbitrary number of decimals, that would be highly preferable. Since NumberFormatter is pretty expensive to initialise, we probably don't want to make that initialiser in Swift. But while we have this workaround, I think we should be more explicit about what is going on, so that nobody accidentally changes to using the other string based initialiser for example. Because Perhaps change There was a problem hiding this comment. Yes |
||
|
|
||
| self = tokenPricesResponse.tokens.reduce(into: [:]) { partialResult, next in | ||
| let trimmed = formatter.string(for: next.price) ?? "" | ||
|
|
@@ -60,19 +59,28 @@ extension TokenPricesClient.TokenPrices { | |
| } | ||
|
|
||
| // MARK: - TokensPriceResponse | ||
| public struct TokensPriceResponse: Decodable { | ||
| public let tokens: [TokenPrice] | ||
|
|
||
| public init(tokens: [TokenPrice]) { | ||
| self.tokens = tokens | ||
| } | ||
| } | ||
|
|
||
| // MARK: TokensPriceResponse.TokenPrice | ||
| extension TokensPriceResponse { | ||
| public struct TokenPrice: Decodable { | ||
| enum CodingKeys: String, CodingKey { | ||
| case resourceAddress = "resource_address" | ||
| case price = "usd_price" | ||
| } | ||
|
|
||
| public let resourceAddress: ResourceAddress | ||
| public let price: Double | ||
|
|
||
| public init(resourceAddress: ResourceAddress, price: Double) { | ||
| self.resourceAddress = resourceAddress | ||
| self.price = price | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,3 +74,16 @@ extension NonFungibleLocalId { | |
| } | ||
| } | ||
| } | ||
|
|
||
| extension TXID { | ||
|
There was a problem hiding this comment. Aha! I suspected we had a regression, because I started adding this in Sargon for transaction hashes but then tested main branch and saw we did not truncated. I should have spoken up :) @micbakos-rdx we can you add a PR implementing this in Sargon? |
||
| public func formatted(_ format: AddressFormat = .default) -> String { | ||
| let str = asStr() | ||
|
|
||
| switch format { | ||
| case .default: | ||
| return str.truncatedMiddle(keepFirst: 4, last: 6) | ||
| case .full, .raw: | ||
| return str | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,10 +3,6 @@ import EngineToolkit | |
| public typealias TXID = TransactionHash | ||
|
|
||
| extension TXID { | ||
|
There was a problem hiding this comment. @GhenadieVP merge conflict imminent, but I see, it must happen, since you needed to implement this a method with this signature. |
||
| public var hex: String { | ||
| bytes().hex() | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| @testable import Radix_Wallet_Dev | ||
| import XCTest | ||
|
|
||
| final class TokenPriceCientTests: XCTestCase { | ||
|
|
||
| func test_zeroPrice() { | ||
| validateDecimalPriceConversion(0, expected: .zero()) | ||
| } | ||
|
|
||
| func test_noDecimalPlaces_1() { | ||
| validateDecimalPriceConversion(10, expected: 10) | ||
| } | ||
|
|
||
| func test_noDecimalPlaces_2() { | ||
| validateDecimalPriceConversion(10000, expected: 10000) | ||
| } | ||
|
|
||
| func test_noDecimalPlaces_3() { | ||
| validateDecimalPriceConversion(10_000_000, expected: 10_000_000) | ||
| } | ||
|
|
||
| func test_withDecimalPlaces_1() { | ||
| validateDecimalPriceConversion(1.99, expected: try! .init(value: "1.99")) | ||
| } | ||
|
|
||
| func test_withDecimalPlaces_2() { | ||
| validateDecimalPriceConversion(1.000099, expected: try! .init(value: "1.000099")) | ||
| } | ||
|
|
||
| func test_belowOne_1() { | ||
| validateDecimalPriceConversion(0.99, expected: 0.99) | ||
| } | ||
|
|
||
| func test_belowOne_2() { | ||
| validateDecimalPriceConversion(0.000099, expected: try! .init(value: "0.000099")) | ||
| } | ||
|
|
||
| // NOTE: All of the below values would be rounded to 14 decimal places | ||
| // As it seems that Swift number formatter for Double cannot express more decimals. | ||
| func test_closeToRETDecimalDivisibility() { | ||
| // 17 decimal places | ||
| validateDecimalPriceConversion(1.12345678901234567, expected: try! .init(value: "1.12345678901235")) | ||
| } | ||
|
|
||
| func test_maxRETDecimalDivisibility() { | ||
| // 18 decimal places | ||
| validateDecimalPriceConversion(1.123456789012345678, expected: try! .init(value: "1.12345678901235")) | ||
| } | ||
|
|
||
| func test_overMaxRETDecimalDivisibility() { | ||
| // 22 decimal places | ||
| validateDecimalPriceConversion(1.1234567890123456789012, expected: try! .init(value: "1.12345678901235")) | ||
| } | ||
|
|
||
| private func validateDecimalPriceConversion( | ||
| _ price: Double, | ||
| expected: RETDecimal, | ||
| file: StaticString = #filePath, | ||
| line: UInt = #line | ||
| ) { | ||
| let tokenPrice = tokenWithPrice(price) | ||
| guard let decimalPrice = TokenPricesClient.TokenPrices(tokenPrice).first?.value else { | ||
| XCTFail("Could'nt convert \(tokenPrice) to RETDecimal", file: file, line: line) | ||
| return | ||
| } | ||
| XCTAssertEqual( | ||
| decimalPrice, | ||
| expected, | ||
| "expected \(expected.formattedPlain()), got \(decimalPrice.formattedPlain())", | ||
| file: file, | ||
| line: line | ||
| ) | ||
| } | ||
|
|
||
| private func tokenWithPrice(_ price: Double) -> TokensPriceResponse { | ||
| TokensPriceResponse(tokens: [ | ||
| TokensPriceResponse.TokenPrice( | ||
| resourceAddress: try! .init(validatingAddress: "resource_rdx1tknxxxxxxxxxradxrdxxxxxxxxx009923554798xxxxxxxxxradxrd"), | ||
| price: price | ||
| ), | ||
| ]) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was causing users to see tokens with zero values when they did performa pull to refresh in Account details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is done here, will that not affect all the endpoints on the client? In transaction history I need the full portfolio, including empty vaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right should be able to do it later, in view layer is Gustafs meaning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably will it will,
will move this at UI level