Fixes #474 #535
Fixes #474 #535
Conversation
| return ofDrops(UnsignedLong.ZERO); | ||
| } | ||
|
|
||
| // Whether positive or negative, clamp the amount to 0 if it's too small, or 100B if it's too big. |
There was a problem hiding this comment.
@nkramer44 I need to fix this comment (the code isn't clamping right now, but is instead enforcing preconditions like the old code. I think that's the right choice, in order to keep things functioning "as-is" but we've been bit so many times by invalid assumptions around numbers, I'll pose the question here.
There was a problem hiding this comment.
I think leaving preconditions here is ok for one main reason, which is that this method is never invoked during serialization, and will only be invoked by a developer. If, for some reason, they need to create an XrpCurrencyAmount that is smaller than the smallest XRP or larger than the largest XRP amount, they can just call ofDrops or of
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return this.value().toString(); | ||
| if (isNegative()) { | ||
| return "-" + this.value().toString(); |
There was a problem hiding this comment.
@nkramer44 WDYT about this one (i.e., printing a negative value)? In this case, I do this because XrpCurrencyAmount always holds an UnsignedLong (the isNegative() function is use to detect negativity since the Unsigned value can't hold a negative).
One could argue that this toString should always show a positive value, to conform with the UnsignedLong underlying.
Note: The details informing this choice are subtly different in the case of IssueCurrencyAmount.
There was a problem hiding this comment.
I think this is the right thing to do. If you deserialize one of those payloads that has a negative XRP amount and then call toString on that amount, it would be misleading/incorrect to not show a - before the number IMO
| .build(); | ||
| assertThat(issuedCurrency.toString()).isEqualTo( | ||
| "IssuedCurrencyAmount{" + | ||
| "value=-1.00, " + |
There was a problem hiding this comment.
See my comment above about whether this should emit as a negative string, or a positive one. In rippled JSON RPC responses, these emit as negative values (there is no isNegative in there) so from that perspective, this format is more aligned. However, it does feel weird to have a negative number with an isNegative property.
Maybe we exclude isNegative from the JSON of IssuedCurrencyAmount and that will make things clearer.
There was a problem hiding this comment.
I wonder if isNegative should live in CurrencyAmount or if it should just be a property of XrpCurrencyAmount...
If we want to keep it in CurrencyAmount, I think it's fine to include it in the toString implementation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #535 +/- ##
============================================
- Coverage 91.65% 91.64% -0.01%
- Complexity 1774 1776 +2
============================================
Files 365 365
Lines 4948 4981 +33
Branches 408 417 +9
============================================
+ Hits 4535 4565 +30
- Misses 280 281 +1
- Partials 133 135 +2 ☔ View full report in Codecov by Sentry. |
| @@ -213,8 +275,13 @@ public static XrpCurrencyAmount ofXrp(BigDecimal amount) { | |||
| * @return A {@link BigDecimal} representing this value denominated in whole XRP units. | |||
| */ | |||
| public BigDecimal toXrp() { | |||
| return new BigDecimal(this.value().bigIntegerValue()) | |||
There was a problem hiding this comment.
Was this a bug? If value() is drops, we should have been scaling by 10^-6, right?
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return this.value().toString(); | ||
| if (isNegative()) { | ||
| return "-" + this.value().toString(); |
There was a problem hiding this comment.
I think this is the right thing to do. If you deserialize one of those payloads that has a negative XRP amount and then call toString on that amount, it would be misleading/incorrect to not show a - before the number IMO
| .build(); | ||
| assertThat(issuedCurrency.toString()).isEqualTo( | ||
| "IssuedCurrencyAmount{" + | ||
| "value=-1.00, " + |
There was a problem hiding this comment.
I wonder if isNegative should live in CurrencyAmount or if it should just be a property of XrpCurrencyAmount...
If we want to keep it in CurrencyAmount, I think it's fine to include it in the toString implementation.
Fixes #474 by adding the
isNegativefunctionCurrencyAmount, and then properly supporting negative value detection in XRP and IssuedCurrency amounts.This fix should also fix issue #527.