feat: PaymentChannelClawback - #7936
Conversation
dd3c590 to
787571d
Compare
787571d to
1e5e8ef
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| : TransactionBase(std::move(tx)) | ||
| { | ||
| // Verify transaction type | ||
| if (tx_->getTxnType() != txType) |
There was a problem hiding this comment.
I don't see anywhere that enforces tx_ to be non null. We might want an assert or a null check.
| */ | ||
| PaymentChannelClawbackBuilder(std::shared_ptr<STTx const> tx) | ||
| { | ||
| if (tx->getTxnType() != ttPAYCHAN_CLAWBACK) |
There was a problem hiding this comment.
Same thing here, nothing is enforcing tx to be non null. Maybe need an assert or a null check.
| { | ||
| throw std::runtime_error("Invalid transaction type for PaymentChannelClawbackBuilder"); | ||
| } | ||
| object_ = *tx; |
There was a problem hiding this comment.
Could probably get around the null check if the constructor parameter was a reference to an STTx instead of a pointer to one.
|
|
||
| public: | ||
| void | ||
| visitEntry(bool, SLE::const_ref, SLE::const_ref); |
There was a problem hiding this comment.
nit: Can these parameters have names? Or maybe there is a reason they are omitted?
| visitEntry(bool, SLE::const_ref, SLE::const_ref); | ||
|
|
||
| [[nodiscard]] bool | ||
| finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; |
There was a problem hiding this comment.
nit: Can these parameters have names?
| public: | ||
| static constexpr auto kConsequencesFactory = ConsequencesFactoryType::Normal; | ||
|
|
||
| explicit PaymentChannelClawback(ApplyContext& ctx) : Transactor(ctx) |
There was a problem hiding this comment.
Not for this PR, but I was curious if there is a reason to not use uniform initialization?
| beast::Journal const& j) const | ||
| { | ||
| // Token channels only exist once the amendment is active. | ||
| if (!view.rules().enabled(featureTokenPaychan)) |
There was a problem hiding this comment.
Should this check also be in the visitEntry function? Do we need to perform the computation up there based on this amendment? This is most likely just a misunderstanding on my part.
| return ret; // LCOV_EXCL_LINE | ||
| } | ||
|
|
||
| (*slep)[sfAmount] = newAmount; |
There was a problem hiding this comment.
nit: Would std::move(newAmount) be useful here? The Asset type is using a std::variant.
2 participants
High Level Overview of Change
Adds
PaymentChannelClawback, letting a token issuer claw back IOUs or MPTs locked in an active payment channel. Stacked on #7935 (Token PayChan); it merges into that branch, so this diff is clawback-only.Split out of the main PR for isolated review. It is part of the same
featureTokenPaychanamendment (not a separate one), so it must merge before that amendment activates.Context of Change
XLS-93d §3.6 deferred clawback with a "close then claw" workaround that does not actually work: the issuer is not a party to a channel and cannot close it, and normal
Clawbackclamps toaccountHolds()which excludes locked funds. This reaches the unclaimed remainder (sfAmount - sfBalance) directly. MPT decrements the holder/issuancesfLockedAmountand issuancesfOutstandingAmount(a redemption, fee waived); IOU retires the channel obligation since the value already sits with the issuer. It reuses the existing clawback opt-in (lsfAllowTrustLineClawback/lsfMPTCanClawback), so it grants issuers no new power, only removes a hiding place.Also adds a
ValidPaymentChannelinvariant (gated onfeatureTokenPaychan) enforcingsfAmount >= sfBalance, asset consistency, and non-decreasing balance across all channel operations.API Impact
PaymentChannelClawbacktransaction)libxrplchangeTest Plan
testIOU/MPTChannelClawbackinPayChanToken_test: full/partial claw, channel deletion and directory cleanup, MPT three-field decrement, subsequent-claim clamp (tecUNFUNDED_PAYMENT), over-claw clamp, wrong-asset (tecWRONG_ASSET), expired-channel claw, transfer-fee waive, and delegation. Full escrow/paychan/MPT/Invariants suites pass.