Amendment `fixFrozenLPTokenTransfer`: Prohibit transfer of LPToken when an asset is frozen #5227

shawnxie999 · 2024-12-18T21:12:37Z

High Level Overview of Change

New Definition

A holder's LPToken is now considered "frozen" if at least one of the two assets in the associated liquidity pool is regularly frozen.

Background

Prior to this amendment, a blacklisted account that owns frozen LPTokens could still transfer them through mechanisms such as Payments, Checks, Offers, or NFTs. This loophole allowed the recipient of these LPTokens to redeem or withdraw the underlying assets of the pool, bypassing the restrictions that should have applied to the blacklisted account. This behavior undermines the freezing mechanism's integrity and creates inconsistencies in asset control.

Note: this amendment only works with the existing regular freeze, and has no correlation with deep-freeze

Example

Alice deposits XRP and USD into an AMM pool.
The USD issuer freezes Alice's USD trustline.
Alice transfers her LPToken to Bob (although Alice cannot transfer USD directly, she can still transfer the LPToken).
Bob redeems USD using the received LPToken, effectively bypassing the freezing restrictions imposed on Alice.

Amendment changes

This amendment ensures that frozen LPTokens cannot be transferred. Below are the changes to specific transaction types:

Payments

A blacklisted account can no longer send frozen LPTokens through direct payment.
Offers attempting to sell frozen LPTokens can no longer be consumed on the DEX.

OfferCreate

Accounts are prohibited from creating offers that attempt to sell frozen LPTokens.

CheckCash

Cashing a check fails if it attempts to cash a frozen LPToken from the sender.

NFTokenCreateOffer

Buyers are prohibited from creating buy offers involving frozen LPTokens.

NFTokenAcceptOffer

Buyers cannot accept a sell offer if the offer requires the use of frozen LPTokens as payment.

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 · 2024-12-18T23:18:30Z

Codecov Report

Attention: Patch coverage is 98.18182% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.0%. Comparing base (ff8b9aa) to head (d1d0abb).
Report is 5 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/app/misc/detail/AMMUtils.cpp	93.3%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5227   +/-   ##
=======================================
  Coverage     78.0%   78.0%           
=======================================
  Files          789     789           
  Lines        66953   66996   +43     
  Branches      8107    8111    +4     
=======================================
+ Hits         52218   52254   +36     
- Misses       14735   14742    +7

Files with missing lines	Coverage Δ
include/xrpl/protocol/Feature.h	`100.0% <ø> (ø)`
src/xrpld/app/paths/detail/DirectStep.cpp	`82.5% <100.0%> (ø)`
src/xrpld/app/paths/detail/StepChecks.h	`100.0% <100.0%> (ø)`
src/xrpld/ledger/View.h	`100.0% <ø> (ø)`
src/xrpld/ledger/detail/View.cpp	`91.3% <100.0%> (+0.2%)`	⬆️
src/xrpld/app/misc/detail/AMMUtils.cpp	`98.9% <93.3%> (-0.5%)`	⬇️

... and 4 files with indirect coverage changes

src/xrpld/ledger/detail/View.cpp

yinyiqian1

It looks functionally fine to me. Just left some comments about style, code coverage and redundant header files included.

src/xrpld/ledger/detail/View.cpp

yinyiqian1 · 2025-01-20T22:06:05Z

src/xrpld/ledger/detail/View.cpp

+                            account,
+                            (*sleAmm)[sfAsset].get<Issue>(),
+                            (*sleAmm)[sfAsset2].get<Issue>()))
+                    {


don't need curly braces

going to leave the braces here for readability since the if condition is fairly long

src/xrpld/app/paths/detail/StepChecks.h

yinyiqian1 · 2025-01-20T22:13:26Z

src/xrpld/app/paths/detail/StepChecks.h

+                    src,
+                    (*sleAmm)[sfAsset].get<Issue>(),
+                    (*sleAmm)[sfAsset2].get<Issue>()))
+            {


don't need curly braces

going leaving the braces here for readability

src/test/app/LPTokenTransfer_test.cpp

src/xrpld/ledger/detail/View.cpp

yinyiqian1 · 2025-01-21T21:41:11Z

src/xrpld/ledger/detail/View.cpp

@@ -304,7 +348,13 @@ accountHolds(
        }
        amount.setIssuer(issuer);
    }
-    JLOG(j.trace()) << "accountHolds:" << " account=" << to_string(account)
+    else
+    {


don't need {}

going to leave here for readability since if has braces

yinyiqian1 · 2025-01-21T21:57:17Z

src/xrpld/app/misc/detail/AMMUtils.cpp

+    auto const sle = view.read(keylet::line(lpAccount, ammAccount, currency));
+    if (!sle)
+    {
+        amount.clear(Issue{currency, ammAccount});


is this line covered? do we need to add an extra test to cover it?

its a copy paste of original accountHolds. This line will be covered by existing amm test cases that check for the balance of LPToken holders

shawnxie999 changed the title ~~Lptoken freeze~~ WIP Lptoken freeze Dec 18, 2024

shawnxie999 marked this pull request as ready for review December 18, 2024 21:40

shawnxie999 force-pushed the lptoken-freeze branch from caa857c to 7a0d1d7 Compare January 3, 2025 16:58

lpt freeze

efa1823

shawnxie999 force-pushed the lptoken-freeze branch from 7a0d1d7 to efa1823 Compare January 6, 2025 16:54

shawnxie999 changed the title ~~WIP Lptoken freeze~~ Amendment fixFrozenLPTokenTransfer: Prohibit transfer of LPToken when an asset is frozen Jan 6, 2025

shawnxie999 requested review from gregtatcam, yinyiqian1 and vvysokikh1 and removed request for gregtatcam January 7, 2025 16:11

shawnxie999 added Bug Amendment labels Jan 7, 2025

kennyzlei mentioned this pull request Jan 13, 2025

Clio Support for LPToken Freeze XRPLF/clio#1827

Open

shawnxie999 commented Jan 14, 2025

View reviewed changes

src/xrpld/ledger/detail/View.cpp Outdated Show resolved Hide resolved

shawnxie999 added 3 commits January 14, 2025 14:36

refactor accountholds

01ce1e3

comment

ea97b08

Merge remote-tracking branch 'upstream/develop' into lptoken-freeze

f0ac627

yinyiqian1 reviewed Jan 21, 2025

View reviewed changes

shawnxie999 added 2 commits January 22, 2025 12:21

addressc comments

0409265

const ref param

d1d0abb

shawnxie999 mentioned this pull request Jan 23, 2025

feat: [WIP] LPT freeze XRPLF/clio#1840

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amendment `fixFrozenLPTokenTransfer`: Prohibit transfer of LPToken when an asset is frozen #5227

Amendment `fixFrozenLPTokenTransfer`: Prohibit transfer of LPToken when an asset is frozen #5227

shawnxie999 commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

yinyiqian1 left a comment

yinyiqian1 Jan 20, 2025

shawnxie999 Jan 22, 2025

yinyiqian1 Jan 20, 2025

shawnxie999 Jan 22, 2025

yinyiqian1 Jan 21, 2025

shawnxie999 Jan 22, 2025

yinyiqian1 Jan 21, 2025

shawnxie999 Jan 22, 2025

Amendment fixFrozenLPTokenTransfer: Prohibit transfer of LPToken when an asset is frozen #5227

Are you sure you want to change the base?

Amendment fixFrozenLPTokenTransfer: Prohibit transfer of LPToken when an asset is frozen #5227

Conversation

shawnxie999 commented Dec 18, 2024 • edited Loading

High Level Overview of Change

New Definition

Background

Example

Amendment changes

Payments

OfferCreate

CheckCash

NFTokenCreateOffer

NFTokenAcceptOffer

Type of Change

API Impact

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

yinyiqian1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amendment `fixFrozenLPTokenTransfer`: Prohibit transfer of LPToken when an asset is frozen #5227

Amendment `fixFrozenLPTokenTransfer`: Prohibit transfer of LPToken when an asset is frozen #5227

shawnxie999 commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading