-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support XRPFees amendment #508
base: main
Are you sure you want to change the base?
Conversation
…eFee is hex and BaseFeeDrops is decimal
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #508 +/- ##
============================================
+ Coverage 91.65% 91.66% +0.01%
- Complexity 1774 1781 +7
============================================
Files 365 366 +1
Lines 4948 4957 +9
Branches 408 408
============================================
+ Hits 4535 4544 +9
Misses 280 280
Partials 133 133 ☔ View full report in Codecov by Sentry. |
@Value.Default | ||
@JsonIgnore | ||
default UnsignedInteger referenceFeeUnits() { | ||
return maybeReferenceFeeUnits().orElse(null); |
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 feels like a breaking change.
E.g., imagine an application using an older version of xrpl4j is calling referenceFeeUnits
. Then, that code upgrades to the version of xrpl4j that has this change (but the application usage of xrpl4j isn't changed because there's just a deprecation warning). If that application starts talking to a server with XRPFees
amendment enabled, that code will throw a NPE likely.
I need to think more about this one -- unless the argument is that what I wrote above can't happen? Or maybe just is unlikely to happen?
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.
I think you're right. Anyone using this method and not checking for null will get a NPE if XRPFees is enabled. Unfortunately I can't think of a way to support the XRPFees change without making this @Nullable
. We could @Value.Default
it to UnsignedInteger.ZERO
to avoid creating NPE, but that almost seems harder to catch as a developer than getting a bunch of NPE in your logs.
Adjusts the
SetFee
pseudo-transaction to work when theXRPFees
amendment goes live.