-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
base: develop
Are you sure you want to change the base?
Conversation
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 <[email protected]> Co-authored-by: Howard Hinnant <[email protected]>
ba5f67c
to
6d6fda2
Compare
lsfMPTCanClawback = 0x00000040, | ||
|
||
// ltMPTOKEN | ||
lsfMPTAuthorized = 0x00000002, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lsfMPTLocked
, lsfMPTCanLock
and lsfMPTAuthorized
do not have unique values within this enum
. Is that on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitting the first phase of my review, a complete review of the changes to libxrpl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review:
Co-authored-by: Ed Hennis <[email protected]>
Update MPT Payment errors to be consistent with IOU Payment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
|
||
namespace ripple { | ||
|
||
class MPTAmount : private boost::totally_ordered<MPTAmount>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion:
- They should be called
MPTNumber
andXRPNumber
because they represent quantities, whileSTAmount
represents 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
Number
after the switchover anyway. In due time, when we retire that amendment, effectively locking it in permanently, then I think we can removeMPTAmount
andXRPAmount
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a ticket to refactor MPTAmount
and XRPAmount
to use a common code + renaming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}Amount
s 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 <[email protected]>
include/xrpl/protocol/Asset.h
Outdated
/* Asset is a variant of Issue (XRP and IOU) and MPTIssue (MPT). | ||
* It enables handling of different issues when either one is expected. | ||
* For instance, it extends STAmount class to support either issue | ||
* in a general way. It handles specifics such arithmetics and serialization | ||
* depending on specific issue type held by Asset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it odd that you think of this as "either one" (of two, Issue
or MPTIssue
) instead of "any one" (of three, XRP, IOU, or MPT). Don't we want our abstract ledger functions to think in terms of 3 issue types instead of 2 specific implementations?
I find the example (STAmount
) a little hard to understand, too. The "it" at the beginning of the last sentence refers to STAmount
, while the "it" in the sentence before is Asset
? I don't think "extends" is the right word either. Is the example just trying to convey that STAmount
uses Asset
to abstract arithmetic and serialization operations over all 3 issue types?
I'm left wondering why we have this Asset
instead of making Issue
support 3 issue types. STAmount
has a native() -> bool
method. Can Asset
have that too? Issue
has a getIssuer() -> AccountID
method. Can Asset
have that too? Why does Asset
exist if it's not going to offer any helper methods? With no helpers, it could just be a type alias for std::variant
.
|
||
namespace ripple { | ||
|
||
class MPTAmount : private boost::totally_ordered<MPTAmount>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion:
- They should be called
MPTNumber
andXRPNumber
because they represent quantities, whileSTAmount
represents 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
Number
after the switchover anyway. In due time, when we retire that amendment, effectively locking it in permanently, then I think we can removeMPTAmount
andXRPAmount
.
include/xrpl/basics/MPTAmount.h
Outdated
friend std::istream& | ||
operator>>(std::istream& s, MPTAmount& val); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was left behind.
friend std::istream& | |
operator>>(std::istream& s, MPTAmount& val); |
include/xrpl/protocol/Indexes.h
Outdated
MPTID | ||
makeMptID(AccountID const& account, std::uint32_t sequence); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of putting sequence
first in the parameter list? (Here and in the overload of mptIssuance()
, and maybe elsewhere.)
include/xrpl/protocol/Indexes.h
Outdated
mptoken(MPTID const& issuanceID, AccountID const& holder) noexcept; | ||
|
||
inline Keylet | ||
mptoken(uint256 const& issuanceKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter should be mptokenKey
right? I think I confused you with my earlier comment on the other parameters. I was just saying that the issuance
parameter name should end with Key
like issuanceKey
and mptokenKey
.
|
||
namespace ripple { | ||
|
||
class MPTIssue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does MPTIssue
give me that MPTID
does not? I'm guessing it's something like "so template code can call issue.getText()
regardless of whether the issue
object is an Issue
or an MPTIssue
." Is that right? I want the description to explain why this class exists and is not just a type alias of MPTID
.
include/xrpl/protocol/jss.h
Outdated
JSS(MPTokenIssuance); // ledger type. | ||
JSS(MPTokenIssuanceCreate); // transaction type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth it so we don't get radical re-formatting every time a few strings are added. We want the change set to highlight meaningful changes. GitHub's whitespace exclusion can hide these changes, but it can mask unwanted changes too. It's fine to restore the previous formatting so that this changeset has only the added strings.
/** MPTID is a 192-bit value representing MPT Issuance ID, | ||
* which is a concatenation of a 32-bit sequence (big endian) | ||
* and a 160-bit account */ | ||
using MPTID = base_uint<192>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shawnxie999 suggests they are serialized in this order because STAmount
is serialized in the order of currency then account ID. Whatever the reason, can you add it to this docstring, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is any relevance between MPTID
and how STAmount
is serialized. MPTID
is serialized as one block of data, there is no distinction between sequence and account in serialization. It could have been account first and then sequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah there isn't any relevance but since we STAmount is serialized as currency + accountID, and MPT amount is serialized as as sequence + accountID. We loosely decided to serialize MPTIssuanceID in the order of sequence + accountID as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
Co-authored-by: Ed Hennis <[email protected]>
use std::min, std::max Co-authored-by: Ed Hennis <[email protected]>
* Refactor getSNValue/getMPTValue * Use std::min, std::max * Fix min/max values for MPT
// MPTokenIssuanceCreate flags: | ||
constexpr std::uint32_t const tfMPTokenIssuanceCreateMask = | ||
~(tfMPTCanLock | tfMPTRequireAuth | tfMPTCanEscrow | tfMPTCanTrade | tfMPTCanTransfer | tfMPTCanClawback | tfUniversal); | ||
|
||
// MPTokenIssuanceDestroy flags: | ||
constexpr std::uint32_t const tfMPTokenIssuanceDestroyMask = ~tfUniversal; | ||
|
||
// MPTokenAuthorize flags: | ||
constexpr std::uint32_t const tfMPTokenAuthorizeMask = ~(tfMPTUnauthorize | tfUniversal); | ||
|
||
// MPTokenIssuanceSet flags: | ||
constexpr std::uint32_t const tfMPTokenIssuanceSetMask = ~(tfMPTLock | tfMPTUnlock | tfUniversal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the masks next to their flags, like the pattern for AMM
, and please put tfUniversal
first in the sequences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parital review
Number(XRPAmount const& x); | ||
Number(MPTAmount const& x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that clarification, @HowardHinnant ! I left the feedback originally because I assumed that we didn't want any accidental conversions to Number
from those other types. Now I understand those are desirable.
@gregtatcam Could you add a comment on these functions so that we have this info preserved for the future. It's probably fine to just copy Howard's comment here.
src/libxrpl/protocol/Asset.cpp
Outdated
if (holds<Issue>()) | ||
return get<Issue>().getIssuer(); | ||
return get<MPTIssue>().getIssuer(); | ||
return std::visit( | ||
[&](auto&& issue) -> AccountID const& { return issue.getIssuer(); }, | ||
issue_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
if (auto const& fName = getFName(); fName == sfMaximumAmount || | ||
fName == sfOutstandingAmount || fName == sfMPTAmount) | ||
{ | ||
return convertToString(value_, 10); // Convert to base 10 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the spirit of soeMPTSupported
, what do you think about adding an sMD_BaseTen
flag to SField
? I know other special case fields (e.g. sfLedgerEntryType
, sfTransactionResult
) use this pattern of checking against the name, but those are all one-offs. OTOH, these have several fields with this property, and there's a chance we'll add more in the future, so defining the behavior in one place is going to be easier to maintain, understand, and remember.
Here's what that would look like: gregtatcam/rippled@feature/mpt-v1-var-issues...ximinez:rippled:gt/5143/base10
(Should I open a PR against this branch?)
case STI_UINT192: | ||
construct<STUInt192>(name); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another case of, "while you're making changes to this file, how about refactoring this duplicated code into a single function?"
Co-authored-by: Ed Hennis <[email protected]>
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
libxrpl
change (any change that may affectlibxrpl
or dependents oflibxrpl
)Test Plan
Added test for new feature:
Future Tasks
Integrate MPT into XRPL DEX.