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
feat: support Concise Transaction Identifier (CTID) (XLS-37) #4418
feat: support Concise Transaction Identifier (CTID) (XLS-37) #4418
Changes from 25 commits
5e4f8ea
1daf5f7
91b0aae
2752108
3782ab7
1aaac47
04be722
fec9fb7
281f20b
2dad662
f39a839
ccea8c9
3a96000
5058592
4073a85
9decc31
701af50
8625bb3
0728556
802144b
7f7a93d
55cbbbd
38170b6
413388d
1f61853
027949c
b2ddfde
80f308f
32a27c5
61d2f5b
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.
Micro-nit: prefer
for(auto const& tx : lgr->txs)
instead.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.
Do we need to add "rpcWRONG_NETWORK" in unorderedErrorInfos ( ErrorCodes.cpp )
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'm not following why this is a string instead of a uint64_t.
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 because the intention is for a CTID to always be 16 hex digits, and there's no "easy" way to format that from a
std::uint64_t
(hence the ugly bit of code.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.
Bit late :)
But yeah
{network_id, ledger_seq, txn_index}
https:/XRPLF/XRPL-Standards/discussions/91\#discussioncomment-5230494 https:/XRPLF/XRPL-Standards/discussions/91\#discussioncomment-5079139
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.
Consider:
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.
Consider doing groups for 4 here (see comment above).
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'm not following why this is a string instead of a u64. (I'm not saying there isn't a reason, I'm just not seeing why it's done).
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.
Two reasons:
C
and identify it easily). If it is to be placed into the JSON as a hex-encoded string why store it in an intermediate state first?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.
That makes sense. Thanks @RichardAH!
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.
Same comments here.
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'd recommend putting these checks in a single place (namely in
encodeCTID
) so that you don't need to check in "unrelated" places and keep the code cleaner.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 personally prefer to avoid
std::stringstream
. I'd recommend: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.
Unneeded if you adopt my
std::from_chars
recommendation.