Skip to content

XLS-68: Sponsor#5887

Open
tequdev wants to merge 169 commits intoXRPLF:developfrom
tequdev:sponsor
Open

XLS-68: Sponsor#5887
tequdev wants to merge 169 commits intoXRPLF:developfrom
tequdev:sponsor

Conversation

@tequdev
Copy link
Member

@tequdev tequdev commented Oct 14, 2025

High Level Overview of Change

Context of Change

Type of Change

  • New feature (non-breaking change which adds functionality)

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)

{
// uncomment if you want to log the invariant failure
// std::cerr << " --> " << m << std::endl;
// std::cerr << " expected --> " << m << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The std::cerr lines should be deleted, as well as line 98

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these lines are well worth keeping for debugging purposes.

Copy link
Collaborator

@yinyiqian1 yinyiqian1 Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I see. Just verified that the existing rippled/develop is already having the std::cerr.
So maybe we don't need to make any change here.

if (account != sponsor)
return temMALFORMED;

if ((flags & tfSponsorshipSetRequireSignForFee) && (flags & tfSponsorshipClearRequireSignForFee))
Copy link
Collaborator

@yinyiqian1 yinyiqian1 Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was suggesting in my previous comment (#5887 (comment)) that we should move the flag check for deleting object into our main if (flags & tfDeleteObject) section.

//////  this was in your previous version in the beginning of preflight.
 if (flags & tfDeleteObject)
        {
            // check Flags
            if (flags &
                (tfSponsorshipSetRequireSignForFee | tfSponsorshipSetRequireSignForReserve |
                 tfSponsorshipClearRequireSignForFee | tfSponsorshipClearRequireSignForReserve))
                return temINVALID_FLAG;
        }

The else section should not be touched.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

fe07547

Copy link
Collaborator

@yinyiqian1 yinyiqian1 Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This did not address the comment either.

  1. You have a global check for the flags, but you checked them again in the else section.
  2. We should not use two if (flags & tfDeleteObject)... else ... in preflight. Please combine in one if else.

The code will be like:

NotTEC
SponsorshipSet::preflight(PreflightContext const& ctx)
{
    auto const flags = ctx.tx.getFlags();

    if ((flags & tfSponsorshipSetRequireSignForFee) && (flags & tfSponsorshipClearRequireSignForFee))
        return temINVALID_FLAG;
    if ((flags & tfSponsorshipSetRequireSignForReserve) && (flags & tfSponsorshipClearRequireSignForReserve))
        return temINVALID_FLAG;

   //.....

    if (flags & tfDeleteObject)
    {
        // can not combine with any modification flags when deleting
        constexpr std::uint32_t modifyFlags = tfSponsorshipSetRequireSignForFee |
            tfSponsorshipSetRequireSignForReserve | tfSponsorshipClearRequireSignForFee |
            tfSponsorshipClearRequireSignForReserve;

        if (flags & modifyFlags)
            return temINVALID_FLAG;
        // can not include these fields when deleting
        if (ctx.tx.isFieldPresent(sfFeeAmount) || ctx.tx.isFieldPresent(sfReserveCount) ||
            ctx.tx.isFieldPresent(sfMaxFee))
            return temMALFORMED;
    }
    else
    {
        // ...
    }

    return tesSUCCESS;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that. Now I understand what you were pointing out.

TYPED_SFIELD(sfSponsor, ACCOUNT, 27)
TYPED_SFIELD(sfHighSponsor, ACCOUNT, 28)
TYPED_SFIELD(sfLowSponsor, ACCOUNT, 29)
TYPED_SFIELD(sfCounterpartySponsor, ACCOUNT, 30)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XLS-68 updated, sfCounterpartySponsor not present

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvadari
I believe this field is necessary, but was it missing from the spec?

featureSponsor,
noPriv,
({
{sfCounterpartySponsor, soeOPTIONAL},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XLS-68 updated, please fix (sfSponsor)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

featureSponsor,
noPriv,
({
{sfCounterpartySponsor, soeOPTIONAL},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XLS updated, sfSponsor

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Amendment Blocked: Needs Final XLS The corresponding final XLS must have been merged before this PR is merged. QE test required RippleX QE Team must look at this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants