1.5.0 Hot fixes #1070

GhenadieVP · 2024-04-03T06:04:22Z

1.5.0 Hot fixes

Properly truncate price before initialising the RETDecimal.
Apply empty vaults when refreshing the portfolio for a given account.
TXID truncation.

FLOOP worth:

Tokens with near zero value are still shown

TXID truncation

GhenadieVP · 2024-04-03T06:04:56Z

RadixWallet/Clients/AccountPortfoliosClient/AccountPortfoliosClient+Live.swift

@@ -133,7 +133,7 @@ extension AccountPortfoliosClient: DependencyKey {
 				cacheClient.removeFolder(.onLedgerEntity(.account(accountAddress.asGeneral)))
 			}

-			let account = try await onLedgerEntitiesClient.getAccount(accountAddress)
+			let account = try await onLedgerEntitiesClient.getAccount(accountAddress).nonEmptyVaults


This was causing users to see tokens with zero values when they did performa pull to refresh in Account details

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.

right should be able to do it later, in view layer is Gustafs meaning?

probably will it will,
will move this at UI level

GhenadieVP · 2024-04-03T06:05:37Z

RadixWallet/Clients/TokenPriceClient/TokenPriceClient+Live.swift

@@ -4,11 +4,8 @@ extension TokenPricesClient {
 		@Dependency(\.jsonDecoder) var jsonDecoder
 		@Dependency(\.jsonEncoder) var jsonEncoder
 		@Dependency(\.cacheClient) var cacheClient
-		#if DEBUG


Opt to always use prod token price service to facilitate testing. Dev endpoint might not have all prices available or latest ones.

GhenadieVP · 2024-04-03T06:06:37Z

RadixWallet/Clients/TokenPriceClient/TokenPriceClient+Live.swift

 		formatter.numberStyle = .decimal
 		formatter.maximumFractionDigits = Int(RETDecimal.maxDivisibility)
 		formatter.roundingMode = .down
-		formatter.decimalSeparator = "." // Enfore dot notation for RETDecimal
+		formatter.decimalSeparator = "." // Enforce dot notation for RETDecimal
+		formatter.usesGroupingSeparator = false // No grouping separator for RETDecimal


Not having this caused the prices > 1000 to be formatted with a thousand separator which then couldn't be converted to RETDecimal.

there is another initialiser than value: String, there is formatted: String as well. which should be able to accept strings with a thousands separator.

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?

I presume that RETDecimal(value: String) is the version that takes a machine readable string, so that it ignores the user preference on decimal separator, because otherwise this wouldn't work, but even so, I don't like this, it doesn't seem nice or necessary to convert the price to a Double, then a String and the RETDecimal, just in order to round it.

If we do want to round it here, can't we convert it (back) to RETDecimal and just round that, without going by way of String?

Why do we want price to be a Double at all? This operation here would be even simpler if it was kept a RETDecimal until it's to be displayed.

Is the behaviour actually correct? What is the purpose of clamping the number of decimals in the price according to the divisibility, isn't the price here given in a fiat currency?

there is another initialiser than value: String, there is formatted: String as well. which should be able to accept strings with a thousands separator.

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.

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

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 RETDecimal(formatted: String) could not actually be used here? It does respect locale, right? @CyonAlexRDX

Perhaps change // Enforce dot notation for RETDecimal to something like // Use a period as decimal separator, for the sake of RETDecimal(formatted: String).

Yes RETDecimal.init(formattedStrinf:locale) is respects locale

GhenadieVP · 2024-04-03T06:58:47Z

RadixWallet/Clients/TokenPriceClient/TokenPriceClient+Live.swift

@@ -45,10 +42,12 @@ extension TokenPricesClient {
 extension TokenPricesClient.TokenPrices {
 	fileprivate init(_ tokenPricesResponse: TokensPriceResponse) {
 		let formatter = NumberFormatter()


Would be nice to have a RETDecimal initializer in Sargon to handle the case of providing a Double with more than 18 decimal places.

yeah I can fix one!

Sajjon · 2024-04-03T07:50:59Z

RadixWallet/Core/FeaturePrelude/AddressView/AddressFormat.swift

@@ -74,3 +74,16 @@ extension NonFungibleLocalId {
 		}
 	}
 }
+
+extension TXID {


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?

Sajjon · 2024-04-03T07:51:20Z

RadixWalletTests/Clients/TokenPriceCientTests/TokenPriceCientTests.swift

+@testable import Radix_Wallet_Dev
+import XCTest
+
+final class TokenPriceCientTests: XCTestCase {


tests! 🙂

Sajjon · 2024-04-03T07:54:16Z

RadixWallet/EngineKit/TXID.swift

@@ -3,10 +3,6 @@ import EngineToolkit
 public typealias TXID = TransactionHash

 extension TXID {
-	public func formatted(_ format: AddressFormat = .default) -> String {


@GhenadieVP merge conflict imminent, but I see, it must happen, since you needed to implement this a method with this signature.

CyonAlexRDX

LGTM !

GhenadieVP added 3 commits April 2, 2024 21:53

fix

1c5dc0b

fix

99c46b1

version bump

6fa04b5

GhenadieVP commented Apr 3, 2024

View reviewed changes

GhenadieVP marked this pull request as ready for review April 3, 2024 06:08

tests

15d1d7c

GhenadieVP commented Apr 3, 2024

View reviewed changes

Sajjon reviewed Apr 3, 2024

View reviewed changes

CyonAlexRDX approved these changes Apr 3, 2024

View reviewed changes

micbakos-rdx mentioned this pull request Apr 3, 2024

Implement Intent/SignedIntent Hash formatted method radixdlt/sargon#56

Merged

wip

d0def7e

GhenadieVP merged commit bc17e07 into main Apr 3, 2024
7 checks passed

GhenadieVP deleted the price_trimm_bugfix branch April 3, 2024 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1.5.0 Hot fixes #1070

1.5.0 Hot fixes #1070

GhenadieVP commented Apr 3, 2024 •

edited

Loading

GhenadieVP Apr 3, 2024

kugel3 Apr 3, 2024

Sajjon Apr 3, 2024

GhenadieVP Apr 3, 2024

GhenadieVP Apr 3, 2024

GhenadieVP Apr 3, 2024

Sajjon Apr 3, 2024

kugel3 Apr 3, 2024

kugel3 Apr 3, 2024

GhenadieVP Apr 3, 2024 •

edited

Loading

kugel3 Apr 3, 2024 •

edited

Loading

CyonAlexRDX Apr 3, 2024

GhenadieVP Apr 3, 2024

Sajjon Apr 3, 2024

Sajjon Apr 3, 2024

Sajjon Apr 3, 2024

Sajjon Apr 3, 2024

CyonAlexRDX left a comment

1.5.0 Hot fixes #1070

1.5.0 Hot fixes #1070

Conversation

GhenadieVP commented Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GhenadieVP Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

kugel3 Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyonAlexRDX left a comment

Choose a reason for hiding this comment

GhenadieVP commented Apr 3, 2024 •

edited

Loading

GhenadieVP Apr 3, 2024 •

edited

Loading

kugel3 Apr 3, 2024 •

edited

Loading