-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add AMMClawback Transaction (XLS-0073d) #5142
base: develop
Are you sure you want to change the base?
Conversation
58b7089
to
384a73c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5142 +/- ##
========================================
Coverage 76.2% 76.2%
========================================
Files 760 762 +2
Lines 61568 61703 +135
Branches 8126 8129 +3
========================================
+ Hits 46898 47029 +131
- Misses 14670 14674 +4
|
Note: The PR title is wrong, it should be XLS-73d |
|
ad78c7c
to
a8f9c1b
Compare
a8f9c1b
to
6bf51d1
Compare
da47fb2
to
6510b72
Compare
6510b72
to
0d88f0c
Compare
0d88f0c
to
9b30183
Compare
4879ffc
to
3be6878
Compare
8a8553d
to
01cbe02
Compare
include/xrpl/protocol/TER.h
Outdated
@@ -139,6 +139,8 @@ enum TEMcodes : TERUnderlyingType { | |||
|
|||
temARRAY_EMPTY, | |||
temARRAY_TOO_LARGE, | |||
temBAD_ASSET_AMOUNT, |
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.
Use temBAD_AMOUN and temBAD_ISSUER instead.
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.
There's a temBAD_AMOUNT already defined for non-positive amount errors.
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.
That's what I mean, don't need to define new ones.
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.
Then the request's error message for this one will be inaccurate: Can only send positive amounts.
But I intended to return error message as Malformed: Amount does not match Asset.
temBAD_AMOUNT is only for positive amount check.
I think defining this new error will benefit if in the future, we need to check if asset
matches with amount
again.
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.
could just use temMALFORMED
then.
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.
It's also thrown in the escrow code if the asset isn't XRP.
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.
in escrow code, if it's using temBAD_AMOUNT
, the error message will be "Can only send positive amounts."
. It does not match with that the asset isn't XRP. I think in escrow, it should just use temMalformed
or define more accurate error type. Maybe we should enrich our error code to return more accurate error messages. Because these errors can be used in the future.
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.
Why not just change the error message, so that it makes sense both in Escrow and here, instead of needing a new error code? I believe changing the error message isn't considered a breaking change/needing an amendment.
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 can change the message for temBAD_AMOUNT
to "bad amount provided". What do you think?
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.
Malformed: Bad amount.
would keep it in line with the others.
5381c04
to
51df02e
Compare
51df02e
to
1f6c180
Compare
1f6c180
to
4bdb019
Compare
99c0d02
to
908318b
Compare
908318b
to
2796e8d
Compare
e2942d3
to
a1e2249
Compare
a1e2249
to
e449a09
Compare
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.
👍 LGTM
f9a094b
to
bc50276
Compare
bc50276
to
9f714bb
Compare
}; | ||
|
||
if (!checkAsset(ctx.tx[sfAsset]) || !checkAsset(ctx.tx[sfAsset2])) | ||
return tecFROZEN; | ||
// if (!checkAsset(ctx.tx[sfAsset]) || !checkAsset(ctx.tx[sfAsset2])) |
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.
Don't need these comments.
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.
updated
return tecFROZEN; | ||
// if (!checkAsset(ctx.tx[sfAsset]) || !checkAsset(ctx.tx[sfAsset2])) | ||
// return tecFROZEN; | ||
if (auto const ter = checkAsset(ctx.tx[sfAsset])) |
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.
Should add a unit-test to check for this updated condition.
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.
just updated. (yesterday it's not ready yet)
f493c03
to
b206991
Compare
src/test/app/AMM_test.cpp
Outdated
@@ -1013,8 +1051,8 @@ struct AMM_test : public jtx::AMMTest | |||
ammAlice.deposit(bob, XRP(10)); | |||
ammAlice.deposit( | |||
bob, | |||
XRP(1'000), | |||
std::nullopt, | |||
gw["USD"](100), |
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.
Shouldn't we keep the original test with XRP and null USD?
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.
accidentally changed it. just updated it back
src/test/app/AMM_test.cpp
Outdated
@@ -7030,55 +7068,57 @@ struct AMM_test : public jtx::AMMTest | |||
run() override | |||
{ | |||
FeatureBitset const all{jtx::supported_amendments()}; | |||
testInvalidInstance(); | |||
testInstanceCreate(); | |||
// testInvalidInstance(); |
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.
You commented out most of the tests.
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.
updated. Now it's ready for review. Sorry for the confusion. Next time I'll create another branch from this amm-clawback for review when it's ready
8c2a8b4
to
369d626
Compare
369d626
to
a15db14
Compare
High Level Overview of Change
Context of Change
Spec link: XRPLF/XRPL-Standards#212
This PR includes:
changes are in
AMMCreate
andAMM_test
changes are in
AMMDeposit
andAMM_test
mainly in
AMMClawback
andAMMClawback_test
Some refactor happens because I want to call some functions from AMMWithdraw.
1. Refactor of
withdraw
:withdraw
function to return theamountWithdrawActual
andamount2WithdrawActual
as well.withdraw
.withdraw
function, it checksif (!(ctx_.tx[sfFlags] & (tfWithdrawAll | tfOneAssetWithdrawAll)))
to decide if it needs to calladjustAmountsByLPTokens
. Because these flags only belong toAMMWithdraw
, to make this function also work for AMMClawback, I changed it by passing enum WithdrawAll.2. Refactor of
equalWithdrawTokens
:Because I want to call this function in
AMMClawback
, I make a public staticequalWithdrawTokens
function. And also private overloadequalWithdrawTokens
3. Added
deleteAMMAccountIfEmpty
Since I need to reuse the logic in AMMWithdraw that when all the tokens are withdrawn, the amm account is deleted, so I added function
deleteAMMAccountIfEmpty
.Reason of adding flag
tfClawTwoAssets
:If the issuer issues both assets
A
andB
in the amm pool, when clawing backA
,tfClawTwoAssets
is used to decide if the paired tokenB
to be clawed back as well or goes back to the holder. If tfClawTwoAssets` is set true, the paired token will be clawed back as well. Otherwise, the paired token goes back to the holder.Reason of adding both
asset
andamount
in theAMMClawback
request:amount
is used to decide if the issuer wants to clawback all the tokens or not. (ifamount
is not provided, then clawback all) Imagine if an issuer issues both assetA
andB
in the amm pool, and the issuer wants to clawback allA
and give back all the correspondingB
to the holder. Then the issuer just needs to specifyA
inasset
field and leave outamount
field.Reasons for setting tfee as 0:
Because we are doing a two-asset withdrawal. tfee is not used.
Reasons for using asset1 and asset2 to navigate amm
To keep the same with other amm transactions and make it simple.
So we require providing asset1 and asset2 in the AMMClawback request.
Type of Change
.gitignore
, formatting, dropping support for older tooling)API Impact
libxrpl
change (any change that may affectlibxrpl
or dependents oflibxrpl
)