From a9629ae325147120ee7a8f4aec089ff1635ac2b0 Mon Sep 17 00:00:00 2001 From: tequ Date: Fri, 21 Nov 2025 17:44:05 +0900 Subject: [PATCH] address reviews --- include/xrpl/protocol/TxFlags.h | 2 +- src/test/app/Sponsor_test.cpp | 14 ++++++- src/test/jtx/sponsor.h | 5 ++- src/xrpld/app/tx/detail/SponsorshipSet.cpp | 43 +++++++++++----------- src/xrpld/app/tx/detail/Transactor.cpp | 4 +- 5 files changed, 40 insertions(+), 28 deletions(-) diff --git a/include/xrpl/protocol/TxFlags.h b/include/xrpl/protocol/TxFlags.h index 2a54aa55da..a664cc5e94 100644 --- a/include/xrpl/protocol/TxFlags.h +++ b/include/xrpl/protocol/TxFlags.h @@ -46,7 +46,7 @@ constexpr std::uint32_t tfUniversalMask = ~tfUniversal; // Sponsor flags (Global): constexpr std::uint32_t tfSponsorFee = 0x00000001; constexpr std::uint32_t tfSponsorReserve = 0x00000002; -constexpr std::uint32_t tfSponsorMask = tfSponsorFee | tfSponsorReserve; +constexpr std::uint32_t tfSponsorMask = ~(tfSponsorFee | tfSponsorReserve); // AccountSet flags: constexpr std::uint32_t tfRequireDestTag = 0x00010000; diff --git a/src/test/app/Sponsor_test.cpp b/src/test/app/Sponsor_test.cpp index 5703f61e4b..8aca80c9df 100644 --- a/src/test/app/Sponsor_test.cpp +++ b/src/test/app/Sponsor_test.cpp @@ -335,7 +335,7 @@ public: // Invalid Flags env(noop(alice), sponsor::as(sponsor, 4), ter(temINVALID_FLAG)); env(noop(alice), - sponsor::as(sponsor, ~tfSponsorMask), + sponsor::as(sponsor, tfSponsorMask), ter(temINVALID_FLAG)); } @@ -4511,6 +4511,12 @@ public: env.close(); testFeePermission(tesSUCCESS); + + // test with SponsorReserve (should failed) + env(sponsor::set(alice, 0, 100, XRP(100)), + sponsor::sponseeAcc(bob), + delegate::as(carol), + ter(terNO_DELEGATE_PERMISSION)); } // @@ -4549,6 +4555,12 @@ public: env.close(); testReservePermission(tesSUCCESS); + + // test with SponsorFee (should failed) + env(sponsor::set(alice, 0, 100, XRP(100)), + sponsor::sponseeAcc(bob), + delegate::as(carol), + ter(terNO_DELEGATE_PERMISSION)); } } diff --git a/src/test/jtx/sponsor.h b/src/test/jtx/sponsor.h index e9993e2a2f..04ccc92fb3 100644 --- a/src/test/jtx/sponsor.h +++ b/src/test/jtx/sponsor.h @@ -1,4 +1,5 @@ -#pragma once +#ifndef XRPL_TEST_JTX_SPONSOR_H_INCLUDED +#define XRPL_TEST_JTX_SPONSOR_H_INCLUDED #include #include @@ -92,3 +93,5 @@ ledgerEntry( } // namespace jtx } // namespace test } // namespace ripple + +#endif diff --git a/src/xrpld/app/tx/detail/SponsorshipSet.cpp b/src/xrpld/app/tx/detail/SponsorshipSet.cpp index 5aef0cb899..2444be9874 100644 --- a/src/xrpld/app/tx/detail/SponsorshipSet.cpp +++ b/src/xrpld/app/tx/detail/SponsorshipSet.cpp @@ -53,26 +53,27 @@ SponsorshipSet::preflight(PreflightContext const& ctx) if (sponsee == sponsor) return temMALFORMED; - if (ctx.tx.isFieldPresent(sfFeeAmount)) - { - auto const feeAmount = ctx.tx.getFieldAmount(sfFeeAmount); + auto const checkOptionalAmountField = [&](SField const& field) -> NotTEC { + if (!ctx.tx.isFieldPresent(field)) + return tesSUCCESS; - if (!isXRP(feeAmount)) + auto const amount = ctx.tx.getFieldAmount(field); + + if (!isXRP(amount)) return temBAD_AMOUNT; - if (feeAmount.xrp().drops() <= 0) - return temBAD_AMOUNT; - } - - if (ctx.tx.isFieldPresent(sfMaxFee)) - { - auto const maxFee = ctx.tx.getFieldAmount(sfMaxFee); - if (!isXRP(maxFee)) + if (amount.xrp().drops() <= 0) return temBAD_AMOUNT; - if (maxFee.xrp().drops() <= 0) - return temBAD_AMOUNT; - } + return tesSUCCESS; + }; + + if (auto const ret = checkOptionalAmountField(sfFeeAmount); + !isTesSuccess(ret)) + return ret; + + if (auto const ret = checkOptionalAmountField(sfMaxFee); !isTesSuccess(ret)) + return ret; if (ctx.tx.isFieldPresent(sfReserveCount)) { @@ -122,15 +123,13 @@ SponsorshipSet::checkPermission(ReadView const& view, STTx const& tx) auto const sponsoringReserve = tx.isFieldPresent(sfReserveCount) || tx.isFlag(tfSponsorshipSetRequireSignForReserve); - if (granularPermissions.contains(SponsorFee) && sponsoringFee) - return tesSUCCESS; + if (sponsoringFee && !granularPermissions.contains(SponsorFee)) + return terNO_DELEGATE_PERMISSION; - if (granularPermissions.contains(SponsorReserve) && sponsoringReserve) - return tesSUCCESS; + if (sponsoringReserve && !granularPermissions.contains(SponsorReserve)) + return terNO_DELEGATE_PERMISSION; - // TODO: needs to check permission to delete sponsorship? - - return terNO_DELEGATE_PERMISSION; + return tesSUCCESS; } TER diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 1c706875ea..5c82d58066 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -154,9 +154,7 @@ Transactor::preflight1(PreflightContext const& ctx, std::uint32_t flagMask) } if (ctx.tx.isFieldPresent(sfSponsor) && !ctx.rules.enabled(featureSponsor)) - { return temDISABLED; - } if (auto const ret = preflight0(ctx, flagMask)) return ret; @@ -206,7 +204,7 @@ Transactor::preflight1(PreflightContext const& ctx, std::uint32_t flagMask) JLOG(ctx.j.debug()) << "preflight1: invalid sponsor account"; return temMALFORMED; } - if (!(sponsor.getFlags() & tfSponsorMask)) + if (sponsor.getFlags() & tfSponsorMask) { JLOG(ctx.j.debug()) << "preflight1: invalid sponsor flags"; return temINVALID_FLAG;