ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2025-11-19 02:25:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor: Retire fix1571 amendment (#5925)

Browse Source 
Amendments activated for more than 2 years can be retired. This change retires the fix1571 amendment.

Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com>
This commit is contained in:
Pratik Mankawde
2025-10-29 14:21:50 +00:00
committed by GitHub
parent ed5d6f3e22
commit bd3bc917f8
3 changed files with 64 additions and 157 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
include/xrpl/protocol/detail/features.macro
View File

@@ -109,7 +109,6 @@ XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYe
XRPL_FIX (1578, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (1578, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FIX (1623, Supported::yes, VoteBehavior::DefaultYes) XRPL_FIX (1623, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FIX (1571, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FEATURE(DepositAuth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DepositAuth, Supported::yes, VoteBehavior::DefaultYes)
XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes)
@@ -154,3 +153,4 @@ XRPL_RETIRE(fix1513)
XRPL_RETIRE(fix1515) XRPL_RETIRE(fix1515)
XRPL_RETIRE(fix1543) XRPL_RETIRE(fix1543)
XRPL_RETIRE(fix1781) XRPL_RETIRE(fix1781)
XRPL_RETIRE(fix1571)

99
src/test/app/Escrow_test.cpp
View File

@@ -294,74 +294,51 @@ struct Escrow_test : public beast::unit_test::suite
} }
void void
test1571(FeatureBitset features) testRequiresConditionOrFinishAfter(FeatureBitset features)
{ {
using namespace jtx; using namespace jtx;
using namespace std::chrono; using namespace std::chrono;
{ testcase("RequiresConditionOrFinishAfter");
testcase("Implied Finish Time (without fix1571)");
Env env(*this, testable_amendments() - fix1571); Env env(*this, features);
auto const baseFee = env.current()->fees().base; auto const baseFee = env.current()->fees().base;
env.fund(XRP(5000), "alice", "bob", "carol"); env.fund(XRP(5000), "alice", "bob", "carol");
env.close(); env.close();
// Creating an escrow without a finish time and finishing it // Creating an escrow with only a cancel time is not allowed:
// is allowed without fix1571: env(escrow::create("alice", "bob", XRP(100)),
auto const seq1 = env.seq("alice"); escrow::cancel_time(env.now() + 90s),
env(escrow::create("alice", "bob", XRP(100)), fee(baseFee * 150),
escrow::cancel_time(env.now() + 1s), ter(temMALFORMED));
fee(baseFee * 150));
env.close();
env(escrow::finish("carol", "alice", seq1), fee(baseFee * 150));
BEAST_EXPECT(env.balance("bob") == XRP(5100));
env.close(); // Creating an escrow with only a cancel time and a condition is
// allowed:
auto const seq = env.seq("alice");
env(escrow::create("alice", "bob", XRP(100)),
escrow::cancel_time(env.now() + 90s),
escrow::condition(escrow::cb1),
fee(baseFee * 150));
env.close();
env(escrow::finish("carol", "alice", seq),
escrow::condition(escrow::cb1),
escrow::fulfillment(escrow::fb1),
fee(baseFee * 150));
BEAST_EXPECT(env.balance("bob") == XRP(5100));
// Creating an escrow without a finish time and a condition is // Creating an escrow with only a cancel time and a finish time is
// also allowed without fix1571: // allowed:
auto const seq2 = env.seq("alice"); auto const seqFt = env.seq("alice");
env(escrow::create("alice", "bob", XRP(100)), env(escrow::create("alice", "bob", XRP(100)),
escrow::cancel_time(env.now() + 1s), escrow::finish_time(env.now()), // Set finish time to now so that
escrow::condition(escrow::cb1), // we can call finish immediately.
fee(baseFee * 150)); escrow::cancel_time(env.now() + 50s),
env.close(); fee(baseFee * 150));
env(escrow::finish("carol", "alice", seq2), env.close();
escrow::condition(escrow::cb1), env(escrow::finish("carol", "alice", seqFt), fee(150 * baseFee));
escrow::fulfillment(escrow::fb1), BEAST_EXPECT(
fee(baseFee * 150)); env.balance("bob") ==
BEAST_EXPECT(env.balance("bob") == XRP(5200)); XRP(5200)); // 5100 (from last transaction) + 100
}
{
testcase("Implied Finish Time (with fix1571)");
Env env(*this, features);
auto const baseFee = env.current()->fees().base;
env.fund(XRP(5000), "alice", "bob", "carol");
env.close();
// Creating an escrow with only a cancel time is not allowed:
env(escrow::create("alice", "bob", XRP(100)),
escrow::cancel_time(env.now() + 90s),
fee(baseFee * 150),
ter(temMALFORMED));
// Creating an escrow with only a cancel time and a condition is
// allowed:
auto const seq = env.seq("alice");
env(escrow::create("alice", "bob", XRP(100)),
escrow::cancel_time(env.now() + 90s),
escrow::condition(escrow::cb1),
fee(baseFee * 150));
env.close();
env(escrow::finish("carol", "alice", seq),
escrow::condition(escrow::cb1),
escrow::fulfillment(escrow::fb1),
fee(baseFee * 150));
BEAST_EXPECT(env.balance("bob") == XRP(5100));
}
} }
void void
@@ -1708,7 +1685,7 @@ struct Escrow_test : public beast::unit_test::suite
testTiming(features); testTiming(features);
testTags(features); testTags(features);
testDisallowXRP(features); testDisallowXRP(features);
test1571(features); testRequiresConditionOrFinishAfter(features);
testFails(features); testFails(features);
testLockup(features); testLockup(features);
testEscrowConditions(features); testEscrowConditions(features);

120
src/xrpld/app/tx/detail/Escrow.cpp
View File

@@ -151,15 +151,12 @@ EscrowCreate::preflight(PreflightContext const& ctx)
ctx.tx[sfCancelAfter] <= ctx.tx[sfFinishAfter]) ctx.tx[sfCancelAfter] <= ctx.tx[sfFinishAfter])
return temBAD_EXPIRATION; return temBAD_EXPIRATION;
if (ctx.rules.enabled(fix1571)) // In the absence of a FinishAfter, the escrow can be finished
{ // immediately, which can be confusing. When creating an escrow,
// In the absence of a FinishAfter, the escrow can be finished // we want to ensure that either a FinishAfter time is explicitly
// immediately, which can be confusing. When creating an escrow, // specified or a completion condition is attached.
// we want to ensure that either a FinishAfter time is explicitly if (!ctx.tx[~sfFinishAfter] && !ctx.tx[~sfCondition])
// specified or a completion condition is attached. return temMALFORMED;
if (!ctx.tx[~sfFinishAfter] && !ctx.tx[~sfCondition])
return temMALFORMED;
}
if (auto const cb = ctx.tx[~sfCondition]) if (auto const cb = ctx.tx[~sfCondition])
{ {
@@ -446,41 +443,11 @@ EscrowCreate::doApply()
{ {
auto const closeTime = ctx_.view().info().parentCloseTime; auto const closeTime = ctx_.view().info().parentCloseTime;
// Prior to fix1571, the cancel and finish times could be greater if (ctx_.tx[~sfCancelAfter] && after(closeTime, ctx_.tx[sfCancelAfter]))
// than or equal to the parent ledgers' close time. return tecNO_PERMISSION;
//
// With fix1571, we require that they both be strictly greater
// than the parent ledgers' close time.
if (ctx_.view().rules().enabled(fix1571))
{
if (ctx_.tx[~sfCancelAfter] && after(closeTime, ctx_.tx[sfCancelAfter]))
return tecNO_PERMISSION;
if (ctx_.tx[~sfFinishAfter] && after(closeTime, ctx_.tx[sfFinishAfter])) if (ctx_.tx[~sfFinishAfter] && after(closeTime, ctx_.tx[sfFinishAfter]))
return tecNO_PERMISSION; return tecNO_PERMISSION;
}
else
{
// This is old code that needs to stay to support replaying old ledgers,
// but does not need to be covered by new tests.
// LCOV_EXCL_START
if (ctx_.tx[~sfCancelAfter])
{
auto const cancelAfter = ctx_.tx[sfCancelAfter];
if (closeTime.time_since_epoch().count() >= cancelAfter)
return tecNO_PERMISSION;
}
if (ctx_.tx[~sfFinishAfter])
{
auto const finishAfter = ctx_.tx[sfFinishAfter];
if (closeTime.time_since_epoch().count() >= finishAfter)
return tecNO_PERMISSION;
}
// LCOV_EXCL_STOP
}
auto const sle = ctx_.view().peek(keylet::account(account_)); auto const sle = ctx_.view().peek(keylet::account(account_));
if (!sle) if (!sle)
@@ -1027,38 +994,16 @@ EscrowFinish::doApply()
} }
// If a cancel time is present, a finish operation should only succeed prior // If a cancel time is present, a finish operation should only succeed prior
// to that time. fix1571 corrects a logic error in the check that would make // to that time.
// a finish only succeed strictly after the cancel time. auto const now = ctx_.view().info().parentCloseTime;
if (ctx_.view().rules().enabled(fix1571))
{
auto const now = ctx_.view().info().parentCloseTime;
// Too soon: can't execute before the finish time // Too soon: can't execute before the finish time
if ((*slep)[~sfFinishAfter] && !after(now, (*slep)[sfFinishAfter])) if ((*slep)[~sfFinishAfter] && !after(now, (*slep)[sfFinishAfter]))
return tecNO_PERMISSION; return tecNO_PERMISSION;
// Too late: can't execute after the cancel time // Too late: can't execute after the cancel time
if ((*slep)[~sfCancelAfter] && after(now, (*slep)[sfCancelAfter])) if ((*slep)[~sfCancelAfter] && after(now, (*slep)[sfCancelAfter]))
return tecNO_PERMISSION; return tecNO_PERMISSION;
}
else
{
// This is old code that needs to stay to support replaying old ledgers,
// but does not need to be covered by new tests.
// LCOV_EXCL_START
// Too soon?
if ((*slep)[~sfFinishAfter] &&
ctx_.view().info().parentCloseTime.time_since_epoch().count() <=
(*slep)[sfFinishAfter])
return tecNO_PERMISSION;
// Too late?
if ((*slep)[~sfCancelAfter] &&
ctx_.view().info().parentCloseTime.time_since_epoch().count() <=
(*slep)[sfCancelAfter])
return tecNO_PERMISSION;
// LCOV_EXCL_STOP
}
// Check cryptocondition fulfillment // Check cryptocondition fulfillment
{ {
@@ -1315,30 +1260,15 @@ EscrowCancel::doApply()
return tecNO_TARGET; return tecNO_TARGET;
} }
if (ctx_.view().rules().enabled(fix1571)) auto const now = ctx_.view().info().parentCloseTime;
{
auto const now = ctx_.view().info().parentCloseTime;
// No cancel time specified: can't execute at all. // No cancel time specified: can't execute at all.
if (!(*slep)[~sfCancelAfter]) if (!(*slep)[~sfCancelAfter])
return tecNO_PERMISSION; return tecNO_PERMISSION;
// Too soon: can't execute before the cancel time. // Too soon: can't execute before the cancel time.
if (!after(now, (*slep)[sfCancelAfter])) if (!after(now, (*slep)[sfCancelAfter]))
return tecNO_PERMISSION; return tecNO_PERMISSION;
}
else
{
// This is old code that needs to stay to support replaying old ledgers,
// but does not need to be covered by new tests.
// LCOV_EXCL_START
// Too soon?
if (!(*slep)[~sfCancelAfter] ||
ctx_.view().info().parentCloseTime.time_since_epoch().count() <=
(*slep)[sfCancelAfter])
return tecNO_PERMISSION;
// LCOV_EXCL_STOP
}
AccountID const account = (*slep)[sfAccount]; AccountID const account = (*slep)[sfAccount];