Fix: RPC Response #5503
Fix: RPC Response #5503
Conversation
d0bfd6a to
e07208b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5503 +/- ##
=========================================
- Coverage 79.1% 79.1% -0.0%
=========================================
Files 817 817
Lines 71704 71703 -1
Branches 8261 8239 -22
=========================================
- Hits 56725 56715 -10
- Misses 14979 14988 +9
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
bcd6106 to
9f961ea
Compare
src/xrpld/app/tx/detail/Batch.cpp
Outdated
| @@ -61,7 +58,7 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx) | |||
|
|
|||
| // LCOV_EXCL_START | |||
| if (baseFee > maxAmount - view.fees().base) | |||
| throw std::overflow_error("XRPAmount overflow"); | |||
| return INITIAL_XRP; | |||
There was a problem hiding this comment.
If it's related to a bug fix and it's covered by unit tests, can we remove LCOV_EXCL_START around it?
There was a problem hiding this comment.
If it is tested and covered yes we can remove the LCOV_EXCL_START but I believe none of these are tested except nested ttBATCH.
src/xrpld/app/tx/detail/Batch.cpp
Outdated
| * transaction. | ||
| * @param view The ledger view used for fee calculations | ||
| * @param tx The batch transaction for which to calculate fees | ||
| * @return XRPAmount The total fee required for the batch transaction | ||
| */ | ||
| XRPAmount | ||
| Batch::calculateBaseFee(ReadView const& view, STTx const& tx) |
There was a problem hiding this comment.
I have two concerns about this method:
- We are losing the error context because all error paths with return
INITIAL_XRPwill be indistinguishable from each other. - Returning the maximum number of XRP is a new pattern to return errors, and I haven't seen it being used anywhere else.
There was a problem hiding this comment.
Updated back to throw
There was a problem hiding this comment.
After reviewing invoke_calculateBaseFee I don't believe it was ever meant to throw, a try was added but only for supporting a transaction.macro error. Thats why I recommended returning only an amount. The reason for INITIAL_XRP and not 0 is because if there was a bug, then 0 can make it into a ledger, INITIAL_XRP cannot.
Would you like me to add debugLog to the XRPAmount returns or throw errors?
There was a problem hiding this comment.
@vlntb I have changed it back to not throwing and INITIAL_XRP. This is imo how this should be handled. A zero fee here could allow a transaction into the ledger where INITIAL_XRP will not.
I also added some unit tests for calculateBaseFee.
There was a problem hiding this comment.
I see what you mean: 0 or -1 can be treated as a valid amount, while INITIAL_XRP is guaranteed to fail.
Added error logs will help to differentiate between different reasons for failure. LGTM
18344d7 to
3e5adaf
Compare
| @@ -258,6 +258,11 @@ invoke_calculateBaseFee(ReadView const& view, STTx const& tx) | |||
| "ripple::invoke_calculateBaseFee : unknown transaction type"); | |||
| return XRPAmount{0}; | |||
| } | |||
| catch (std::exception const& e) | |||
| { | |||
There was a problem hiding this comment.
If this is how we're solving the issue, should this be covered by unit tests
There was a problem hiding this comment.
I reverted to the original, returning XRPAmount. I added some unit tests.
3e5adaf to
8b42411
Compare
8b42411 to
7b251eb
Compare
High Level Overview of Change
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)