-
Notifications
You must be signed in to change notification settings - Fork 1.6k
VaultClawback: Burn shares of an empty vault #6120
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
base: develop
Are you sure you want to change the base?
Changes from all commits
865bbc9
d46ed4e
c58724e
74f75ce
04e1af6
8a935e1
d049434
61f078d
733bb00
2c63982
ef44ca7
2af722f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -228,8 +228,8 @@ class Vault_test : public beast::unit_test::suite | |
|
|
||
| { | ||
| testcase(prefix + " clawback some"); | ||
| auto code = | ||
| asset.raw().native() ? ter(temMALFORMED) : ter(tesSUCCESS); | ||
| auto code = asset.raw().native() ? ter(tecNO_PERMISSION) | ||
| : ter(tesSUCCESS); | ||
| auto tx = vault.clawback( | ||
| {.issuer = issuer, | ||
| .id = keylet.key, | ||
|
|
@@ -1197,15 +1197,6 @@ class Vault_test : public beast::unit_test::suite | |
|
|
||
| auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); | ||
|
|
||
| { | ||
| auto tx = vault.clawback( | ||
| {.issuer = owner, | ||
| .id = keylet.key, | ||
| .holder = issuer, | ||
| .amount = asset(50)}); | ||
| env(tx, ter(temMALFORMED)); | ||
| } | ||
|
|
||
| { | ||
| auto tx = vault.clawback( | ||
| {.issuer = issuer, | ||
|
|
@@ -5243,6 +5234,167 @@ class Vault_test : public beast::unit_test::suite | |
| }); | ||
| } | ||
|
|
||
| void | ||
| testVaultClawbackBurnShares() | ||
| { | ||
| testcase("testVaultClawbackBurnShares - vault owner burn shares"); | ||
| using namespace test::jtx; | ||
| using namespace loanBroker; | ||
| using namespace loan; | ||
| Env env(*this, beast::severities::kWarning); | ||
| Account owner{"alice"}; | ||
| Account depositor{"bob"}; | ||
| Account issuer{"issuer"}; | ||
|
|
||
| env.fund(XRP(10000), issuer, owner, depositor); | ||
| env.close(); | ||
|
|
||
| auto const vaultAssetBalance = [&](Keylet const& vaultKeylet) { | ||
| auto const sleVault = env.le(vaultKeylet); | ||
| BEAST_EXPECT(sleVault != nullptr); | ||
|
|
||
| return std::make_pair( | ||
| sleVault->at(sfAssetsAvailable), sleVault->at(sfAssetsTotal)); | ||
| }; | ||
|
|
||
| auto const vaultShareBalance = [&](Keylet const& vaultKeylet) { | ||
| auto const sleVault = env.le(vaultKeylet); | ||
| BEAST_EXPECT(sleVault != nullptr); | ||
|
|
||
| auto const sleIssuance = | ||
| env.le(keylet::mptIssuance(sleVault->at(sfShareMPTID))); | ||
| BEAST_EXPECT(sleIssuance != nullptr); | ||
|
|
||
| return sleIssuance->at(sfOutstandingAmount); | ||
| }; | ||
|
|
||
| auto const setupVault = | ||
| [&](PrettyAsset const& asset) -> std::pair<Vault, Keylet> { | ||
| Vault vault{env}; | ||
|
|
||
| auto const [tx, vaultKeylet] = | ||
| vault.create({.owner = owner, .asset = asset}); | ||
| env(tx, ter(tesSUCCESS), THISLINE); | ||
| env.close(); | ||
|
|
||
| env(vault.deposit( | ||
| {.depositor = depositor, | ||
| .id = vaultKeylet.key, | ||
| .amount = asset(100)}), | ||
| ter(tesSUCCESS), | ||
| THISLINE); | ||
| env.close(); | ||
|
|
||
| auto const [availablePreDefault, totalPreDefault] = | ||
| vaultAssetBalance(vaultKeylet); | ||
| BEAST_EXPECT(availablePreDefault == totalPreDefault); | ||
| BEAST_EXPECT(availablePreDefault == asset(100).value()); | ||
|
Comment on lines
+5288
to
+5291
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should do an owner clawback here, before the loan to show that it doesn't work when there are assets available. |
||
|
|
||
| auto const brokerKeylet = | ||
| keylet::loanbroker(owner.id(), env.seq(owner)); | ||
|
|
||
| env(set(owner, vaultKeylet.key), THISLINE); | ||
| env.close(); | ||
|
|
||
| auto const loanKeylet = keylet::loan(brokerKeylet.key, 1); | ||
|
|
||
| // Create a simple Loan for the full amount of Vault assets | ||
| env(set(depositor, brokerKeylet.key, asset(100).value()), | ||
| loan::interestRate(TenthBips32(0)), | ||
| gracePeriod(10), | ||
| paymentInterval(120), | ||
| paymentTotal(10), | ||
| sig(sfCounterpartySignature, owner), | ||
| fee(env.current()->fees().base * 2), | ||
| ter(tesSUCCESS), | ||
| THISLINE); | ||
| env.close(); | ||
|
Comment on lines
+5301
to
+5311
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should also do an owner clawback here, before the default to show that it doesn't work when assets total is not zero, even if available is. |
||
|
|
||
| env.close(std::chrono::seconds{120 + 10}); | ||
|
|
||
| env(manage(owner, loanKeylet.key, tfLoanDefault), | ||
| ter(tesSUCCESS), | ||
| THISLINE); | ||
|
|
||
| auto const [availablePostDefault, totalPostDefault] = | ||
| vaultAssetBalance(vaultKeylet); | ||
|
|
||
| BEAST_EXPECT(availablePostDefault == totalPostDefault); | ||
| BEAST_EXPECT(availablePostDefault == asset(0).value()); | ||
|
|
||
| return std::make_pair(vault, vaultKeylet); | ||
| }; | ||
|
|
||
| auto const testCase = [&](PrettyAsset const& asset) { | ||
| // The owner cannot perform a non-zero share burn | ||
| { | ||
| auto [vault, vaultKeylet] = setupVault(asset); | ||
| env(vault.clawback({ | ||
| .issuer = owner, | ||
| .id = vaultKeylet.key, | ||
| .holder = depositor, | ||
| .amount = asset(1).value(), | ||
| }), | ||
| ter(tecLIMIT_EXCEEDED), | ||
| THISLINE); | ||
| env.close(); | ||
| } | ||
|
|
||
| // // The owner can clawback all shares, burning them | ||
| { | ||
| auto [vault, vaultKeylet] = setupVault(asset); | ||
| env(vault.clawback({ | ||
| .issuer = owner, | ||
| .id = vaultKeylet.key, | ||
| .holder = depositor, | ||
| }), | ||
| ter(tesSUCCESS), | ||
| THISLINE); | ||
| env.close(); | ||
| BEAST_EXPECT(vaultShareBalance(vaultKeylet) == 0); | ||
| } | ||
|
|
||
| // The owner can clawback explicitly all shares, burning them | ||
| { | ||
| auto [vault, vaultKeylet] = setupVault(asset); | ||
| env(vault.clawback({ | ||
| .issuer = owner, | ||
| .id = vaultKeylet.key, | ||
| .holder = depositor, | ||
| .amount = asset(vaultShareBalance(vaultKeylet)), | ||
|
Comment on lines
+5360
to
+5364
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is subtle. The spec is very clear that the amount must be the shares asset type. But this is the vault asset type. This test should be failing. However, as I noted in VaultClawback, if you pass the shares type, you'll get (When you fix the issues, you should keep these transactions to verify they fail.) |
||
| }), | ||
| ter(tesSUCCESS), | ||
| THISLINE); | ||
| env.close(); | ||
| BEAST_EXPECT(vaultShareBalance(vaultKeylet) == 0); | ||
| } | ||
|
Comment on lines
+5357
to
+5370
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be worth another test where amount is explicitly zero. |
||
| }; | ||
|
|
||
| // Test XRP | ||
| PrettyAsset xrp = xrpIssue(); | ||
| testCase(xrp); | ||
|
|
||
| // Test IOU | ||
| PrettyAsset IOU = issuer["IOU"]; | ||
| env.trust(IOU(1000), owner); | ||
| env.trust(IOU(1000), depositor); | ||
| env(pay(issuer, owner, IOU(100))); | ||
| env(pay(issuer, depositor, IOU(100))); | ||
| env.close(); | ||
| testCase(IOU); | ||
|
|
||
| // Test MPT | ||
| MPTTester mptt{env, issuer, mptInitNoFund}; | ||
| mptt.create( | ||
| {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock}); | ||
| PrettyAsset MPT = mptt.issuanceID(); | ||
| mptt.authorize({.account = owner}); | ||
| mptt.authorize({.account = depositor}); | ||
| env(pay(issuer, depositor, MPT(1000))); | ||
| env.close(); | ||
| testCase(MPT); | ||
| } | ||
|
|
||
| public: | ||
| void | ||
| run() override | ||
|
|
@@ -5261,6 +5413,7 @@ class Vault_test : public beast::unit_test::suite | |
| testScaleIOU(); | ||
| testRPC(); | ||
| testDelegate(); | ||
| testVaultClawbackBurnShares(); | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -95,6 +95,7 @@ hasPrivilege(STTx const& tx, Privilege priv) | |||||||||||||||
| switch (tx.getTxnType()) | ||||||||||||||||
| { | ||||||||||||||||
| #include <xrpl/protocol/detail/transactions.macro> | ||||||||||||||||
|
|
||||||||||||||||
| // Deprecated types | ||||||||||||||||
| default: | ||||||||||||||||
| return false; | ||||||||||||||||
|
|
@@ -2622,6 +2623,7 @@ ValidVault::Vault::make(SLE const& from) | |||||||||||||||
| self.key = from.key(); | ||||||||||||||||
| self.asset = from.at(sfAsset); | ||||||||||||||||
| self.pseudoId = from.getAccountID(sfAccount); | ||||||||||||||||
| self.owner = from.at(sfOwner); | ||||||||||||||||
| self.shareMPTID = from.getFieldH192(sfShareMPTID); | ||||||||||||||||
| self.assetsTotal = from.at(sfAssetsTotal); | ||||||||||||||||
| self.assetsAvailable = from.at(sfAssetsAvailable); | ||||||||||||||||
|
|
@@ -3066,6 +3068,10 @@ ValidVault::finalize( | |||||||||||||||
| : std::nullopt; | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| auto const vaultHoldsNoAssets = [&](Vault const& vault) { | ||||||||||||||||
| return vault.assetsAvailable == 0 && vault.assetsTotal == 0; | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| // Technically this does not need to be a lambda, but it's more | ||||||||||||||||
| // convenient thanks to early "return false"; the not-so-nice | ||||||||||||||||
| // alternatives are several layers of nested if/else or more complex | ||||||||||||||||
|
|
@@ -3448,29 +3454,56 @@ ValidVault::finalize( | |||||||||||||||
| if (vaultAsset.native() || | ||||||||||||||||
| vaultAsset.getIssuer() != tx[sfAccount]) | ||||||||||||||||
| { | ||||||||||||||||
| JLOG(j.fatal()) << // | ||||||||||||||||
| "Invariant failed: clawback may only be performed by " | ||||||||||||||||
| "the asset issuer"; | ||||||||||||||||
| return false; // That's all we can do | ||||||||||||||||
| // The owner can use clawback to force-burn shares when the | ||||||||||||||||
| // vault is empty but there are outstanding shares | ||||||||||||||||
| if (!(beforeShares && beforeShares->sharesTotal > 0 && | ||||||||||||||||
| vaultHoldsNoAssets(beforeVault) && | ||||||||||||||||
| beforeVault.owner == tx[sfAccount])) | ||||||||||||||||
| { | ||||||||||||||||
| JLOG(j.fatal()) << // | ||||||||||||||||
| "Invariant failed: clawback may only be performed " | ||||||||||||||||
| "by the asset issuer"; | ||||||||||||||||
|
Comment on lines
+3463
to
+3465
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
| return false; // That's all we can do | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| auto const vaultDeltaAssets = deltaAssets(afterVault.pseudoId); | ||||||||||||||||
| if (vaultDeltaAssets) | ||||||||||||||||
| { | ||||||||||||||||
| if (*vaultDeltaAssets >= zero) | ||||||||||||||||
| { | ||||||||||||||||
| JLOG(j.fatal()) << // | ||||||||||||||||
| "Invariant failed: clawback must decrease vault " | ||||||||||||||||
| "balance"; | ||||||||||||||||
| result = false; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (!vaultDeltaAssets) | ||||||||||||||||
| if (beforeVault.assetsTotal + *vaultDeltaAssets != | ||||||||||||||||
| afterVault.assetsTotal) | ||||||||||||||||
| { | ||||||||||||||||
| JLOG(j.fatal()) << // | ||||||||||||||||
| "Invariant failed: clawback and assets outstanding " | ||||||||||||||||
| "must add up"; | ||||||||||||||||
| result = false; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (beforeVault.assetsAvailable + *vaultDeltaAssets != | ||||||||||||||||
| afterVault.assetsAvailable) | ||||||||||||||||
| { | ||||||||||||||||
| JLOG(j.fatal()) << // | ||||||||||||||||
| "Invariant failed: clawback and assets available " | ||||||||||||||||
| "must " | ||||||||||||||||
| "add up"; | ||||||||||||||||
|
Comment on lines
+3493
to
+3496
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
| result = false; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| else if (!vaultHoldsNoAssets(beforeVault)) | ||||||||||||||||
| { | ||||||||||||||||
| JLOG(j.fatal()) << // | ||||||||||||||||
| "Invariant failed: clawback must change vault balance"; | ||||||||||||||||
| return false; // That's all we can do | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (*vaultDeltaAssets >= zero) | ||||||||||||||||
| { | ||||||||||||||||
| JLOG(j.fatal()) << // | ||||||||||||||||
| "Invariant failed: clawback must decrease vault " | ||||||||||||||||
| "balance"; | ||||||||||||||||
| result = false; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| auto const accountDeltaShares = deltaShares(tx[sfHolder]); | ||||||||||||||||
| if (!accountDeltaShares) | ||||||||||||||||
| { | ||||||||||||||||
|
|
@@ -3503,24 +3536,6 @@ ValidVault::finalize( | |||||||||||||||
| result = false; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (beforeVault.assetsTotal + *vaultDeltaAssets != | ||||||||||||||||
| afterVault.assetsTotal) | ||||||||||||||||
| { | ||||||||||||||||
| JLOG(j.fatal()) << // | ||||||||||||||||
| "Invariant failed: clawback and assets outstanding " | ||||||||||||||||
| "must add up"; | ||||||||||||||||
| result = false; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (beforeVault.assetsAvailable + *vaultDeltaAssets != | ||||||||||||||||
| afterVault.assetsAvailable) | ||||||||||||||||
| { | ||||||||||||||||
| JLOG(j.fatal()) << // | ||||||||||||||||
| "Invariant failed: clawback and assets available must " | ||||||||||||||||
| "add up"; | ||||||||||||||||
| result = false; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return result; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
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'm not usually a fan of removing test cases unless they're redundant or mess up later tests. I think it's usually better to update the test with the new expected result. Do you think that is appropriate here?