ABW-2249 Transfer Max LSU #759
Conversation
Adopt API changes with hardcoded divisibility (18)
|
@maciek-rdx can you help explain what you have done, why, how, rationale, etc...? :) |
Sure, sorry, I should have marked it as a Draft. |
Sources/EngineKit/ManifestBuilder/ManifestBuilder+CustomInstructions.swift
Show resolved
Hide resolved
| import Prelude | ||
|
|
||
| extension EngineToolkit.Decimal { | ||
| public static let integerAndDecimalPartsSeparator = "." |
There was a problem hiding this comment.
please retain the comment about why we hardcode "." instead of using Locale.current.decimalSeparator:
// N.B. We cannot use `Local.current.decimalSeperator` here because
// `github.com/Zollerbo1/BigDecimal` package **hardcodes** usage of
// the decimal separator ".", see this line here:
// https://github.com/Zollerboy1/BigDecimal/blob/main/Sources/BigDecimal/BigDecimal.swift#L469
There was a problem hiding this comment.
oh wait, you have it in two places...?
There was a problem hiding this comment.
so it ought to be deleted from here?
There was a problem hiding this comment.
The comment that you mentioned does not apply here. These can change for independent reasons. This one is about the dot required by the EngineToolkit.Decimal.init(value:). I can add something in the shape of "Required by EngineToolkit.Decimal to construct it from a String", but for me, the current name (EngineToolkit.Decimal.integerAndDecimalPartsSeparator) is good enough.
However you prefer.
|
|
||
| extension BigDecimal { | ||
| public func asDecimal(withDivisibility divisibility: Int? = nil) throws -> EngineToolkit.Decimal { | ||
| let (integerPart, decimalPart) = integerAndDecimalPart( |
There was a problem hiding this comment.
I wonder if this is not the same as withPrecision(divisibility).removeTrailingZeros?
At the same time, why do we care what is the divisibility of the resource itself?
There was a problem hiding this comment.
I wonder if this is not the same as
withPrecision(divisibility).removeTrailingZeros?
I think in general it's better and simpler to maintain our own logic for that. It's not complicated and we're sure that we do only what's essential for our case without a necessity to rely on and understand anything else.
... why do we care what is the divisibility of the resource itself?
Engine Toolkit will reject numbers that have more decimal digits than divisibility. Ultimately, I think we should solve that problem/do not allow more decimals than divisibility in the UI (what I proposed in the description of the PR).
There was a problem hiding this comment.
... more decimal digits than divisibility.
You mean in the context of say a transaction manifest? that it checks the a given decimal value against divisibility of the resource it refers to?
There was a problem hiding this comment.
Yes, exactly. Here's a message from @0xOmarA that confirms it.
There was a problem hiding this comment.
Ok, I didn't see the explicit message about what I have described above, I mean when constructing the EngineToolkit Decimal there is no resource context, so EngineToolkit cannot know what is the correct divisibility. The only place it could do so, is when creating the instruction for the manifest when we do also pass the resource address along with the decimal value.
but I guess it is ok to take into account the resource divisibility for the scenario when we don't forbid users at UI level to enter more decimals than possible - maybe we should.
There was a problem hiding this comment.
when constructing the EngineToolkit Decimal there is no resource context, so EngineToolkit cannot know what is the correct divisibility. The only place it could do so, is when creating the instruction for the manifest when we do also pass the resource address along with the decimal value.
Ah, I see what you mean. But yeah, ultimately I'd do that in the UI, so there's no surprise to the user. But would rather do that as a separate thing. Thoughts @CyonAlexRDX ?
There was a problem hiding this comment.
It is good enough now, if a resource has 2 as divisibility, even if user enters 10 decimals, in the transaction review they will see the value adjusted to the divisibility, so it is not completely silent operation.
# Conflicts: # Sources/EngineKit/BigDecimal.swift
Jira ticket: paste link here
Description
This PR removes the usage of
BigDecimal.toString(withPrecision:)in the code that translatesBigDecimals intoEngineToolkit.Decimal.Notes
EngineToolkit.Decimalhas a very specific validation criteria and we should rely solely on that. I think also that we may want to propagate that knowledge to the UI layer. Currently we allow numbers of any length in the UI. I know - it's extreme if we want to make a number of1000 digits in integer part and(we actually can't do that ⬅️ - we have a validation logic for that) 1000 digits in decimal part, but still - this can seem like a valid input to the user.This code is also a great candidate to be moved into the Rust library that we're about to create. Maybe its worth to have a Jira ticket or at least a
FIXMEfor that?How to test
Additionally you can test what @GhenadieVP experienced here
Video
RPReplay_Final1694765435.MP4
PR submission checklist