Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduce MPT support (XLS-33d): #5143
Changes from 2 commits
6d6fda2
aaf7fe5
f84e9ea
18515dd
aef1426
9136a89
16a029a
2beb2d9
3cc7003
069273c
86bcebd
b9cb9ca
8af452e
fab0c78
ea887bf
d9fc97f
98d419f
3b7861d
93c545d
aacb1c8
560cf91
6a7a8c2
8796641
757b55a
11aa41c
eac438c
9d51389
8774b90
4bcf463
9b8564e
ac45342
af29f41
9e3cfef
4bbf613
3bb44c2
0bf5b42
924dd9c
31b3be3
c3f2bd4
29c6f5d
bd97cc9
38781df
e239fcb
fe6fa60
f4bc120
780af7a
e4009c8
5f33c80
e8c8329
c53df59
b8f3b83
5349827
0905e37
9d84f3b
e8201f3
5aa36e4
7a6272e
3439bfa
b636836
7c08b58
9ce2061
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 toXRPAmount
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:
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.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 thatAmount
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. AndXRPAmount
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 thinksNumber
is better. There are over 300 instances ofXRPAmount
,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
andXRPAmount
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 thatSTAmount
has quantity + issue, while these{XRP,IOU,MPT}Amount
s only have quantity, likeNumber
, which is their common representation. The alternative fix is to renameSTAmount
to a different suffix, but I think that would be even more disruptive, especially to external projects.