Introduce MPT support (XLS-33d): #5143
Conversation
gregtatcam
force-pushed
the
feature/mpt-v1-var-issues
branch
2 times, most recently
from
4ca85a2
to
ba5f67c
Compare
New Transactions: - MPTokenIssuanceCreate - MPTokenIssuanceDestory - MPTokenIssuanceSet - MPTokenAuthorize Modified Transactions: - Payment - Clawback New Objects: - MPTokenIssuance - MPTokenAuthorize API updates: - ledger_entry - account_objects - ledger_data Read full spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0033d-multi-purpose-tokens --------- Co-authored-by: Shawn Xie <shawnxie920@gmail.com> Co-authored-by: Howard Hinnant <howard.hinnant@gmail.com>
gregtatcam
force-pushed
the
feature/mpt-v1-var-issues
branch
from
ba5f67c
to
6d6fda2
Compare
| lsfMPTCanClawback = 0x00000040, | ||
|
|
||
| // ltMPTOKEN | ||
| lsfMPTAuthorized = 0x00000002, |
There was a problem hiding this comment.
lsfMPTLocked, lsfMPTCanLock and lsfMPTAuthorized do not have unique values within this enum. Is that on purpose?
There was a problem hiding this comment.
The LedgerSpecificFlags enum is really just a way to define a bunch of constants. The flags are unique per ledger object type. Those are divided by the comment / label before each one. It might be worthwhile to split them into one per object type, but that's probably beyond the scope of this PR.
Co-authored-by: Ed Hennis <ed@ripple.com>
Update MPT Payment errors to be consistent with IOU Payment.
|
|
||
| namespace ripple { | ||
|
|
||
| class MPTAmount : private boost::totally_ordered<MPTAmount>, |
There was a problem hiding this comment.
Seeing how similar MPTAmount is to XRPAmount it's a pity that they aren't implemented with common code. I.e. through a base class, or a template or something. Rather than diving down the potential rabbit hole of implementing it, I'm just going to leave this note here for someone else or for future reference.
There was a problem hiding this comment.
My opinion:
- They should be called
MPTNumberandXRPNumberbecause they represent quantities, whileSTAmountrepresents a quantity plus an asset / issue / unit (however you want to think of it). These are more likeNumber, and convert directly to and from it. - All of the arithmetic is moving to
Numberafter the switchover anyway. In due time, when we retire that amendment, effectively locking it in permanently, then I think we can removeMPTAmountandXRPAmount.
There was a problem hiding this comment.
Should IOUAmount change too? I think that Amount better communicates what the value is. I don't think you'd say number when talking about tokens or currencies even if the values don't have associate unit. And XRPAmount doesn't need an issue, it's implicit. Also doesn't seem like this refactoring has to be done in MPT PR. But this is just my opinion. I'll change if everyone thinks Number is better. There are over 300 instances of XRPAmount, MPTAmount, IOUAmount.
There was a problem hiding this comment.
I created a ticket to refactor MPTAmount and XRPAmount to use a common code + renaming.
There was a problem hiding this comment.
Yeah, I'm not asking it to be done here, or at all even. Was just stating my position. My opinion is that IOUAmount should be renamed too (in an ideal world), and my only reason for these renames is that STAmount has quantity + issue, while these {XRP,IOU,MPT}Amounts only have quantity, like Number, which is their common representation. The alternative fix is to rename STAmount to a different suffix, but I think that would be even more disruptive, especially to external projects.
Co-authored-by: Ed Hennis <ed@ripple.com>
|
|
||
| namespace ripple { | ||
|
|
||
| class MPTAmount : private boost::totally_ordered<MPTAmount>, |
There was a problem hiding this comment.
My opinion:
- They should be called
MPTNumberandXRPNumberbecause they represent quantities, whileSTAmountrepresents a quantity plus an asset / issue / unit (however you want to think of it). These are more likeNumber, and convert directly to and from it. - All of the arithmetic is moving to
Numberafter the switchover anyway. In due time, when we retire that amendment, effectively locking it in permanently, then I think we can removeMPTAmountandXRPAmount.
Co-authored-by: Ed Hennis <ed@ripple.com>
use std::min, std::max Co-authored-by: Ed Hennis <ed@ripple.com>
* Refactor getSNValue/getMPTValue * Use std::min, std::max * Fix min/max values for MPT
| if (flagsIn != flagsOut) | ||
| sle->setFieldU32(sfFlags, flagsOut); | ||
|
|
||
| view().update(sle); |
There was a problem hiding this comment.
You can move this statement into the previous branch.
There was a problem hiding this comment.
I think we'd like to keep it here because other transactors have it similar (eg. AccountSet). Also potential in the future we may add new functionalities to this transactor so might as well leave it here.
| auto const sleMptIssuance = ctx.view.read( | ||
| keylet::mptIssuance(ctx.tx[sfMPTokenIssuanceID])); | ||
| assert(sleMptIssuance); | ||
| if (!sleMptIssuance) | ||
| return tefINTERNAL; |
There was a problem hiding this comment.
I feel like this check is technically unnecessary. We assert it is always true.
There was a problem hiding this comment.
Ed proposed this change
There was a problem hiding this comment.
It would make more sense to remove the assert
| if (flagsIn != flagsOut) | ||
| sleMpt->setFieldU32(sfFlags, flagsOut); | ||
|
|
||
| view.update(sleMpt); |
There was a problem hiding this comment.
This line can move into the previous then-block.
There was a problem hiding this comment.
Doesn't hurt to leave it as it as is IMO
| if (!ctx.rules.enabled(featureMPTokensV1)) | ||
| return temDISABLED; | ||
|
|
||
| if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) |
There was a problem hiding this comment.
These branches, here and elsewhere, can all be simplified.
| if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) | |
| if (auto const ret = preflight1(ctx)) |
There was a problem hiding this comment.
I think this is fine. Every other transactions has this. And we are returning the tec code if it is not tesSuccess, so I dont see how this simplification would work.
There was a problem hiding this comment.
"Other transactions have this" is not a good reason. This simplification works because a TER evaluates to true if it is not success, just like a Linux error code.
There was a problem hiding this comment.
You know, I've gotten so used to seeing the symbolic isTesSuccess, that I've never really thought about the fact that tesSUCCESS == 0. But, yeah, this optimization / simplification should work.
The question is what does it gain us?
On one hand, if I see isTesSuccess (or any of the other is* test functions), I don't have to think about the underlying enum values. Additionally, isTesSuccess is defined as
inline bool
isTesSuccess(TER x)
{
return ((x) == tesSUCCESS);
}
Since that's inline, when compiled it should just boil down to x == 0. As an optimization, this probably doesn't gain much.
On the other hand, as a simplification, it does read a lot nicer, and fits the pattern we use everywhere else the result can be evaluated as a bool. I think I personally would adjust pretty quickly to seeing it this way, so I still wouldn't have to think about the underlying values.
My vote is to remove the call to isTesSuccess, and optionally add a comment anywhere it's removed to remind readers that this works.
| if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) | |
| // tesSUCCESS = 0, so any other return will be returned. | |
| if (auto const ret = preflight1(ctx)) |
There was a problem hiding this comment.
Another idea, similar to my first suggestion:
| if (auto const ret = preflight1(ctx); !isTesSuccess(ret)) | |
| // tesSUCCESS == 0. Any other value indicates an error. | |
| if (auto const err = preflight1(ctx)) |
(then return err;)
Using the name err instantly communicates "This function will return an error if it doesn't succeed."
Co-authored-by: John Freeman <jfreeman08@gmail.com>
|
#5042 is blocked by this PR |
Co-authored-by: Ed Hennis <ed@ripple.com>
* Update MPT unit-test
* rename * space
* Remove asserts in tests * Update copyright
shawnxie999
added
the
Passed
High Level Overview of Change
New Transactions:
Modified Transactions:
New Objects:
API updates:
Refactor:
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Test Plan
Added test for new feature:
Future Tasks
Integrate MPT into XRPL DEX.