Add AMMClawback Transaction (XLS-0073d)

[yinyiqian1]

High Level Overview of Change

Context of Change

Spec link: XRPLF/XRPL-Standards#212

This PR includes:

1. Allow AMMCreate for clawback-enabled tokens
   changes are in AMMCreate and AMM_test
2. AMMDeposit prohibits depositing if either of the paired tokens is frozen
   changes are in AMMDeposit and AMM_test
3. Added AMMClawback transaction.
   mainly in AMMClawback and AMMClawback_test

---

Some refactor happens because I want to call some functions from AMMWithdraw.
1. Refactor of withdraw:

• Added static withdraw function to return the amountWithdrawActual and amount2WithdrawActual as well.
• private overload withdraw.
• Previously in withdraw function, it checks if (!(ctx_.tx[sfFlags] & (tfWithdrawAll | tfOneAssetWithdrawAll))) to decide if it needs to call adjustAmountsByLPTokens. Because these flags only belong to AMMWithdraw, 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 static equalWithdrawTokens function. And also private overload equalWithdrawTokens

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 and B in the amm pool, when clawing back A, tfClawTwoAssets is used to decide if the paired token B 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 and amount in the AMMClawback request:
amount is used to decide if the issuer wants to clawback all the tokens or not. (if amount is not provided, then clawback all) Imagine if an issuer issues both asset A and B in the amm pool, and the issuer wants to clawback all A and give back all the corresponding B to the holder. Then the issuer just needs to specify A in asset field and leave out amount 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

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Performance (increase or change in throughput and/or latency)
• Tests (you added tests for code that already exists, or your new feature included in this PR)
• Documentation update
• Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• Release

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.2%. Comparing base (bf4a7b6) to head (d3d7292).

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5142     +/-   ##
=========================================
+ Coverage     76.2%   76.2%   +0.1%     
=========================================
  Files          760     762      +2     
  Lines        61568   61697    +129     
  Branches      8126    8110     -16     
=========================================
+ Hits         46898   47043    +145     
+ Misses       14670   14654     -16     

Files with missing lines	Coverage Δ
include/xrpl/protocol/Feature.h	100.0% <ø> (ø)
include/xrpl/protocol/SField.h	100.0% <ø> (ø)
src/libxrpl/protocol/Feature.cpp	94.6% <ø> (ø)
src/libxrpl/protocol/SField.cpp	77.5% <ø> (ø)
src/libxrpl/protocol/TER.cpp	100.0% <ø> (ø)
src/libxrpl/protocol/TxFormats.cpp	100.0% <100.0%> (ø)
src/xrpld/app/tx/detail/AMMClawback.cpp	100.0% <100.0%> (ø)
src/xrpld/app/tx/detail/AMMClawback.h	100.0% <100.0%> (ø)
src/xrpld/app/tx/detail/AMMCreate.cpp	90.3% <100.0%> (+0.1%)	⬆️
src/xrpld/app/tx/detail/AMMDeposit.cpp	95.0% <100.0%> (-0.1%)	⬇️
... and 4 more

... and 4 files with indirect coverage changes

[mvadari]

Note: The PR title is wrong, it should be XLS-73d

[yinyiqian1]

Note: The PR title is wrong, it should be XLS-73d
Just corrected it. Thank you.

[gregtatcam]

Use temBAD_AMOUN and temBAD_ISSUER instead.

[yinyiqian1]

There's a temBAD_AMOUNT already defined for non-positive amount errors.

[gregtatcam]

That's what I mean, don't need to define new ones.

[yinyiqian1]

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.

[gregtatcam]

could just use temMALFORMED then.

[mvadari]

It's also thrown in the escrow code if the asset isn't XRP.

[yinyiqian1]

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.

[mvadari]

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.

[yinyiqian1]

I can change the message for temBAD_AMOUNT to "bad amount provided". What do you think?

[dangell7]

Malformed: Bad amount. would keep it in line with the others.

[gregtatcam]

👍 LGTM