Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XRPFees: Fee setting and handling improvements #4247

Merged
merged 11 commits into from Feb 3, 2023
22 changes: 13 additions & 9 deletions src/ripple/app/tx/impl/Change.cpp
Expand Up @@ -93,12 +93,16 @@ Change::preclaim(PreclaimContext const& ctx)
case ttFEE:
if (ctx.view.rules().enabled(featureXRPFees))
{
// The ttFEE transaction format defines these fields as
// optional, but once the XRPFees feature is enabled, they are
// forbidden.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are the required ones, right? Not forbidden? The code returns malformed if they are missing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, woops. Got those backwards.

if (!ctx.tx.isFieldPresent(sfBaseFeeDrops) ||
!ctx.tx.isFieldPresent(sfReserveBaseDrops) ||
!ctx.tx.isFieldPresent(sfReserveIncrementDrops))
return temMALFORMED;
// The transaction should only have one set of fields or the
// other.
// The ttFEE transaction format defines these fields as
// optional, but once the XRPFees feature is enabled, they are
// required.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, these are the forbidden ones.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, woops. Got those backwards.

if (ctx.tx.isFieldPresent(sfBaseFee) ||
ctx.tx.isFieldPresent(sfReferenceFeeUnits) ||
ctx.tx.isFieldPresent(sfReserveBase) ||
Expand All @@ -107,18 +111,18 @@ Change::preclaim(PreclaimContext const& ctx)
}
else
{
// The transaction format formerly enforced these fields as
// required. With featureXRPFees, those fields were made
// optional with the expectation that they won't be used once
// the feature is enabled. Since the feature hasn't yet
// been enabled, they should all still be here.
// The ttFEE transaction format formerly defined these fields
// as required. When the XRPFees feature was implemented, they
// were changed to be optional. Until the feature has been
// enabled, they are required.
if (!ctx.tx.isFieldPresent(sfBaseFee) ||
!ctx.tx.isFieldPresent(sfReferenceFeeUnits) ||
!ctx.tx.isFieldPresent(sfReserveBase) ||
!ctx.tx.isFieldPresent(sfReserveIncrement))
return temMALFORMED;
// The transaction should only have one or the other. If the new
// fields are present without the amendment, that's bad, too.
// The ttFEE transaction format defines these fields as
// optional, but without the XRPFees feature, they are
// forbidden.
if (ctx.tx.isFieldPresent(sfBaseFeeDrops) ||
ctx.tx.isFieldPresent(sfReserveBaseDrops) ||
ctx.tx.isFieldPresent(sfReserveIncrementDrops))
Expand Down