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

Introduce MPT support (XLS-33d): #5143

Merged
merged 61 commits into from
Oct 29, 2024
Merged
Changes from 1 commit
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
6d6fda2
Introduce MPT support (XLS-33d):
gregtatcam Jun 28, 2024
aaf7fe5
Merge branch 'develop' into feature/mpt-v1-var-issues
gregtatcam Sep 30, 2024
f84e9ea
Apply suggestions from code review
gregtatcam Oct 1, 2024
18515dd
Address reviewer's feedback
gregtatcam Oct 1, 2024
aef1426
Allow MPT SendMax in Payment tx.
gregtatcam Oct 1, 2024
9136a89
Apply suggestions from code review
gregtatcam Oct 2, 2024
16a029a
Address reviewer's feedback
gregtatcam Oct 2, 2024
2beb2d9
Manually align jss and disable clang-format
gregtatcam Oct 3, 2024
3cc7003
Apply suggestions from code review
gregtatcam Oct 3, 2024
069273c
Fix clang format
gregtatcam Oct 3, 2024
86bcebd
Apply suggestions from code review
gregtatcam Oct 3, 2024
b9cb9ca
Address reviewer's feedback
gregtatcam Oct 3, 2024
8af452e
remove unneeded fields (#36)
shawnxie999 Oct 3, 2024
fab0c78
rename codes (#37)
shawnxie999 Oct 3, 2024
ea887bf
Address reviewer feedback
gregtatcam Oct 3, 2024
d9fc97f
Check Payment's sendMax and amount have the same issue
gregtatcam Oct 4, 2024
98d419f
Apply suggestions from code review
gregtatcam Oct 4, 2024
3b7861d
Address reviewer's feedback
gregtatcam Oct 7, 2024
93c545d
Fix static_assert in STVar and other changes
gregtatcam Oct 7, 2024
aacb1c8
comment (#38)
shawnxie999 Oct 7, 2024
560cf91
Fix SendMax with no transfer fee
gregtatcam Oct 7, 2024
6a7a8c2
Combine Issue/MPTIssue handling in Payment transactor plus other changes
gregtatcam Oct 9, 2024
8796641
Fix non-unity build
gregtatcam Oct 9, 2024
757b55a
Update src/libxrpl/protocol/TxFormats.cpp
gregtatcam Oct 9, 2024
11aa41c
New SField flag to override hex input/output formatting to use decima…
ximinez Oct 10, 2024
eac438c
Change maxAmount in the test to uint64 for better visualization
gregtatcam Oct 10, 2024
9d51389
Refactor Clawback plus other minor refactoring
gregtatcam Oct 10, 2024
8774b90
Fix requireAuth (#40)
shawnxie999 Oct 11, 2024
4bcf463
Address reviewer's feedback
gregtatcam Oct 11, 2024
9b8564e
Fix metadata and add metadata unit-test
gregtatcam Oct 12, 2024
ac45342
Address reviewer's feedback:
gregtatcam Oct 14, 2024
af29f41
use mptJson
shawnxie999 Oct 15, 2024
9e3cfef
Add += and -= operators to ValueProxy
gregtatcam Oct 15, 2024
4bbf613
ledger_entry test (#41)
shawnxie999 Oct 15, 2024
3bb44c2
Simplify constraint for +=, -= of ValueProxy
gregtatcam Oct 16, 2024
0bf5b42
Address reviewer's feedback on mpt uni-test helper
gregtatcam Oct 16, 2024
924dd9c
refactor mpt_issuance_id into SLE::getJson (#42)
shawnxie999 Oct 16, 2024
31b3be3
Merge branch 'develop' into feature/mpt-v1-var-issues
gregtatcam Oct 16, 2024
c3f2bd4
Fix clang-format
gregtatcam Oct 16, 2024
29c6f5d
Merge with the latest develop
gregtatcam Oct 17, 2024
bd97cc9
Address reviewer's feedback
gregtatcam Oct 17, 2024
38781df
Fix inline isFrozen definition
gregtatcam Oct 17, 2024
e239fcb
Address reviewer feedback
gregtatcam Oct 17, 2024
fe6fa60
Fix clang-format
gregtatcam Oct 18, 2024
f4bc120
Address reviewer's feedback
gregtatcam Oct 18, 2024
780af7a
removed mptHolders (#43)
shawnxie999 Oct 18, 2024
e4009c8
address comments (#44)
shawnxie999 Oct 18, 2024
5f33c80
move flags (#45)
shawnxie999 Oct 21, 2024
e8c8329
Address reviewer's feedback - refactor mpt.[h,cpp]
gregtatcam Oct 21, 2024
c53df59
Apply suggestions from code review - update MPT unit-tests
gregtatcam Oct 21, 2024
b8f3b83
Fix clang-format
gregtatcam Oct 21, 2024
5349827
Address reviewer's feedback - add MPT unit-tests
gregtatcam Oct 21, 2024
0905e37
Apply suggestions from code review
gregtatcam Oct 22, 2024
9d84f3b
Address reviewer's feedback
gregtatcam Oct 22, 2024
e8201f3
address comments (#46)
shawnxie999 Oct 23, 2024
5aa36e4
Apply suggestions from code review
gregtatcam Oct 24, 2024
7a6272e
Address reviewer's feedback
gregtatcam Oct 24, 2024
3439bfa
comments (#48)
shawnxie999 Oct 25, 2024
b636836
Rename mpt holder (#49)
shawnxie999 Oct 28, 2024
7c08b58
Address reviewer's feedback
gregtatcam Oct 29, 2024
9ce2061
Ed's comments (#50)
shawnxie999 Oct 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Rename mpt holder (#49)
* rename

* space
shawnxie999 authored Oct 28, 2024
commit b636836a47b48f758d64ca4ea96ee42a5035fde3
2 changes: 1 addition & 1 deletion include/xrpl/protocol/detail/sfields.macro
Original file line number Diff line number Diff line change
@@ -270,7 +270,7 @@ TYPED_SFIELD(sfUnauthorize, ACCOUNT, 6)
TYPED_SFIELD(sfRegularKey, ACCOUNT, 8)
TYPED_SFIELD(sfNFTokenMinter, ACCOUNT, 9)
TYPED_SFIELD(sfEmitCallback, ACCOUNT, 10)
TYPED_SFIELD(sfMPTokenHolder, ACCOUNT, 11)
TYPED_SFIELD(sfHolder, ACCOUNT, 11)

// account (uncommon)
TYPED_SFIELD(sfHookAccount, ACCOUNT, 16)
6 changes: 3 additions & 3 deletions include/xrpl/protocol/detail/transactions.macro
Original file line number Diff line number Diff line change
@@ -224,7 +224,7 @@ TRANSACTION(ttNFTOKEN_ACCEPT_OFFER, 29, NFTokenAcceptOffer, ({
/** This transaction claws back issued tokens. */
TRANSACTION(ttCLAWBACK, 30, Clawback, ({
{sfAmount, soeREQUIRED, soeMPTSupported},
{sfMPTokenHolder, soeOPTIONAL},
{sfHolder, soeOPTIONAL},
}))

/** This transaction type creates an AMM instance */
@@ -403,13 +403,13 @@ TRANSACTION(ttMPTOKEN_ISSUANCE_DESTROY, 55, MPTokenIssuanceDestroy, ({
/** This transaction type sets flags on a MPTokensIssuance or MPToken instance */
TRANSACTION(ttMPTOKEN_ISSUANCE_SET, 56, MPTokenIssuanceSet, ({
{sfMPTokenIssuanceID, soeREQUIRED},
{sfMPTokenHolder, soeOPTIONAL},
{sfHolder, soeOPTIONAL},
}))

/** This transaction type authorizes a MPToken instance */
TRANSACTION(ttMPTOKEN_AUTHORIZE, 57, MPTokenAuthorize, ({
{sfMPTokenIssuanceID, soeREQUIRED},
{sfMPTokenHolder, soeOPTIONAL},
{sfHolder, soeOPTIONAL},
}))

/** This system-generated transaction type is used to update the status of the various amendments.
2 changes: 1 addition & 1 deletion src/test/app/MPToken_test.cpp
Original file line number Diff line number Diff line change
@@ -1648,7 +1648,7 @@ class MPToken_test : public beast::unit_test::suite
void
testTxJsonMetaFields(FeatureBitset features)
{
// checks synthetically parsed mptissuanceid from `tx` response
// checks synthetically injected mptissuanceid from `tx` response
testcase("Test synthetic fields from tx response");

using namespace test::jtx;
4 changes: 2 additions & 2 deletions src/test/jtx/impl/mpt.cpp
Original file line number Diff line number Diff line change
@@ -157,7 +157,7 @@ MPTTester::authorize(MPTAuthorize const& arg)
jv[sfMPTokenIssuanceID] = to_string(*id_);
}
if (arg.holder)
jv[sfMPTokenHolder] = arg.holder->human();
jv[sfHolder] = arg.holder->human();
if (auto const result = submit(arg, jv); result == tesSUCCESS)
{
// Issuer authorizes
@@ -230,7 +230,7 @@ MPTTester::set(MPTSet const& arg)
jv[sfMPTokenIssuanceID] = to_string(*id_);
}
if (arg.holder)
jv[sfMPTokenHolder] = arg.holder->human();
jv[sfHolder] = arg.holder->human();
if (submit(arg, jv) == tesSUCCESS && arg.flags.value_or(0))
{
auto require = [&](std::optional<Account> const& holder,
2 changes: 1 addition & 1 deletion src/test/jtx/impl/trust.cpp
Original file line number Diff line number Diff line change
@@ -75,7 +75,7 @@ claw(
jv[jss::TransactionType] = jss::Clawback;

if (mptHolder)
jv[sfMPTokenHolder.jsonName] = mptHolder->human();
jv[sfHolder.jsonName] = mptHolder->human();

return jv;
}
11 changes: 5 additions & 6 deletions src/xrpld/app/tx/detail/Clawback.cpp
Original file line number Diff line number Diff line change
@@ -36,7 +36,7 @@
NotTEC
preflightHelper<Issue>(PreflightContext const& ctx)
{
if (ctx.tx.isFieldPresent(sfMPTokenHolder))
if (ctx.tx.isFieldPresent(sfHolder))
return temMALFORMED;

AccountID const issuer = ctx.tx[sfAccount];
@@ -58,7 +58,7 @@
if (!ctx.rules.enabled(featureMPTokensV1))
return temDISABLED;

auto const mptHolder = ctx.tx[~sfMPTokenHolder];
auto const mptHolder = ctx.tx[~sfHolder];
auto const clawAmount = ctx.tx[sfAmount];

if (!mptHolder)
@@ -82,7 +82,7 @@
return temDISABLED;

if (auto const ret = preflight1(ctx); !isTesSuccess(ret))
return ret;

Check warning on line 85 in src/xrpld/app/tx/detail/Clawback.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/Clawback.cpp#L85

Added line #L85 was not covered by tests

if (ctx.tx.getFlags() & tfClawbackMask)
return temINVALID_FLAG;
@@ -199,9 +199,8 @@
{
AccountID const issuer = ctx.tx[sfAccount];
auto const clawAmount = ctx.tx[sfAmount];
AccountID const holder = clawAmount.holds<Issue>()
? clawAmount.getIssuer()
: ctx.tx[sfMPTokenHolder];
AccountID const holder =
clawAmount.holds<Issue>() ? clawAmount.getIssuer() : ctx.tx[sfHolder];

auto const sleIssuer = ctx.view.read(keylet::account(issuer));
auto const sleHolder = ctx.view.read(keylet::account(holder));
@@ -209,7 +208,7 @@
return terNO_ACCOUNT;

if (sleHolder->isFieldPresent(sfAMMID))
return tecAMM_ACCOUNT;

Check warning on line 211 in src/xrpld/app/tx/detail/Clawback.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/Clawback.cpp#L211

Added line #L211 was not covered by tests

return std::visit(
[&]<typename T>(T const&) {
@@ -260,7 +259,7 @@
{
AccountID const issuer = ctx.tx[sfAccount];
auto clawAmount = ctx.tx[sfAmount];
AccountID const holder = ctx.tx[sfMPTokenHolder];
AccountID const holder = ctx.tx[sfHolder];

// Get the spendable balance. Must use `accountHolds`.
STAmount const spendableAmount = accountHolds(
2 changes: 1 addition & 1 deletion src/xrpld/app/tx/detail/InvariantCheck.cpp
Original file line number Diff line number Diff line change
@@ -928,7 +928,7 @@
{
JLOG(j.fatal())
<< "Invariant failed: trustline balance is negative";
return false;

Check warning on line 931 in src/xrpld/app/tx/detail/InvariantCheck.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/InvariantCheck.cpp#L931

Added line #L931 was not covered by tests
}
}
}
@@ -945,7 +945,7 @@
{
JLOG(j.fatal()) << "Invariant failed: some mptokens were changed "
"despite failure of the transaction.";
return false;

Check warning on line 948 in src/xrpld/app/tx/detail/InvariantCheck.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/InvariantCheck.cpp#L948

Added line #L948 was not covered by tests
}
}

@@ -1031,19 +1031,19 @@

if (tx.getTxnType() == ttMPTOKEN_AUTHORIZE)
{
bool const submittedByIssuer = tx.isFieldPresent(sfMPTokenHolder);
bool const submittedByIssuer = tx.isFieldPresent(sfHolder);

if (mptIssuancesCreated_ > 0)
{
JLOG(j.fatal()) << "Invariant failed: MPT authorize "
"succeeded but created MPT issuances";
return false;

Check warning on line 1040 in src/xrpld/app/tx/detail/InvariantCheck.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/InvariantCheck.cpp#L1040

Added line #L1040 was not covered by tests
}
else if (mptIssuancesDeleted_ > 0)
{
JLOG(j.fatal()) << "Invariant failed: MPT authorize "
"succeeded but deleted issuances";
return false;

Check warning on line 1046 in src/xrpld/app/tx/detail/InvariantCheck.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/InvariantCheck.cpp#L1046

Added line #L1046 was not covered by tests
}
else if (
submittedByIssuer &&
@@ -1052,7 +1052,7 @@
JLOG(j.fatal())
<< "Invariant failed: MPT authorize submitted by issuer "
"succeeded but created/deleted mptokens";
return false;

Check warning on line 1055 in src/xrpld/app/tx/detail/InvariantCheck.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/InvariantCheck.cpp#L1055

Added line #L1055 was not covered by tests
}
else if (
!submittedByIssuer &&
@@ -1063,7 +1063,7 @@
JLOG(j.fatal())
<< "Invariant failed: MPT authorize submitted by holder "
"succeeded but created/deleted bad number of mptokens";
return false;

Check warning on line 1066 in src/xrpld/app/tx/detail/InvariantCheck.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/InvariantCheck.cpp#L1066

Added line #L1066 was not covered by tests
}

return true;
6 changes: 3 additions & 3 deletions src/xrpld/app/tx/detail/MPTokenAuthorize.cpp
Original file line number Diff line number Diff line change
@@ -32,12 +32,12 @@
return temDISABLED;

if (auto const ret = preflight1(ctx); !isTesSuccess(ret))
return ret;

Check warning on line 35 in src/xrpld/app/tx/detail/MPTokenAuthorize.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenAuthorize.cpp#L35

Added line #L35 was not covered by tests

if (ctx.tx.getFlags() & tfMPTokenAuthorizeMask)
return temINVALID_FLAG;

if (ctx.tx[sfAccount] == ctx.tx[~sfMPTokenHolder])
if (ctx.tx[sfAccount] == ctx.tx[~sfHolder])
return temMALFORMED;

return preflight2(ctx);
@@ -47,7 +47,7 @@
MPTokenAuthorize::preclaim(PreclaimContext const& ctx)
{
auto const accountID = ctx.tx[sfAccount];
auto const holderID = ctx.tx[~sfMPTokenHolder];
auto const holderID = ctx.tx[~sfHolder];

// if non-issuer account submits this tx, then they are trying either:
// 1. Unauthorize/delete MPToken
@@ -78,7 +78,7 @@
keylet::mptIssuance(ctx.tx[sfMPTokenIssuanceID]));
assert(sleMptIssuance);
if (!sleMptIssuance)
return tefINTERNAL;

Check warning on line 81 in src/xrpld/app/tx/detail/MPTokenAuthorize.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenAuthorize.cpp#L81

Added line #L81 was not covered by tests

return tecHAS_OBLIGATIONS;
}
@@ -144,7 +144,7 @@
{
auto const sleAcct = view.peek(keylet::account(args.account));
if (!sleAcct)
return tecINTERNAL;

Check warning on line 147 in src/xrpld/app/tx/detail/MPTokenAuthorize.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenAuthorize.cpp#L147

Added line #L147 was not covered by tests

// If the account that submitted the tx is a holder
// Note: `account_` is holder's account
@@ -160,14 +160,14 @@
keylet::mptoken(args.mptIssuanceID, args.account);
auto const sleMpt = view.peek(mptokenKey);
if (!sleMpt || (*sleMpt)[sfMPTAmount] != 0)
return tecINTERNAL;

Check warning on line 163 in src/xrpld/app/tx/detail/MPTokenAuthorize.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenAuthorize.cpp#L163

Added line #L163 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's good to keep the same informative error codes returned by preclaim(), here and elsewhere. Consider future callers trying to debug.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The rule that I observ is that errors in apply are generally tecINTERNAL since the these errors should've already been caught in the preclaim in the first place, so we'd never expect this condition be triggered.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that's a good reason.

Copy link
Collaborator

@shawnxie999 shawnxie999 Oct 24, 2024

Choose a reason for hiding this comment

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

I think this is practice the codebase currently has, not sure if we want to change it. Perhaps @ximinez can weigh in

Copy link
Collaborator

@ximinez ximinez Oct 24, 2024

Choose a reason for hiding this comment

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

If something has already been checked in an earlier step (like preflight or preclaim), and thus should be impossible, then our practice has been to return tef/tecINTERNAL in later steps.

Think of it more like an assert: We want to know something went wrong, but we don't want to crash the server. Particularly with tecINTERNAL, if we see that on a validated ledger, we know something bad happened, and we can replay the transaction to see what happened and fix it. If we just return the same error codes that preclaim() returned, we'd never know there was a problem.


if (!view.dirRemove(
keylet::ownerDir(args.account),
(*sleMpt)[sfOwnerNode],
sleMpt->key(),
false))
return tecINTERNAL;

Check warning on line 170 in src/xrpld/app/tx/detail/MPTokenAuthorize.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenAuthorize.cpp#L170

Added line #L170 was not covered by tests

adjustOwnerCount(view, sleAcct, -1, journal);

@@ -201,7 +201,7 @@
describeOwnerDir(args.account));

if (!ownerNode)
return tecDIR_FULL;

Check warning on line 204 in src/xrpld/app/tx/detail/MPTokenAuthorize.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenAuthorize.cpp#L204

Added line #L204 was not covered by tests

auto mptoken = std::make_shared<SLE>(mptokenKey);
(*mptoken)[sfAccount] = args.account;
@@ -219,18 +219,18 @@
auto const sleMptIssuance =
view.read(keylet::mptIssuance(args.mptIssuanceID));
if (!sleMptIssuance)
return tecINTERNAL;

Check warning on line 222 in src/xrpld/app/tx/detail/MPTokenAuthorize.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenAuthorize.cpp#L222

Added line #L222 was not covered by tests

// If the account that submitted this tx is the issuer of the MPT
// Note: `account_` is issuer's account
// `holderID` is holder's account
if (args.account != (*sleMptIssuance)[sfIssuer])
return tecINTERNAL;

Check warning on line 228 in src/xrpld/app/tx/detail/MPTokenAuthorize.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenAuthorize.cpp#L228

Added line #L228 was not covered by tests

auto const sleMpt =
view.peek(keylet::mptoken(args.mptIssuanceID, *args.holderID));
if (!sleMpt)
return tecINTERNAL;

Check warning on line 233 in src/xrpld/app/tx/detail/MPTokenAuthorize.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenAuthorize.cpp#L233

Added line #L233 was not covered by tests

std::uint32_t const flagsIn = sleMpt->getFieldU32(sfFlags);
std::uint32_t flagsOut = flagsIn;
@@ -262,7 +262,7 @@
.mptIssuanceID = tx[sfMPTokenIssuanceID],
.account = account_,
.flags = tx.getFlags(),
.holderID = tx[~sfMPTokenHolder]});
.holderID = tx[~sfHolder]});
}

} // namespace ripple
6 changes: 3 additions & 3 deletions src/xrpld/app/tx/detail/MPTokenIssuanceSet.cpp
Original file line number Diff line number Diff line change
@@ -32,7 +32,7 @@
return temDISABLED;

if (auto const ret = preflight1(ctx); !isTesSuccess(ret))
return ret;

Check warning on line 35 in src/xrpld/app/tx/detail/MPTokenIssuanceSet.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenIssuanceSet.cpp#L35

Added line #L35 was not covered by tests

auto const txFlags = ctx.tx.getFlags();

@@ -44,7 +44,7 @@
return temINVALID_FLAG;

auto const accountID = ctx.tx[sfAccount];
auto const holderID = ctx.tx[~sfMPTokenHolder];
auto const holderID = ctx.tx[~sfHolder];
if (holderID && accountID == holderID)
return temMALFORMED;

@@ -68,7 +68,7 @@
if ((*sleMptIssuance)[sfIssuer] != ctx.tx[sfAccount])
return tecNO_PERMISSION;

if (auto const holderID = ctx.tx[~sfMPTokenHolder])
if (auto const holderID = ctx.tx[~sfHolder])
{
// make sure holder account exists
if (!ctx.view.exists(keylet::account(*holderID)))
@@ -88,7 +88,7 @@
{
auto const mptIssuanceID = ctx_.tx[sfMPTokenIssuanceID];
auto const txFlags = ctx_.tx.getFlags();
auto const holderID = ctx_.tx[~sfMPTokenHolder];
auto const holderID = ctx_.tx[~sfHolder];
std::shared_ptr<SLE> sle;

if (holderID)
@@ -97,7 +97,7 @@
sle = view().peek(keylet::mptIssuance(mptIssuanceID));

if (!sle)
return tecINTERNAL;

Check warning on line 100 in src/xrpld/app/tx/detail/MPTokenIssuanceSet.cpp

Codecov / codecov/patch

src/xrpld/app/tx/detail/MPTokenIssuanceSet.cpp#L100

Added line #L100 was not covered by tests

std::uint32_t const flagsIn = sle->getFieldU32(sfFlags);
std::uint32_t flagsOut = flagsIn;
Loading