Transaction ProposalSign for ordinary and batch

[Kassaking7]

High Level Overview of Change

Context of Change

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[codecov]

Codecov Report

❌ Patch coverage is 87.37201% with 37 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...x/transactors/proposal/TransactionProposalSign.cpp	82.4%	18 Missing ⚠️
src/libxrpl/ledger/helpers/ProposalHelpers.cpp	88.1%	16 Missing ⚠️
src/libxrpl/tx/Transactor.cpp	80.0%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[ckeshava]

We can save compute cycles by moving this check to the top of the preclaim method.

[ckeshava]

If the proposedTx payload makes a mention of sfDelegate, the delegated-account must authorize this transaction. A TransactionProposalSign from the actual account must be rejected with an error code. The order of these two checks need to be flipped.

Reference documentation for Delegated transactions.

Other parts of rippled have switched the order of checks. i.e. we first check if sfDelegate is specified in the payload of the transaction, if not, we default to the sfAccount value.

Here is the test case that demonstrates this issue:

void
testDelegatedPayloadRejectsAccountSigningFor(FeatureBitset features)
{
    testcase("delegated payload rejects SigningFor = Account");

    using namespace jtx;
    using namespace std::chrono_literals;

    Env env{*this, features};

    Account const target{"target"};
    Account const dest{"dest"};
    Account const delegateAcct{"delegateAcct"};
    Account const ds1{"ds1"};
    env.fund(XRP(10000), target, dest, delegateAcct, ds1);
    env.close();

    env(delegate::set(target, delegateAcct, {"Payment"}));
    env(signers(delegateAcct, 1, {{ds1, 1}}));
    env.close();

    std::uint32_t const ticketSeq = proposal::createTicket(env, target);
    json::Value tx = pay(target, dest, XRP(1));
    tx[sfDelegate.jsonName] = delegateAcct.human();
    env(proposal::create(
        target,
        proposal::unsignedPayload(env, tx, ticketSeq, /*extraSigners=*/1),
        proposal::expiration(env, 100s)));
    env.close();

    // The Delegate authorizes this payload; the Account does not. Its
    // contribution must be refused rather than written into the payload's
    // own signature slot, where it would lock the Delegate out for good.
    env(proposal::sign(env, target, target, ticketSeq, target, target),
        Ter(tecNO_PERMISSION));
    env.close();

    {
        auto const sle = proposal::entry(env, target, ticketSeq);
        if (!BEAST_EXPECT(sle))
            return;
        auto const stored = sle->getFieldObject(sfProposedTransaction);
        BEAST_EXPECT(stored.getFieldVL(sfSigningPubKey).empty());
        BEAST_EXPECT(!stored.isFieldPresent(sfTxnSignature));
    }

    // The Delegate's signer is still able to contribute and complete it.
    env(proposal::sign(env, ds1, target, ticketSeq, delegateAcct, ds1));
    env.close();

    env(proposedJson(env, target, ticketSeq), Sig(kNone));
    env.close();

    BEAST_EXPECT(!proposal::entry(env, target, ticketSeq));
    BEAST_EXPECT(env.balance(dest) == XRP(10000) + XRP(1));
}