Skip to content
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

Merged
merged 16 commits into from
Nov 4, 2024
Merged

Conversation

yinyiqian1
Copy link
Collaborator

@yinyiqian1 yinyiqian1 commented Sep 19, 2024

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)

@yinyiqian1 yinyiqian1 marked this pull request as draft September 19, 2024 04:38
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.6%. Comparing base (d6dbf0e) to head (25f9ba6).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5142     +/-   ##
=========================================
+ Coverage     77.5%   77.6%   +0.1%     
=========================================
  Files          777     779      +2     
  Lines        65833   65975    +142     
  Branches      8182    8173      -9     
=========================================
+ Hits         51031   51180    +149     
+ Misses       14802   14795      -7     
Files with missing lines Coverage Δ
include/xrpl/protocol/Feature.h 100.0% <ø> (ø)
include/xrpl/protocol/detail/transactions.macro 100.0% <100.0%> (ø)
src/libxrpl/protocol/TER.cpp 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.5% <100.0%> (+0.1%) ⬆️
src/xrpld/app/tx/detail/AMMDeposit.cpp 95.3% <100.0%> (-0.1%) ⬇️
src/xrpld/app/tx/detail/AMMWithdraw.cpp 95.9% <100.0%> (+4.5%) ⬆️
src/xrpld/app/tx/detail/AMMWithdraw.h 100.0% <ø> (ø)
src/xrpld/app/tx/detail/InvariantCheck.cpp 86.5% <100.0%> (+<0.1%) ⬆️
... and 1 more

... and 5 files with indirect coverage changes

Impacted file tree graph

@mvadari
Copy link
Collaborator

mvadari commented Sep 19, 2024

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

@yinyiqian1 yinyiqian1 changed the title Add AMMClawback Transaction (XLS-0074d) Add AMMClawback Transaction (XLS-0073d) Sep 19, 2024
@yinyiqian1
Copy link
Collaborator Author

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

@yinyiqian1 yinyiqian1 changed the title Add AMMClawback Transaction (XLS-0073d) [Do not review] Add AMMClawback Transaction (XLS-0073d) Sep 19, 2024
@yinyiqian1 yinyiqian1 force-pushed the amm-clawback branch 2 times, most recently from ad78c7c to a8f9c1b Compare September 19, 2024 17:28
@yinyiqian1 yinyiqian1 changed the title [Do not review] Add AMMClawback Transaction (XLS-0073d) Add AMMClawback Transaction (XLS-0073d) Sep 19, 2024
@yinyiqian1 yinyiqian1 marked this pull request as ready for review September 23, 2024 16:36
include/xrpl/protocol/jss.h Outdated Show resolved Hide resolved
include/xrpl/protocol/SField.h Outdated Show resolved Hide resolved
include/xrpl/protocol/TER.h Outdated Show resolved Hide resolved
src/libxrpl/protocol/TER.cpp Outdated Show resolved Hide resolved
src/xrpld/app/tx/detail/AMMDeposit.cpp Outdated Show resolved Hide resolved
src/xrpld/app/tx/detail/AMMClawback.cpp Outdated Show resolved Hide resolved
src/xrpld/app/tx/detail/AMMClawback.cpp Outdated Show resolved Hide resolved
src/xrpld/app/tx/detail/AMMWithdraw.h Show resolved Hide resolved
src/xrpld/app/tx/detail/AMMWithdraw.cpp Show resolved Hide resolved
src/test/jtx/impl/AMM.cpp Show resolved Hide resolved
@kennyzlei kennyzlei requested review from dangell7 and removed request for shawnxie999 September 26, 2024 17:41
src/test/app/AMM_test.cpp Outdated Show resolved Hide resolved
src/test/app/AMMClawback_test.cpp Outdated Show resolved Hide resolved
src/test/app/AMMClawback_test.cpp Outdated Show resolved Hide resolved
@yinyiqian1 yinyiqian1 force-pushed the amm-clawback branch 3 times, most recently from 5381c04 to 51df02e Compare September 27, 2024 14:57
src/test/app/AMM_test.cpp Outdated Show resolved Hide resolved
src/test/app/AMM_test.cpp Outdated Show resolved Hide resolved
@yinyiqian1 yinyiqian1 force-pushed the amm-clawback branch 2 times, most recently from 8c2a8b4 to 369d626 Compare October 14, 2024 19:25
@yinyiqian1 yinyiqian1 added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Nov 1, 2024
@yinyiqian1 yinyiqian1 added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Nov 4, 2024
@ximinez
Copy link
Collaborator

ximinez commented Nov 4, 2024

@yinyiqian1 Are we happy with the commit message being just

Add AMMClawback Transaction (XLS-0073d) (#5142)

?

If not, please suggest the details you'd like to include.

@yinyiqian1
Copy link
Collaborator Author

yinyiqian1 commented Nov 4, 2024

@yinyiqian1 Are we happy with the commit message being just

Add AMMClawback Transaction (XLS-0073d) (#5142)

?

If not, please suggest the details you'd like to include.

@ximinez I think it's fine that we keep the current message

@ximinez ximinez merged commit 54a350b into XRPLF:develop Nov 4, 2024
20 checks passed
@intelliot intelliot added this to the 2.3.0 (Nov 2024) milestone Nov 4, 2024
This was referenced Nov 6, 2024
vlntb pushed a commit to vlntb/rippled that referenced this pull request Nov 19, 2024
Amendment:
- AMMClawback

New Transactions:
- AMMClawback

Modified Transactions:
- AMMCreate
- AMMDeposit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

7 participants