From 242ce3e9e4280aa7f9e6292062117c8ccd8861c8 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 20 May 2026 15:47:59 -0400 Subject: [PATCH 1/2] refactor: Fix `sfGeneric` and `sfInvalid` field names (#7300) --- include/xrpl/protocol/SField.h | 4 +-- include/xrpl/protocol/STBlob.h | 2 +- src/libxrpl/protocol/SField.cpp | 8 ++--- src/libxrpl/protocol/STAmount.cpp | 2 +- src/libxrpl/protocol/STBase.cpp | 2 +- src/libxrpl/protocol/STParsedJSON.cpp | 12 +++---- src/libxrpl/protocol/XChainAttestations.cpp | 4 +-- src/libxrpl/server/Manifest.cpp | 10 +++--- src/test/app/Manifest_test.cpp | 18 +++++------ src/test/app/Path_test.cpp | 6 ++-- src/test/app/ValidatorList_test.cpp | 4 +-- src/test/jtx/TrustedPublisherServer.h | 2 +- src/test/jtx/impl/TestHelpers.cpp | 6 ++-- src/test/overlay/compression_test.cpp | 6 ++-- src/test/protocol/Hooks_test.cpp | 6 ++-- src/test/protocol/STAmount_test.cpp | 2 +- src/test/protocol/STObject_test.cpp | 32 +++++++++---------- src/test/protocol/STTx_test.cpp | 2 +- src/test/rpc/Simulate_test.cpp | 4 +-- .../libxrpl/protocol_autogen/TestHelpers.h | 2 +- .../rpc/handlers/transaction/Simulate.cpp | 2 +- 21 files changed, 68 insertions(+), 68 deletions(-) diff --git a/include/xrpl/protocol/SField.h b/include/xrpl/protocol/SField.h index 1d4e4e69a0..34fb66ce00 100644 --- a/include/xrpl/protocol/SField.h +++ b/include/xrpl/protocol/SField.h @@ -365,8 +365,8 @@ using SF_XCHAIN_BRIDGE = TypedField; #define UNTYPED_SFIELD(sfName, stiSuffix, fieldValue, ...) extern SField const sfName; #define TYPED_SFIELD(sfName, stiSuffix, fieldValue, ...) extern SF_##stiSuffix const sfName; -extern SField const kSfInvalid; -extern SField const kSfGeneric; +extern SField const sfInvalid; // NOLINT(readability-identifier-naming) +extern SField const sfGeneric; // NOLINT(readability-identifier-naming) #include diff --git a/include/xrpl/protocol/STBlob.h b/include/xrpl/protocol/STBlob.h index c9110a367e..0667c54e30 100644 --- a/include/xrpl/protocol/STBlob.h +++ b/include/xrpl/protocol/STBlob.h @@ -24,7 +24,7 @@ public: STBlob(SField const& f, void const* data, std::size_t size); STBlob(SField const& f, Buffer&& b); STBlob(SField const& n); - STBlob(SerialIter&, SField const& name = kSfGeneric); + STBlob(SerialIter&, SField const& name = sfGeneric); [[nodiscard]] std::size_t size() const; diff --git a/src/libxrpl/protocol/SField.cpp b/src/libxrpl/protocol/SField.cpp index f2a523db49..a2d4903c6a 100644 --- a/src/libxrpl/protocol/SField.cpp +++ b/src/libxrpl/protocol/SField.cpp @@ -53,8 +53,8 @@ TypedField::TypedField(PrivateAccessTagT pat, Args&&... args) ##__VA_ARGS__); // SFields which, for historical reasons, do not follow naming conventions. -SField const kSfInvalid(access, -1, ""); -SField const kSfGeneric(access, 0, "Generic"); +SField const sfInvalid(access, -1, ""); +SField const sfGeneric(access, 0, "Generic"); // The following two fields aren't used anywhere, but they break tests/have // downstream effects. SField const kSfHash(access, STI_UINT256, 257, "hash"); @@ -121,7 +121,7 @@ SField::getField(int code) { return *(it->second); } - return kSfInvalid; + return sfInvalid; } int @@ -149,7 +149,7 @@ SField::getField(std::string const& fieldName) { return *(it->second); } - return kSfInvalid; + return sfInvalid; } } // namespace xrpl diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 2282488fb6..2d051722bf 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -1151,7 +1151,7 @@ amountFromJsonNoThrow(STAmount& result, json::Value const& jvSource) { try { - result = amountFromJson(kSfGeneric, jvSource); + result = amountFromJson(sfGeneric, jvSource); return true; } catch (std::exception const& e) diff --git a/src/libxrpl/protocol/STBase.cpp b/src/libxrpl/protocol/STBase.cpp index d1ae1e5aa8..f029f10e75 100644 --- a/src/libxrpl/protocol/STBase.cpp +++ b/src/libxrpl/protocol/STBase.cpp @@ -12,7 +12,7 @@ namespace xrpl { -STBase::STBase() : fName_(&kSfGeneric) +STBase::STBase() : fName_(&sfGeneric) { } diff --git a/src/libxrpl/protocol/STParsedJSON.cpp b/src/libxrpl/protocol/STParsedJSON.cpp index 7b9989afac..64c4dfd1be 100644 --- a/src/libxrpl/protocol/STParsedJSON.cpp +++ b/src/libxrpl/protocol/STParsedJSON.cpp @@ -258,7 +258,7 @@ parseUInt16( safeCast(static_cast( TxFormats::getInstance().findTypeByName(strValue)))); - if (*name == kSfGeneric) + if (*name == sfGeneric) name = &sfTransaction; } else if (field == sfLedgerEntryType) @@ -268,7 +268,7 @@ parseUInt16( safeCast(static_cast( LedgerFormats::getInstance().findTypeByName(strValue)))); - if (*name == kSfGeneric) + if (*name == sfGeneric) name = &sfLedgerEntry; } else @@ -361,7 +361,7 @@ parseLeaf( auto const& field = SField::getField(fieldName); // checked in parseObject - if (field == kSfInvalid) + if (field == sfInvalid) { // LCOV_EXCL_START error = unknownField(jsonName, fieldName); @@ -1013,7 +1013,7 @@ parseObject( json::Value const& value = json[fieldName]; auto const& field = SField::getField(fieldName); - if (field == kSfInvalid) + if (field == sfInvalid) { error = unknownField(jsonName, fieldName); return std::nullopt; @@ -1144,7 +1144,7 @@ parseArray( std::string const memberName(json[i].getMemberNames()[0]); auto const& nameField(SField::getField(memberName)); - if (nameField == kSfInvalid) + if (nameField == sfInvalid) { error = unknownField(jsonName, memberName); return std::nullopt; @@ -1189,7 +1189,7 @@ parseArray( STParsedJSONObject::STParsedJSONObject(std::string const& name, json::Value const& json) { using namespace STParsedJSONDetail; - object = parseObject(name, json, kSfGeneric, 0, error); + object = parseObject(name, json, sfGeneric, 0, error); } } // namespace xrpl diff --git a/src/libxrpl/protocol/XChainAttestations.cpp b/src/libxrpl/protocol/XChainAttestations.cpp index e41e5f3d61..805d08c097 100644 --- a/src/libxrpl/protocol/XChainAttestations.cpp +++ b/src/libxrpl/protocol/XChainAttestations.cpp @@ -195,7 +195,7 @@ AttestationClaim::message( std::uint64_t claimID, std::optional const& dst) { - STObject o{kSfGeneric}; + STObject o{sfGeneric}; // Serialize in SField order to make python serializers easier to write o[sfXChainClaimID] = claimID; o[sfAmount] = sendingAmount; @@ -332,7 +332,7 @@ AttestationCreateAccount::message( std::uint64_t createCount, AccountID const& dst) { - STObject o{kSfGeneric}; + STObject o{sfGeneric}; // Serialize in SField order to make python serializers easier to write o[sfXChainAccountCreateCount] = createCount; o[sfAmount] = sendingAmount; diff --git a/src/libxrpl/server/Manifest.cpp b/src/libxrpl/server/Manifest.cpp index 1b0493dd9d..b26c67e531 100644 --- a/src/libxrpl/server/Manifest.cpp +++ b/src/libxrpl/server/Manifest.cpp @@ -90,7 +90,7 @@ deserializeManifest(Slice s, beast::Journal journal) try { SerialIter sit{s}; - STObject st{sit, kSfGeneric}; + STObject st{sit, sfGeneric}; st.applyTemplate(kManifestFormat); @@ -193,7 +193,7 @@ logMftAct( bool Manifest::verify() const { - STObject st(kSfGeneric); + STObject st(sfGeneric); SerialIter sit(serialized.data(), serialized.size()); st.set(sit); @@ -213,7 +213,7 @@ Manifest::verify() const uint256 Manifest::hash() const { - STObject st(kSfGeneric); + STObject st(sfGeneric); SerialIter sit(serialized.data(), serialized.size()); st.set(sit); return st.getHash(HashPrefix::Manifest); @@ -240,7 +240,7 @@ Manifest::revoked(std::uint32_t sequence) std::optional Manifest::getSignature() const { - STObject st(kSfGeneric); + STObject st(sfGeneric); SerialIter sit(serialized.data(), serialized.size()); st.set(sit); if (!get(st, sfSignature)) @@ -251,7 +251,7 @@ Manifest::getSignature() const Blob Manifest::getMasterSignature() const { - STObject st(kSfGeneric); + STObject st(sfGeneric); SerialIter sit(serialized.data(), serialized.size()); st.set(sit); return st.getFieldVL(sfMasterSignature); diff --git a/src/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp index 55aa93bf79..d559ecd7b5 100644 --- a/src/test/app/Manifest_test.cpp +++ b/src/test/app/Manifest_test.cpp @@ -115,7 +115,7 @@ public: SecretKey const& ssk, int seq) { - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = seq; st[sfPublicKey] = pk; st[sfSigningPubKey] = spk; @@ -137,7 +137,7 @@ public: { auto const pk = derivePublicKey(type, sk); - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = std::numeric_limits::max(); st[sfPublicKey] = pk; @@ -156,7 +156,7 @@ public: { auto const pk = derivePublicKey(type, sk); - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = std::numeric_limits::max(); st[sfPublicKey] = pk; @@ -186,7 +186,7 @@ public: auto const pk = derivePublicKey(type, sk); auto const spk = derivePublicKey(stype, ssk); - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = seq; st[sfPublicKey] = pk; st[sfSigningPubKey] = spk; @@ -363,7 +363,7 @@ public: auto const kp = randomKeyPair(KeyType::Secp256k1); auto const m = makeManifest(sk, KeyType::Ed25519, kp.second, KeyType::Secp256k1, 0); - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = 0; st[sfPublicKey] = pk; st[sfSigningPubKey] = kp.first; @@ -495,7 +495,7 @@ public: auto const spk = derivePublicKey(KeyType::Secp256k1, ssk); auto buildManifestObject = [&](std::uint16_t version) { - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = 3; st[sfPublicKey] = pk; st[sfSigningPubKey] = spk; @@ -573,7 +573,7 @@ public: std::optional domain, bool noSigningPublic = false, bool noSignature = false) { - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = seq; st[sfPublicKey] = pk; @@ -724,7 +724,7 @@ public: } { // reject matching master & ephemeral keys - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = 314159; st[sfPublicKey] = pk; st[sfSigningPubKey] = pk; @@ -797,7 +797,7 @@ public: auto const pk2 = derivePublicKey(KeyType::Secp256k1, sk2); auto test = [&](std::string domain) { - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = 7; st[sfPublicKey] = pk1; st[sfDomain] = makeSlice(domain); diff --git a/src/test/app/Path_test.cpp b/src/test/app/Path_test.cpp index 22044a2572..1cf8131206 100644 --- a/src/test/app/Path_test.cpp +++ b/src/test/app/Path_test.cpp @@ -216,7 +216,7 @@ public: STAmount da; if (result.isMember(jss::destination_amount)) - da = amountFromJson(kSfGeneric, result[jss::destination_amount]); + da = amountFromJson(sfGeneric, result[jss::destination_amount]); STAmount sa; STPathSet paths; @@ -228,10 +228,10 @@ public: auto const& path = alts[0u]; if (path.isMember(jss::source_amount)) - sa = amountFromJson(kSfGeneric, path[jss::source_amount]); + sa = amountFromJson(sfGeneric, path[jss::source_amount]); if (path.isMember(jss::destination_amount)) - da = amountFromJson(kSfGeneric, path[jss::destination_amount]); + da = amountFromJson(sfGeneric, path[jss::destination_amount]); if (path.isMember(jss::paths_computed)) { diff --git a/src/test/app/ValidatorList_test.cpp b/src/test/app/ValidatorList_test.cpp index 15bd1bc586..1b66dd305d 100644 --- a/src/test/app/ValidatorList_test.cpp +++ b/src/test/app/ValidatorList_test.cpp @@ -81,7 +81,7 @@ private: SecretKey const& ssk, int seq) { - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = seq; st[sfPublicKey] = pk; @@ -104,7 +104,7 @@ private: static std::string makeRevocationString(PublicKey const& pk, SecretKey const& sk) { - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = std::numeric_limits::max(); st[sfPublicKey] = pk; diff --git a/src/test/jtx/TrustedPublisherServer.h b/src/test/jtx/TrustedPublisherServer.h index 4c7528ccc3..9a53a32481 100644 --- a/src/test/jtx/TrustedPublisherServer.h +++ b/src/test/jtx/TrustedPublisherServer.h @@ -101,7 +101,7 @@ public: SecretKey const& ssk, int seq) { - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = seq; st[sfPublicKey] = pk; st[sfSigningPubKey] = spk; diff --git a/src/test/jtx/impl/TestHelpers.cpp b/src/test/jtx/impl/TestHelpers.cpp index 3e65410825..e9ea48fb39 100644 --- a/src/test/jtx/impl/TestHelpers.cpp +++ b/src/test/jtx/impl/TestHelpers.cpp @@ -262,7 +262,7 @@ findPaths( STAmount da; if (result.isMember(jss::destination_amount)) - da = amountFromJson(kSfGeneric, result[jss::destination_amount]); + da = amountFromJson(sfGeneric, result[jss::destination_amount]); STAmount sa; STPathSet paths; @@ -274,10 +274,10 @@ findPaths( auto const& path = alts[0u]; if (path.isMember(jss::source_amount)) - sa = amountFromJson(kSfGeneric, path[jss::source_amount]); + sa = amountFromJson(sfGeneric, path[jss::source_amount]); if (path.isMember(jss::destination_amount)) - da = amountFromJson(kSfGeneric, path[jss::destination_amount]); + da = amountFromJson(sfGeneric, path[jss::destination_amount]); if (path.isMember(jss::paths_computed)) { diff --git a/src/test/overlay/compression_test.cpp b/src/test/overlay/compression_test.cpp index adb3b1b27b..d98ffa9b33 100644 --- a/src/test/overlay/compression_test.cpp +++ b/src/test/overlay/compression_test.cpp @@ -142,7 +142,7 @@ public: { auto master = randomKeyPair(KeyType::Ed25519); auto signing = randomKeyPair(KeyType::Ed25519); - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = i; st[sfPublicKey] = std::get<0>(master); st[sfSigningPubKey] = std::get<0>(signing); @@ -299,7 +299,7 @@ public: auto master = randomKeyPair(KeyType::Ed25519); auto signing = randomKeyPair(KeyType::Ed25519); - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = 0; st[sfPublicKey] = std::get<0>(master); st[sfSigningPubKey] = std::get<0>(signing); @@ -326,7 +326,7 @@ public: auto master = randomKeyPair(KeyType::Ed25519); auto signing = randomKeyPair(KeyType::Ed25519); - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfSequence] = 0; st[sfPublicKey] = std::get<0>(master); st[sfSigningPubKey] = std::get<0>(signing); diff --git a/src/test/protocol/Hooks_test.cpp b/src/test/protocol/Hooks_test.cpp index 20fe205579..082507aca7 100644 --- a/src/test/protocol/Hooks_test.cpp +++ b/src/test/protocol/Hooks_test.cpp @@ -73,7 +73,7 @@ class Hooks_test : public beast::unit_test::Suite { SField const& f = rf.get(); - STObject dummy{kSfGeneric}; + STObject dummy{sfGeneric}; BEAST_EXPECT(!dummy.isFieldPresent(f)); @@ -161,8 +161,8 @@ class Hooks_test : public beast::unit_test::Suite case STI_ARRAY: { STArray dummy2{f, 2}; - dummy2.pushBack(STObject{kSfGeneric}); - dummy2.pushBack(STObject{kSfGeneric}); + dummy2.pushBack(STObject{sfGeneric}); + dummy2.pushBack(STObject{sfGeneric}); dummy.setFieldArray(f, dummy2); BEAST_EXPECT(dummy.getFieldArray(f) == dummy2); BEAST_EXPECT(dummy.isFieldPresent(f)); diff --git a/src/test/protocol/STAmount_test.cpp b/src/test/protocol/STAmount_test.cpp index b387c361cb..322d51f84d 100644 --- a/src/test/protocol/STAmount_test.cpp +++ b/src/test/protocol/STAmount_test.cpp @@ -37,7 +37,7 @@ public: s.add(ser); SerialIter sit(ser.slice()); - return STAmount(sit, kSfGeneric); + return STAmount(sit, sfGeneric); } //-------------------------------------------------------------------------- diff --git a/src/test/protocol/STObject_test.cpp b/src/test/protocol/STObject_test.cpp index 59b78d213d..801eebbe63 100644 --- a/src/test/protocol/STObject_test.cpp +++ b/src/test/protocol/STObject_test.cpp @@ -36,19 +36,19 @@ public: { testcase("serialization"); - unexpected(kSfGeneric.isUseful(), "sfGeneric must not be useful"); + unexpected(sfGeneric.isUseful(), "sfGeneric must not be useful"); { // Try to put sfGeneric in an SOTemplate. except( - [&]() { SOTemplate const elements{{kSfGeneric, SoeRequired}}; }); + [&]() { SOTemplate const elements{{sfGeneric, SoeRequired}}; }); } - unexpected(kSfInvalid.isUseful(), "sfInvalid must not be useful"); + unexpected(sfInvalid.isUseful(), "sfInvalid must not be useful"); { // Test return of sfInvalid. auto testInvalid = [this](SerializedTypeID tid, int fv) { SField const& shouldBeInvalid{SField::getField(tid, fv)}; - BEAST_EXPECT(shouldBeInvalid == kSfInvalid); + BEAST_EXPECT(shouldBeInvalid == sfInvalid); }; testInvalid(STI_VL, 255); testInvalid(STI_UINT256, 255); @@ -59,7 +59,7 @@ public: { // Try to put sfInvalid in an SOTemplate. except( - [&]() { SOTemplate const elements{{kSfInvalid, SoeRequired}}; }); + [&]() { SOTemplate const elements{{sfInvalid, SoeRequired}}; }); } { // Try to put the same SField into an SOTemplate twice. @@ -188,7 +188,7 @@ public: { auto const st = [&]() { - STObject s(kSfGeneric); + STObject s(sfGeneric); s.setFieldU32(sf1Outer, 1); s.setFieldU32(sf2Outer, 2); return s; @@ -219,7 +219,7 @@ public: { auto const st = [&]() { - STObject s(sotOuter, kSfGeneric); + STObject s(sotOuter, sfGeneric); s.setFieldU32(sf1Outer, 1); s.setFieldU32(sf2Outer, 2); return s; @@ -239,7 +239,7 @@ public: // write free object { - STObject st(kSfGeneric); + STObject st(sfGeneric); unexcept([&]() { st[sf1Outer]; }); except([&]() { return st[sf1Outer] == 0; }); BEAST_EXPECT(st[~sf1Outer] == std::nullopt); @@ -295,7 +295,7 @@ public: // Write templated object { - STObject st(sotOuter, kSfGeneric); + STObject st(sotOuter, sfGeneric); BEAST_EXPECT(!!st[~sf1Outer]); BEAST_EXPECT(st[~sf1Outer] != std::nullopt); BEAST_EXPECT(st[sf1Outer] == 0); @@ -353,7 +353,7 @@ public: // coercion operator to std::optional { - STObject st(kSfGeneric); + STObject st(sfGeneric); auto const v = ~st[~sf1Outer]; static_assert( std::is_same_v, std::optional>, ""); @@ -362,7 +362,7 @@ public: // UDT scalar fields { - STObject st(kSfGeneric); + STObject st(sfGeneric); st[sfAmount] = STAmount{}; st[sfAccount] = AccountID{}; st[sfDigest] = uint256{}; @@ -375,7 +375,7 @@ public: { { - STObject st(kSfGeneric); + STObject st(sfGeneric); Buffer b(1); BEAST_EXPECT(!b.empty()); st[sf4] = std::move(b); @@ -392,7 +392,7 @@ public: BEAST_EXPECT(Slice(st[sf5]).size() == 2); } { - STObject st(sotOuter, kSfGeneric); + STObject st(sotOuter, sfGeneric); BEAST_EXPECT(st[sf5] == Slice{}); BEAST_EXPECT(!!st[~sf5]); BEAST_EXPECT(!!~st[~sf5]); @@ -408,7 +408,7 @@ public: // UDT blobs { - STObject st(kSfGeneric); + STObject st(sfGeneric); BEAST_EXPECT(!st[~sf5]); auto const kp = generateKeyPair(KeyType::Secp256k1, generateSeed("masterpassphrase")); st[sf5] = kp.first; @@ -419,7 +419,7 @@ public: { auto const& sf = sfIndexes; - STObject st(kSfGeneric); + STObject st(sfGeneric); std::vector v; v.emplace_back(1); v.emplace_back(2); @@ -446,7 +446,7 @@ public: {sf3, SoeDefault}, }; - STObject st(sot, kSfGeneric); + STObject st(sot, sfGeneric); auto const& cst(st); BEAST_EXPECT(cst[sf1].empty()); BEAST_EXPECT(!cst[~sf2]); diff --git a/src/test/protocol/STTx_test.cpp b/src/test/protocol/STTx_test.cpp index 42cb2bb9a7..21518d1406 100644 --- a/src/test/protocol/STTx_test.cpp +++ b/src/test/protocol/STTx_test.cpp @@ -1390,7 +1390,7 @@ public: // Lambda that returns a Payment STObject. auto getPayment = [kp1, id1, id2]() { // Account id1 pays account id2 10,000 XRP. - STObject payment(kSfGeneric); + STObject payment(sfGeneric); payment.setFieldU16(sfTransactionType, ttPAYMENT); payment.setAccountID(sfAccount, id1); payment.setAccountID(sfDestination, id2); diff --git a/src/test/rpc/Simulate_test.cpp b/src/test/rpc/Simulate_test.cpp index b66b3dfe9f..60452e386a 100644 --- a/src/test/rpc/Simulate_test.cpp +++ b/src/test/rpc/Simulate_test.cpp @@ -66,7 +66,7 @@ class Simulate_test : public beast::unit_test::Suite { auto const unHexed = strUnHex(result[jss::tx_blob].asString()); SerialIter sitTrans(makeSlice(*unHexed)); // NOLINT(bugprone-unchecked-optional-access) - txJson = STObject(std::ref(sitTrans), kSfGeneric).getJson(JsonOptions::Values::None); + txJson = STObject(std::ref(sitTrans), sfGeneric).getJson(JsonOptions::Values::None); } BEAST_EXPECT(txJson[jss::TransactionType] == tx[jss::TransactionType]); BEAST_EXPECT(txJson[jss::Account] == tx[jss::Account]); @@ -162,7 +162,7 @@ class Simulate_test : public beast::unit_test::Suite { auto unHexed = strUnHex(txResult[jss::meta_blob].asString()); SerialIter sitTrans(makeSlice(*unHexed)); // NOLINT(bugprone-unchecked-optional-access) - return STObject(std::ref(sitTrans), kSfGeneric).getJson(JsonOptions::Values::None); + return STObject(std::ref(sitTrans), sfGeneric).getJson(JsonOptions::Values::None); } return txResult[jss::meta]; diff --git a/src/tests/libxrpl/protocol_autogen/TestHelpers.h b/src/tests/libxrpl/protocol_autogen/TestHelpers.h index b8449f2bca..a32ab5e20c 100644 --- a/src/tests/libxrpl/protocol_autogen/TestHelpers.h +++ b/src/tests/libxrpl/protocol_autogen/TestHelpers.h @@ -159,7 +159,7 @@ canonical_ARRAY() inline STObject canonical_OBJECT() { - return STObject{kSfGeneric}; + return STObject{sfGeneric}; } inline STPathSet diff --git a/src/xrpld/rpc/handlers/transaction/Simulate.cpp b/src/xrpld/rpc/handlers/transaction/Simulate.cpp index ba108eb5b2..7a11b728ce 100644 --- a/src/xrpld/rpc/handlers/transaction/Simulate.cpp +++ b/src/xrpld/rpc/handlers/transaction/Simulate.cpp @@ -194,7 +194,7 @@ getTxJsonFromParams(json::Value const& params) try { SerialIter sitTrans(makeSlice(*unHexed)); - txJson = STObject(std::ref(sitTrans), kSfGeneric).getJson(JsonOptions::Values::None); + txJson = STObject(std::ref(sitTrans), sfGeneric).getJson(JsonOptions::Values::None); } catch (std::runtime_error const&) { From 9cb07406735a6bb967015495c33668d904d9ddee Mon Sep 17 00:00:00 2001 From: yinyiqian1 Date: Wed, 20 May 2026 16:24:09 -0400 Subject: [PATCH 2/2] fix: Fix multisign and signfor to check for delegate (#7064) --- src/libxrpl/protocol/STTx.cpp | 4 +- src/test/app/Delegate_test.cpp | 146 +++++++++++++++++++++++ src/xrpld/rpc/detail/TransactionSign.cpp | 8 +- 3 files changed, 155 insertions(+), 3 deletions(-) diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index 878aa296dd..8636c99284 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -535,8 +535,10 @@ STTx::checkMultiSign(Rules const& rules, STObject const& sigObject) const { // Used inside the loop in multiSignHelper to enforce that // the account owner may not multisign for themselves. + // For delegated transactions sfDelegate is the account whose signer list is checked, + // the delegate account itself can not be among the signers. auto const txnAccountID = - &sigObject != this ? std::nullopt : std::optional(getAccountID(sfAccount)); + &sigObject != this ? std::nullopt : std::optional(getFeePayer()); // We can ease the computational load inside the loop a bit by // pre-constructing part of the data that we hash. Fill a Serializer diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index 06884e9931..588aeee634 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -1879,6 +1880,149 @@ class Delegate_test : public beast::unit_test::Suite BEAST_EXPECT(env.balance(edward) == edwardBalance); } + void + testMultiSignDelegatorAsSigner() + { + // checkMultiSign disallows the owner of the account to + // be part of the multisigner list. When it is a delegated transaction, + // the delegate account should not be part of the multisigner list. + testcase("test delegator as multisigner in delegate's signer list"); + using namespace jtx; + + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const carol{"carol"}; + Account const daria{"daria"}; + env.fund(XRP(100000), alice, bob, carol, daria); + env.close(); + + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + // bob's signer list includes the delegator alice and daria + env(signers(bob, 2, {{alice, 1}, {daria, 1}})); + env.close(); + + auto aliceBalance = env.balance(alice); + auto bobBalance = env.balance(bob); + auto carolBalance = env.balance(carol); + auto const amt = 100; + + // alice can sign as a multisigner for bob + env(pay(alice, carol, XRP(100)), Fee(XRP(10)), delegate::As(bob), Msig(alice, daria)); + env.close(); + + BEAST_EXPECT(env.balance(alice) == aliceBalance - XRP(amt)); + BEAST_EXPECT(env.balance(bob) == bobBalance - XRP(10)); + BEAST_EXPECT(env.balance(carol) == carolBalance + XRP(amt)); + + // alice can not sign as a multisigner if she sent the transaction by herself. + env(pay(alice, carol, XRP(100)), Fee(XRP(10)), Msig(alice, daria), Ter(telENV_RPC_FAILED)); + env.close(); + + // Get new balances + aliceBalance = env.balance(alice); + bobBalance = env.balance(bob); + carolBalance = env.balance(carol); + + // bob (the delegate) should not appear as a multisigner in his transaction sent on behalf + // of alice. STTx::checkMultiSign catches this at the local-check stage, so the jtx + // framework returns telENV_RPC_FAILED. + env(pay(alice, carol, XRP(50)), + Fee(XRP(10)), + delegate::As(bob), + Msig(alice, bob), + Ter(telENV_RPC_FAILED)); + env.close(); + BEAST_EXPECT(env.balance(alice) == aliceBalance); + BEAST_EXPECT(env.balance(bob) == bobBalance); + BEAST_EXPECT(env.balance(carol) == carolBalance); + } + + void + testSignForDelegated() + { + // In sortAndValidateSigners, if it is a delegated transaction, the delegate account is + // the forbidden account from appearing in its own Signers array. + testcase("test sign_for with delegated transaction"); + using namespace jtx; + + Env env(*this); + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const carol{"carol"}; + Account const daria{"daria"}; + env.fund(XRP(100000), alice, bob, carol, daria); + env.close(); + + env(delegate::set(alice, bob, {"Payment"})); + env.close(); + + // bob's signer list includes the delegator alice and daria + env(signers(bob, 2, {{alice, 1}, {daria, 1}})); + env.close(); + + auto const baseFee = env.current()->fees().base; + + auto const sendAmt = 1'000'000; + auto makeDelegateTx = [&]() -> json::Value { + json::Value jv; + jv[jss::tx_json][jss::Account] = alice.human(); + jv[jss::tx_json][sfDelegate.jsonName] = bob.human(); + jv[jss::tx_json][jss::TransactionType] = jss::Payment; + jv[jss::tx_json][jss::Destination] = carol.human(); + jv[jss::tx_json][jss::Amount] = sendAmt; + jv[jss::tx_json][jss::Fee] = std::to_string((10 * baseFee).drops()); + jv[jss::tx_json][jss::Sequence] = env.seq(alice); + jv[jss::tx_json][jss::SigningPubKey] = ""; + return jv; + }; + + // The delegator alice and daria both sign via sign_for, which is valid + { + auto const aliceBalance = env.balance(alice); + auto const bobBalance = env.balance(bob); + auto const dariaBalance = env.balance(daria); + auto const carolBalance = env.balance(carol); + + json::Value jv = makeDelegateTx(); + jv[jss::account] = alice.human(); + jv[jss::secret] = alice.name(); + auto jrr = env.rpc("json", "sign_for", to_string(jv))[jss::result]; + BEAST_EXPECT(jrr[jss::status] == "success"); + + json::Value jv2; + jv2[jss::tx_json] = jrr[jss::tx_json]; + jv2[jss::account] = daria.human(); + jv2[jss::secret] = daria.name(); + jrr = env.rpc("json", "sign_for", to_string(jv2))[jss::result]; + BEAST_EXPECT(jrr[jss::status] == "success"); + + json::Value jvSubmit; + jvSubmit[jss::tx_json] = jrr[jss::tx_json]; + jrr = env.rpc("json", "submit_multisigned", to_string(jvSubmit))[jss::result]; + BEAST_EXPECT(jrr[jss::status] == "success"); + env.close(); + BEAST_EXPECT(env.balance(alice) == aliceBalance - XRPAmount(sendAmt)); + BEAST_EXPECT(env.balance(bob) == bobBalance - (10 * baseFee)); + BEAST_EXPECT(env.balance(daria) == dariaBalance); + BEAST_EXPECT(env.balance(carol) == carolBalance + XRPAmount(sendAmt)); + } + + // The delegated account bob attempts sign_for, will be rejected. + { + json::Value jv = makeDelegateTx(); + jv[jss::account] = bob.human(); + jv[jss::secret] = bob.name(); + auto jrr = env.rpc("json", "sign_for", to_string(jv))[jss::result]; + BEAST_EXPECT(jrr[jss::status] == "error"); + BEAST_EXPECT( + jrr[jss::error_message].asString().find( + "A Signer may not be the transaction's Account") != std::string::npos); + } + } + void testPermissionValue(FeatureBitset features) { @@ -2086,6 +2230,8 @@ class Delegate_test : public beast::unit_test::Suite testSingleSignBadSecret(); testMultiSign(); testMultiSignQuorumNotMet(); + testMultiSignDelegatorAsSigner(); + testSignForDelegated(); testPermissionValue(all); testTxRequireFeatures(all); testTxDelegableCount(); diff --git a/src/xrpld/rpc/detail/TransactionSign.cpp b/src/xrpld/rpc/detail/TransactionSign.cpp index 4c47914d66..9faa82dfe8 100644 --- a/src/xrpld/rpc/detail/TransactionSign.cpp +++ b/src/xrpld/rpc/detail/TransactionSign.cpp @@ -1255,7 +1255,9 @@ transactionSignFor( signers.emplaceBack(std::move(signer)); // The array must be sorted and validated. - auto err = sortAndValidateSigners(signers, (*sttx)[sfAccount]); + // For delegated transactions, the delegate account is + // the one forbidden from appearing in its own Signers array. + auto err = sortAndValidateSigners(signers, sttx->getFeePayer()); if (RPC::containsError(err)) return err; } @@ -1422,7 +1424,9 @@ transactionSubmitMultiSigned( } // The array must be sorted and validated. - auto err = sortAndValidateSigners(signers, srcAddressID); + // For delegated transactions, getFeePayer() returns sfDelegate, + // that account is the one forbidden from appearing in its own Signers array. + auto err = sortAndValidateSigners(signers, stTx->getFeePayer()); if (RPC::containsError(err)) return err;