XLS-85d: Token-Enabled Escrows #5185
Conversation
src/test/app/EscrowToken_test.cpp
Outdated
| { | ||
| testMPTEnablement(features); | ||
| // testIOULockup(features); | ||
| // testIOURippleState(features); |
There was a problem hiding this comment.
Why these are disabled?
| XRPAmount A = (amt1.signum() == -1 ? -(amt1.xrp()) : amt1.xrp()); | ||
| XRPAmount B = (amt2.signum() == -1 ? -(amt2.xrp()) : amt2.xrp()); | ||
|
|
||
| XRPAmount finalAmt = A + B; |
There was a problem hiding this comment.
As underlying type of the XRPAmount is signed, this is incorrect (undefined behavior in case of the overflow).
Consider something like
if( amt1.signum() != amt2.signum()) return true;
std::int64_t const A = amt1.xrp().drops();
std::int64_t const B = (std::int64_t)(((std::uint64_t)A) + ((std::uint64_t)amt2.xrp().drops()));
return ((A >= 0) && (B >= A)) || ( (A < 0) && (B <= A)));
| A.setIssue(noIssue()); | ||
| B.setIssue(noIssue()); | ||
|
|
||
| STAmount lhs = divide((A - B) + B, A, noIssue()) - one; |
There was a problem hiding this comment.
The logic behind addition / division for STAmount is a bit complicated to guess if this will work. Please add tests for isAddable function to the STAmount_test.cpp.
|
@oleks-rip Thank you for the initial review. Right now I'm just focusing on the actual core implementation and how it effects MPT and IOU. I will address the nit changes you requested once the core implementation is done. |
| auto const balance = STAmount((*sle)[sfBalance]).xrp(); | ||
| auto const reserve = | ||
| ctx_.view().fees().accountReserve((*sle)[sfOwnerCount] + 1); | ||
| STAmount const amount{ctx_.tx[sfAmount]}; |
There was a problem hiding this comment.
doApply function has become too big. Please consider to move out some part of it to helper functions.
| TER | ||
| EscrowFinish::doApply() | ||
| { | ||
| PaymentSandbox psb(&ctx_.view()); |
There was a problem hiding this comment.
Why sandbox introduced here? Is it for rippleCredit?
There was a problem hiding this comment.
EscrowCreate and EscrowCancel use rippleCredit too, should they be updated with sandbox too?
There was a problem hiding this comment.
yes we should do that actually
| auto const k = keylet::escrow(ctx_.tx[sfOwner], ctx_.tx[sfOfferSequence]); | ||
| auto const slep = ctx_.view().peek(k); | ||
| auto const slep = psb.peek(k); |
There was a problem hiding this comment.
doApply function has become too big. Please consider to move out some part of it to helper functions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5185 +/- ##
========================================
Coverage 78.1% 78.1%
========================================
Files 790 790
Lines 67908 68310 +402
Branches 8230 8237 +7
========================================
+ Hits 53034 53361 +327
- Misses 14874 14949 +75
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| auto const reserve = | ||
| ctx_.view().fees().accountReserve((*sle)[sfOwnerCount] + 1); | ||
| AccountID const issuer = amount.getIssuer(); | ||
|
|
||
| if (balance < reserve) | ||
| if (mSourceBalance < reserve) |
There was a problem hiding this comment.
mSourceBalance differs from mPriorBalance by Fee. Direct replacement for *sle)[sfBalance]).xrp() will be mPriorBalance . Are you sure mSourceBalance is good here?
| std::array<TestAccountData, 8> gwSrcTests = {{ | ||
| // src > dst && src > issuer && dst no trustline | ||
| {Account("gw0"), Account{"alice2"}, false, true}, | ||
| // // src < dst && src < issuer && dst no trustline |
| } | ||
|
|
||
| std::array<TestAccountData, 4> gwDstTests = {{ | ||
| // // // // src > dst && src > issuer && dst has trustline |
High Level Overview of Change
XLS: XRPLF/XRPL-Standards#248
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)