-
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
Patch CTID #4738
Open
dangell7
wants to merge
20
commits into
XRPLF:develop
Choose a base branch
from
Transia-RnD:patch-ctid
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Patch CTID #4738
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
98b139b
ctid everywhere
RichardAH 6996d08
ensure subscription has ctid
RichardAH fc8477e
add wrong network error code
dangell7 ee8bbf1
add wrong network id rpc test
dangell7 9b889e1
clang-format
dangell7 9575307
Merge branch 'develop' into patch-ctid
dangell7 67af70e
Merge branch 'develop' into patch-ctid
dangell7 ffad0af
add tx tests
dangell7 8e1f5fb
clang-format
dangell7 198f006
Merge branch 'develop' into patch-ctid
dangell7 51d78a1
[FOLD] A collection of suggested changes from review (#24)
dangell7 d3d98d2
Merge branch 'develop' into patch-ctid
dangell7 c61475f
address review updates
dangell7 68b9b65
Merge branch 'develop' into patch-ctid
dangell7 12b8740
Merge branch 'develop' into patch-ctid
dangell7 9c16620
Update Tx.cpp
dangell7 b90428a
Merge branch 'develop' into patch-ctid
dangell7 65568cf
Update Transaction.cpp
dangell7 a02a04d
Merge branch 'develop' into patch-ctid
dangell7 47df4c2
Merge branch 'develop' into patch-ctid
dangell7 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
@scottschurr Can you look at this? Wouldn't sfTransactionIndex always exist? If I do
account_tx
on a non validated ledger I get "Ledger not validated."SetStatus
values are optional. So can we just call this every time?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.
Or how do I test
account_tx
with a tx that doesnt have a sfTransactionIndex?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.
@dangell7, thanks for asking. I can't immediately think of a situation where you could retrieve metadata for a transaction from a ledger and not have
sfTransactionIndex
filled in. However I prefer the conservative approach of verifying that the field is present before accessing it. Bugs happen. Doing checks like this can prevent a bug elsewhere from turning catastrophic.Taking a step back, we ran for a few years without code coverage as part of CI. And, thanks to some great work by good folks, we started getting code coverage reports again recently. But we're still learning how to best apply the tool. There will be code that can't be exercised by reasonable tests (testing
asserts
anyone?). We have a new microscope and we've discovered that there are tiny mites living on our skin. Over time we'll learn to relax and know we don't have to check that each individual mite is harmless.Having said that, if I were bound and determined to exercise the
else
condition here, I'd doing it by calling theconvertBlobsToTxResult()
function directly. That would allow me to pass in whatever I wanted in therawMeta
argument. This test would pass inrawMeta
that omitted ansfTransactionIndex
. I'd guess that getting that test in place might take a day or so of effort.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.
Awesome. Thank you for this response. I do like the code coverage stuff. I will take a look at how to get some more coverage on this.
convertBlobsToTxResult
sounds pretty simple. Thanks