Compare commits

..

2 Commits

Author SHA1 Message Date
Ayaz Salikhov
e460ea0840 ci: Move Type of Change from PR template to CONTRIBUTING (#6522)
Now that prefixes in PR titles are being validated as part of CI, the "Type of Change" section in the PR template is no longer needed. The prefixes and descriptions in the `CONTRIBUTING.md` file have been updated to reflect the currently supported list.
2026-03-12 06:39:40 +01:00
yinyiqian1
46d5c67a8d fix: Mark SAV and Lending transactions as NotDelegable (#6489)
New transactions should be marked as `NotDelegable`, until the interactions with other transactions have been fully tested and validated.
2026-03-11 21:27:35 +00:00
8 changed files with 91 additions and 279 deletions

View File

@@ -29,22 +29,6 @@ If a refactor, how is this better than the previous implementation?
If there is a spec or design document for this feature, please link it here.
-->
### Type of Change
<!--
Please check [x] relevant options, delete irrelevant ones.
-->
- [ ] 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
<!--

View File

@@ -127,26 +127,6 @@ tl;dr
> 6. Wrap the body at 72 characters.
> 7. Use the body to explain what and why vs. how.
In addition to those guidelines, please add one of the following
prefixes to the subject line if appropriate.
- `fix:` - The primary purpose is to fix an existing bug.
- `perf:` - The primary purpose is performance improvements.
- `refactor:` - The changes refactor code without affecting
functionality.
- `test:` - The changes _only_ affect unit tests.
- `docs:` - The changes _only_ affect documentation. This can
include code comments in addition to `.md` files like this one.
- `build:` - The changes _only_ affect the build process,
including CMake and/or Conan settings.
- `chore:` - Other tasks that don't affect the binary, but don't fit
any of the other cases. e.g. formatting, git settings, updating
Github Actions jobs.
Whenever possible, when updating commits after the PR is open, please
add the PR number to the end of the subject line. e.g. `test: Add
unit tests for Feature X (#1234)`.
## Pull requests
In general, pull requests use `develop` as the base branch.
@@ -180,6 +160,23 @@ credibility of the existing approvals is insufficient.
Pull requests must be merged by [squash-and-merge][squash]
to preserve a linear history for the `develop` branch.
### Type of Change
In addition to those guidelines, please start your PR title with one of the following:
- `build:` - The changes _only_ affect the build process, including CMake and/or Conan settings.
- `feat`: New feature (change which adds functionality).
- `fix:` - The primary purpose is to fix an existing bug.
- `docs:` - The changes _only_ affect documentation.
- `test:` - The changes _only_ affect unit tests.
- `ci`: Continuous Integration (changes to our CI configuration files and scripts).
- `style`: Code style (formatting).
- `refactor:` - The changes refactor code without affecting functionality.
- `perf:` - The primary purpose is performance improvements.
- `chore:` - Other tasks that don't affect the binary, but don't fit any of the other cases. e.g. `git` settings, `clang-tidy`, removing dead code, dropping support for older tooling.
First letter after the type prefix should be capitalized, and the type prefix should be followed by a colon and a space. e.g. `feat: Add support for Borrowing Protocol`.
### "Ready to merge"
A pull request should only have the "Ready to merge" label added when it

View File

@@ -64,48 +64,6 @@
namespace xrpl {
// Feature names must not exceed this length (in characters, excluding the null terminator).
static constexpr std::size_t maxFeatureNameSize = 63;
// Reserve this exact feature-name length (in characters/bytes, excluding the null terminator)
// so that a 32-byte uint256 (for example, in WASM or other interop contexts) can be used
// as a compact, fixed-size feature selector without conflicting with human-readable names.
static constexpr std::size_t reservedFeatureNameSize = 32;
// Both validFeatureNameSize and validFeatureName are consteval functions that can be used in
// static_asserts to validate feature names at compile time. They are only used inside
// enforceValidFeatureName in Feature.cpp, but are exposed here for testing. The expected
// parameter `auto fn` is a constexpr lambda which returns a const char*, making it available
// for compile-time evaluation. Read more in https://accu.org/journals/overload/30/172/wu/
consteval auto
validFeatureNameSize(auto fn) -> bool
{
constexpr char const* n = fn();
// Note, std::strlen is not constexpr, we need to implement our own here.
constexpr std::size_t N = [](auto n) {
std::size_t ret = 0;
for (auto ptr = n; *ptr != '\0'; ret++, ++ptr)
;
return ret;
}(n);
return N != reservedFeatureNameSize && //
N != reservedFeatureNameSize + 1 && //
N <= maxFeatureNameSize;
}
consteval auto
validFeatureName(auto fn) -> bool
{
constexpr char const* n = fn();
// Prevent the use of visually confusable characters and enforce that feature names
// are always valid ASCII. This is needed because C++ allows Unicode identifiers.
for (auto ptr = n; *ptr != '\0'; ++ptr)
{
if (*ptr & 0x80 || *ptr < 0x20)
return false;
}
return true;
}
enum class VoteBehavior : int { Obsolete = -1, DefaultNo = 0, DefaultYes };
enum class AmendmentSupport : int { Retired = -1, Supported = 0, Unsupported };

View File

@@ -830,7 +830,7 @@ TRANSACTION(ttDELEGATE_SET, 64, DelegateSet,
# include <xrpl/tx/transactors/vault/VaultCreate.h>
#endif
TRANSACTION(ttVAULT_CREATE, 65, VaultCreate,
Delegation::delegable,
Delegation::notDelegable,
featureSingleAssetVault,
createPseudoAcct | createMPTIssuance | mustModifyVault,
({
@@ -848,7 +848,7 @@ TRANSACTION(ttVAULT_CREATE, 65, VaultCreate,
# include <xrpl/tx/transactors/vault/VaultSet.h>
#endif
TRANSACTION(ttVAULT_SET, 66, VaultSet,
Delegation::delegable,
Delegation::notDelegable,
featureSingleAssetVault,
mustModifyVault,
({
@@ -863,7 +863,7 @@ TRANSACTION(ttVAULT_SET, 66, VaultSet,
# include <xrpl/tx/transactors/vault/VaultDelete.h>
#endif
TRANSACTION(ttVAULT_DELETE, 67, VaultDelete,
Delegation::delegable,
Delegation::notDelegable,
featureSingleAssetVault,
mustDeleteAcct | destroyMPTIssuance | mustModifyVault,
({
@@ -875,7 +875,7 @@ TRANSACTION(ttVAULT_DELETE, 67, VaultDelete,
# include <xrpl/tx/transactors/vault/VaultDeposit.h>
#endif
TRANSACTION(ttVAULT_DEPOSIT, 68, VaultDeposit,
Delegation::delegable,
Delegation::notDelegable,
featureSingleAssetVault,
mayAuthorizeMPT | mustModifyVault,
({
@@ -888,7 +888,7 @@ TRANSACTION(ttVAULT_DEPOSIT, 68, VaultDeposit,
# include <xrpl/tx/transactors/vault/VaultWithdraw.h>
#endif
TRANSACTION(ttVAULT_WITHDRAW, 69, VaultWithdraw,
Delegation::delegable,
Delegation::notDelegable,
featureSingleAssetVault,
mayDeleteMPT | mayAuthorizeMPT | mustModifyVault,
({
@@ -903,7 +903,7 @@ TRANSACTION(ttVAULT_WITHDRAW, 69, VaultWithdraw,
# include <xrpl/tx/transactors/vault/VaultClawback.h>
#endif
TRANSACTION(ttVAULT_CLAWBACK, 70, VaultClawback,
Delegation::delegable,
Delegation::notDelegable,
featureSingleAssetVault,
mayDeleteMPT | mustModifyVault,
({
@@ -932,7 +932,7 @@ TRANSACTION(ttBATCH, 71, Batch,
# include <xrpl/tx/transactors/lending/LoanBrokerSet.h>
#endif
TRANSACTION(ttLOAN_BROKER_SET, 74, LoanBrokerSet,
Delegation::delegable,
Delegation::notDelegable,
featureLendingProtocol,
createPseudoAcct | mayAuthorizeMPT, ({
{sfVaultID, soeREQUIRED},
@@ -949,7 +949,7 @@ TRANSACTION(ttLOAN_BROKER_SET, 74, LoanBrokerSet,
# include <xrpl/tx/transactors/lending/LoanBrokerDelete.h>
#endif
TRANSACTION(ttLOAN_BROKER_DELETE, 75, LoanBrokerDelete,
Delegation::delegable,
Delegation::notDelegable,
featureLendingProtocol,
mustDeleteAcct | mayAuthorizeMPT, ({
{sfLoanBrokerID, soeREQUIRED},
@@ -960,7 +960,7 @@ TRANSACTION(ttLOAN_BROKER_DELETE, 75, LoanBrokerDelete,
# include <xrpl/tx/transactors/lending/LoanBrokerCoverDeposit.h>
#endif
TRANSACTION(ttLOAN_BROKER_COVER_DEPOSIT, 76, LoanBrokerCoverDeposit,
Delegation::delegable,
Delegation::notDelegable,
featureLendingProtocol,
noPriv, ({
{sfLoanBrokerID, soeREQUIRED},
@@ -972,7 +972,7 @@ TRANSACTION(ttLOAN_BROKER_COVER_DEPOSIT, 76, LoanBrokerCoverDeposit,
# include <xrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.h>
#endif
TRANSACTION(ttLOAN_BROKER_COVER_WITHDRAW, 77, LoanBrokerCoverWithdraw,
Delegation::delegable,
Delegation::notDelegable,
featureLendingProtocol,
mayAuthorizeMPT, ({
{sfLoanBrokerID, soeREQUIRED},
@@ -987,7 +987,7 @@ TRANSACTION(ttLOAN_BROKER_COVER_WITHDRAW, 77, LoanBrokerCoverWithdraw,
# include <xrpl/tx/transactors/lending/LoanBrokerCoverClawback.h>
#endif
TRANSACTION(ttLOAN_BROKER_COVER_CLAWBACK, 78, LoanBrokerCoverClawback,
Delegation::delegable,
Delegation::notDelegable,
featureLendingProtocol,
noPriv, ({
{sfLoanBrokerID, soeOPTIONAL},
@@ -999,7 +999,7 @@ TRANSACTION(ttLOAN_BROKER_COVER_CLAWBACK, 78, LoanBrokerCoverClawback,
# include <xrpl/tx/transactors/lending/LoanSet.h>
#endif
TRANSACTION(ttLOAN_SET, 80, LoanSet,
Delegation::delegable,
Delegation::notDelegable,
featureLendingProtocol,
mayAuthorizeMPT | mustModifyVault, ({
{sfLoanBrokerID, soeREQUIRED},
@@ -1026,7 +1026,7 @@ TRANSACTION(ttLOAN_SET, 80, LoanSet,
# include <xrpl/tx/transactors/lending/LoanDelete.h>
#endif
TRANSACTION(ttLOAN_DELETE, 81, LoanDelete,
Delegation::delegable,
Delegation::notDelegable,
featureLendingProtocol,
noPriv, ({
{sfLoanID, soeREQUIRED},
@@ -1037,7 +1037,7 @@ TRANSACTION(ttLOAN_DELETE, 81, LoanDelete,
# include <xrpl/tx/transactors/lending/LoanManage.h>
#endif
TRANSACTION(ttLOAN_MANAGE, 82, LoanManage,
Delegation::delegable,
Delegation::notDelegable,
featureLendingProtocol,
// All of the LoanManage options will modify the vault, but the
// transaction can succeed without options, essentially making it
@@ -1051,7 +1051,7 @@ TRANSACTION(ttLOAN_MANAGE, 82, LoanManage,
# include <xrpl/tx/transactors/lending/LoanPay.h>
#endif
TRANSACTION(ttLOAN_PAY, 84, LoanPay,
Delegation::delegable,
Delegation::notDelegable,
featureLendingProtocol,
mayAuthorizeMPT | mustModifyVault, ({
{sfLoanID, soeREQUIRED},

View File

@@ -395,20 +395,10 @@ featureToName(uint256 const& f)
#pragma push_macro("XRPL_RETIRE_FIX")
#undef XRPL_RETIRE_FIX
consteval auto
enforceValidFeatureName(auto fn) -> char const*
{
static_assert(validFeatureName(fn), "Invalid feature name");
static_assert(validFeatureNameSize(fn), "Invalid feature name size");
return fn();
}
#define XRPL_FEATURE(name, supported, vote) \
uint256 const feature##name = \
registerFeature(enforceValidFeatureName([] { return #name; }), supported, vote);
uint256 const feature##name = registerFeature(#name, supported, vote);
#define XRPL_FIX(name, supported, vote) \
uint256 const fix##name = \
registerFeature(enforceValidFeatureName([] { return "fix" #name; }), supported, vote);
uint256 const fix##name = registerFeature("fix" #name, supported, vote);
// clang-format off
#define XRPL_RETIRE_FEATURE(name) \

View File

@@ -1614,13 +1614,7 @@ class Delegate_test : public beast::unit_test::suite
{"CredentialDelete", featureCredentials},
{"NFTokenModify", featureDynamicNFT},
{"PermissionedDomainSet", featurePermissionedDomains},
{"PermissionedDomainDelete", featurePermissionedDomains},
{"VaultCreate", featureSingleAssetVault},
{"VaultSet", featureSingleAssetVault},
{"VaultDelete", featureSingleAssetVault},
{"VaultDeposit", featureSingleAssetVault},
{"VaultWithdraw", featureSingleAssetVault},
{"VaultClawback", featureSingleAssetVault}};
{"PermissionedDomainDelete", featurePermissionedDomains}};
// Can not delegate tx if any required feature disabled.
{
@@ -1660,6 +1654,56 @@ class Delegate_test : public beast::unit_test::suite
}
}
void
testTxDelegableCount()
{
testcase("Delegable Transactions Completeness");
std::size_t delegableCount = 0;
#pragma push_macro("TRANSACTION")
#undef TRANSACTION
#define TRANSACTION(tag, value, name, delegable, ...) \
if (delegable == xrpl::delegable) \
{ \
delegableCount++; \
}
#include <xrpl/protocol/detail/transactions.macro>
#undef TRANSACTION
#pragma pop_macro("TRANSACTION")
// ====================================================================
// IMPORTANT NOTICE:
//
// If this test fails, it indicates that the 'Delegation::delegable' status
// in transactions.macro has been changed. Delegation allows accounts to act
// on behalf of others, significantly increasing the security surface.
//
//
// To ENSURE any added transaction is safe and compatible with delegation:
//
// 1. Verify that the transaction is intended to be delegable.
// 2. Every standard test case for that transaction MUST be
// duplicated and verified for a Delegated context.
// 3. Ensure that Fee, Reserve, and Signing are correctly handled.
//
// DO NOT modify expectedDelegableCount unless all scenarios, including
// edge cases, have been fully tested and verified.
// ====================================================================
std::size_t const expectedDelegableCount = 75;
BEAST_EXPECTS(
delegableCount == expectedDelegableCount,
"\n[SECURITY] New delegable transaction detected!"
"\n Expected: " +
std::to_string(expectedDelegableCount) +
"\n Actual: " + std::to_string(delegableCount) +
"\n Action: Verify security requirements to interact with Delegation feature");
}
void
run() override
{
@@ -1684,6 +1728,7 @@ class Delegate_test : public beast::unit_test::suite
testMultiSignQuorumNotMet();
testPermissionValue(all);
testTxRequireFeatures(all);
testTxDelegableCount();
}
};
BEAST_DEFINE_TESTSUITE(Delegate, app, xrpl);

View File

@@ -4500,131 +4500,6 @@ class Vault_test : public beast::unit_test::suite
}
}
void
testDelegate()
{
using namespace test::jtx;
Env env(*this, testable_amendments());
Account alice{"alice"};
Account bob{"bob"};
Account carol{"carol"};
struct CaseArgs
{
PrettyAsset asset = xrpIssue();
};
auto const xrpBalance = [this](
Env const& env, Account const& account) -> std::optional<long> {
auto sle = env.le(keylet::account(account.id()));
if (BEAST_EXPECT(sle != nullptr))
return sle->getFieldAmount(sfBalance).xrp().drops();
return std::nullopt;
};
auto testCase = [&, this](auto test, CaseArgs args = {}) {
Env env{*this, testable_amendments() | featureSingleAssetVault};
Vault vault{env};
// use different initial amount to distinguish the source balance
env.fund(XRP(10000), alice);
env.fund(XRP(20000), bob);
env.fund(XRP(30000), carol);
env.close();
env(delegate::set(
carol,
alice,
{"Payment",
"VaultCreate",
"VaultSet",
"VaultDelete",
"VaultDeposit",
"VaultWithdraw",
"VaultClawback"}));
test(env, vault, args.asset);
};
testCase([&, this](Env& env, Vault& vault, PrettyAsset const& asset) {
testcase("delegated vault creation");
auto startBalance = xrpBalance(env, carol);
if (!BEAST_EXPECT(startBalance.has_value()))
return;
auto [tx, keylet] = vault.create({.owner = carol, .asset = asset});
env(tx, delegate::as(alice));
env.close();
BEAST_EXPECT(xrpBalance(env, carol) == *startBalance);
});
testCase([&, this](Env& env, Vault& vault, PrettyAsset const& asset) {
testcase("delegated deposit and withdrawal");
auto [tx, keylet] = vault.create({.owner = carol, .asset = asset});
env(tx);
env.close();
auto const amount = 1513;
auto const baseFee = env.current()->fees().base;
auto startBalance = xrpBalance(env, carol);
if (!BEAST_EXPECT(startBalance.has_value()))
return;
tx = vault.deposit({.depositor = carol, .id = keylet.key, .amount = asset(amount)});
env(tx, delegate::as(alice));
env.close();
BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - amount);
tx =
vault.withdraw({.depositor = carol, .id = keylet.key, .amount = asset(amount - 1)});
env(tx, delegate::as(alice));
env.close();
BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - 1);
tx = vault.withdraw({.depositor = carol, .id = keylet.key, .amount = asset(1)});
env(tx);
env.close();
BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - baseFee);
});
testCase([&, this](Env& env, Vault& vault, PrettyAsset const& asset) {
testcase("delegated withdrawal same as base fee and deletion");
auto [tx, keylet] = vault.create({.owner = carol, .asset = asset});
env(tx);
env.close();
auto const amount = 25537;
auto const baseFee = env.current()->fees().base;
auto startBalance = xrpBalance(env, carol);
if (!BEAST_EXPECT(startBalance.has_value()))
return;
tx = vault.deposit({.depositor = carol, .id = keylet.key, .amount = asset(amount)});
env(tx);
env.close();
BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - amount - baseFee);
tx = vault.withdraw({.depositor = carol, .id = keylet.key, .amount = asset(baseFee)});
env(tx, delegate::as(alice));
env.close();
BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - amount);
tx = vault.withdraw(
{.depositor = carol, .id = keylet.key, .amount = asset(amount - baseFee)});
env(tx, delegate::as(alice));
env.close();
BEAST_EXPECT(xrpBalance(env, carol) == *startBalance - baseFee);
tx = vault.del({.owner = carol, .id = keylet.key});
env(tx, delegate::as(alice));
env.close();
});
}
void
testVaultClawbackBurnShares()
{
@@ -5374,7 +5249,6 @@ public:
testFailedPseudoAccount();
testScaleIOU();
testRPC();
testDelegate();
testVaultClawbackBurnShares();
testVaultClawbackAssets();
testAssetsMaximum();

View File

@@ -2,7 +2,6 @@
#include <xrpl/ledger/AmendmentTable.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/digest.h>
#include <xrpl/protocol/jss.h>
namespace xrpl {
@@ -169,18 +168,16 @@ class Feature_test : public beast::unit_test::suite
using namespace test::jtx;
Env env{*this};
std::string const name = "fixAMMOverflowOffer";
auto jrr = env.rpc("feature", name)[jss::result];
auto jrr = env.rpc("feature", "fixAMMOverflowOffer")[jss::result];
BEAST_EXPECTS(jrr[jss::status] == jss::success, "status");
jrr.removeMember(jss::status);
BEAST_EXPECT(jrr.size() == 1);
auto const expected = to_string(sha512Half(Slice(name.data(), name.size())));
char const sha[] = "12523DF04B553A0B1AD74F42DDB741DE8DC06A03FC089A0EF197E2A87F1D8107";
BEAST_EXPECT(expected == sha);
BEAST_EXPECT(jrr.isMember(expected));
BEAST_EXPECT(jrr.isMember(
"12523DF04B553A0B1AD74F42DDB741DE8DC06A03FC089A0EF197E"
"2A87F1D8107"));
auto feature = *(jrr.begin());
BEAST_EXPECTS(feature[jss::name] == name, "name");
BEAST_EXPECTS(feature[jss::name] == "fixAMMOverflowOffer", "name");
BEAST_EXPECTS(!feature[jss::enabled].asBool(), "enabled");
BEAST_EXPECTS(feature[jss::vetoed].isBool() && !feature[jss::vetoed].asBool(), "vetoed");
BEAST_EXPECTS(feature[jss::supported].asBool(), "supported");
@@ -189,39 +186,6 @@ class Feature_test : public beast::unit_test::suite
jrr = env.rpc("feature", "fMM")[jss::result];
BEAST_EXPECT(jrr[jss::error] == "badFeature");
BEAST_EXPECT(jrr[jss::error_message] == "Feature unknown or invalid.");
// Test feature name size checks
constexpr auto ok63Name = [] {
return "123456789012345678901234567890123456789012345678901234567890123";
};
static_assert(validFeatureNameSize(ok63Name));
constexpr auto bad64Name = [] {
return "1234567890123456789012345678901234567890123456789012345678901234";
};
static_assert(!validFeatureNameSize(bad64Name));
constexpr auto ok31Name = [] { return "1234567890123456789012345678901"; };
static_assert(validFeatureNameSize(ok31Name));
constexpr auto bad32Name = [] { return "12345678901234567890123456789012"; };
static_assert(!validFeatureNameSize(bad32Name));
constexpr auto bad33Name = [] { return "123456789012345678901234567890123"; };
static_assert(!validFeatureNameSize(bad33Name));
constexpr auto ok34Name = [] { return "1234567890123456789012345678901234"; };
static_assert(validFeatureNameSize(ok34Name));
// Test feature character set checks
constexpr auto okName = [] { return "AMM_123"; };
static_assert(validFeatureName(okName));
// First character is Greek Capital Alpha, visually confusable with ASCII 'A'
constexpr auto badName = [] { return "ΑMM_123"; };
static_assert(!validFeatureName(badName));
constexpr auto badEmoji = [] { return "🔥"; };
static_assert(!validFeatureName(badEmoji));
}
void