Currently, the ledger command only takes int for ledger_index - update the logic to allow string as well. #3582

gregtatcam · 2020-08-13T14:06:32Z

Assume ledger_index is quoted int if it is not "current", "closed", or "validated".

FIXES #3533

cjcobb23

Actually, @gregtatcam can you write a unit test for this? Then I'll approve

nbougalis

Looks good. Left some suggestions that I feel improve the code. I'd like to see them adopted unless anyone objects.

src/ripple/rpc/impl/RPCHelpers.cpp

src/test/rpc/LedgerRPC_test.cpp

scottschurr

Looks pretty good, but I'd like to request a couple of changes.

The DepositAuthorized unit tests are failing as a result of your change. You can choose to fix them yourself, or you could cherry-pick this commit which has a fix: scottschurr@3461cd4
The commit messages need some work. Here is generally good advice to follow: https://chris.beams.io/posts/git-commit/ But there are two specific changes I'd like to see

The first line of the first commit message is too long. It should be 60 characters or less. If there's more stuff coming then that first line should end with a colon followed by a blank line followed by the additional stuff. Consider something like this for that first commit message...

RPC ledger_index argument accepts quoted numeric strings:

Some RPC commands return ledger_index as a quoted numeric string.
This change allows the returned value to be directly copied and
used for follow-on RPC commands.

FIXES 3533

Presumably the remaining commits will want to be squashed into the first commit when this pull request is merged. Please start the commit message of any commits you want squashed with [FOLD]. The person assembling a release may not have been following the code review. Including [FOLD] helps the person building the release to know whether they have additional work to do. So the second commit message might look like this...

[FOLD] Update unit-test

The additional bug-fix and tuning commits should all include [FOLD] at the front. So, for example, the commit message for the third commit might look like...

[FOLD] Simplify ledger_index parsing:

Change lexicalCastChecked() argument to std::unit32_t.
Update unit-test.

I'm sure you get the idea. Thanks.

scottschurr · 2020-08-18T23:28:56Z

src/ripple/json/json_value.h

@@ -136,7 +136,7 @@ operator!=(StaticString x, std::string const& y)
 * The sequence of an #arrayValue will be automatically resize and initialized
 * with #nullValue. resize() can be used to enlarge or truncate an #arrayValue.
 *
- * The get() methods can be used to obtanis default value in the case the
+ * The get() methods can be used to obtain default value in the case the


Thanks for noticing and fixing the typo. I think you could slightly improve readability with "...to obtain a default...". But that's at your discretion.

Thank you. Updated.

scottschurr · 2020-08-18T23:42:11Z

src/test/rpc/LedgerRPC_test.cpp

@@ -393,7 +419,7 @@ class LedgerRPC_test : public beast::unit_test::suite
    void
    testLedgerEntryDepositPreauth()
    {
-        testcase("ledger_entry Request Directory");
+        testcase("ledger_entry Deposit Preauth");


Thanks for noticing and fixing this.

gregtatcam · 2020-08-24T17:04:55Z

Looks pretty good, but I'd like to request a couple of changes.

The DepositAuthorized unit tests are failing as a result of your change. You can choose to fix them yourself, or you could cherry-pick this commit which has a fix: scottschurr@3461cd4

The commit messages need some work. Here is generally good advice to follow: https://chris.beams.io/posts/git-commit/ But there are two specific changes I'd like to see

The first line of the first commit message is too long. It should be 60 characters or less. If there's more stuff coming then that first line should end with a colon followed by a blank line followed by the additional stuff. Consider something like this for that first commit message...

RPC ledger_index argument accepts quoted numeric strings:
Some RPC commands return ledger_index as a quoted numeric string.
This change allows the returned value to be directly copied and
used for follow-on RPC commands.
FIXES 3533

Presumably the remaining commits will want to be squashed into the first commit when this pull request is merged. Please start the commit message of any commits you want squashed with [FOLD]. The person assembling a release may not have been following the code review. Including [FOLD] helps the person building the release to know whether they have additional work to do. So the second commit message might look like this...

[FOLD] Update unit-test

The additional bug-fix and tuning commits should all include [FOLD] at the front. So, for example, the commit message for the third commit might look like...

[FOLD] Simplify ledger_index parsing:
Change lexicalCastChecked() argument to std::unit32_t.
Update unit-test.

I'm sure you get the idea. Thanks.

Thank you. I fixed the unit test.

I'll update the commit message and squash per your recommendations once the PR is approved.

scottschurr

👍 LGTM and continuous integration passes!

Some RPC commands return ledger_index as a quoted numeric string. This change allows the returned value to be directly copied and used for follow-on RPC commands. FIXES 3533 [FOLD] Update unit-test [FOLD] Simplify ledger index parsing: Change lexicalCastChecked() argument to std::unit32_t Update unit-test [FOLD] Fix unit-test

gregtatcam assigned scottschurr and cjcobb23 Aug 13, 2020

cjcobb23 approved these changes Aug 13, 2020

View reviewed changes

cjcobb23 suggested changes Aug 13, 2020

View reviewed changes

gregtatcam requested a review from cjcobb23 August 13, 2020 19:25

cjcobb23 approved these changes Aug 13, 2020

View reviewed changes

nbougalis reviewed Aug 15, 2020

View reviewed changes

src/ripple/rpc/impl/RPCHelpers.cpp Outdated Show resolved Hide resolved

src/ripple/rpc/impl/RPCHelpers.cpp Outdated Show resolved Hide resolved

src/test/rpc/LedgerRPC_test.cpp Show resolved Hide resolved

nbougalis approved these changes Aug 18, 2020

View reviewed changes

scottschurr suggested changes Aug 19, 2020

View reviewed changes

scottschurr approved these changes Aug 24, 2020

View reviewed changes

gregtatcam force-pushed the ledger3533 branch from 0294522 to 0fc48fc Compare August 24, 2020 21:17

carlhua added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 31, 2020

nbougalis mentioned this pull request Sep 1, 2020

Proposed 1.7.0-b1 #3599

Merged

nbougalis closed this in #3599 Sep 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Currently, the ledger command only takes int for ledger_index - update the logic to allow string as well. #3582

Currently, the ledger command only takes int for ledger_index - update the logic to allow string as well. #3582

gregtatcam commented Aug 13, 2020 •

edited by intelliot

Loading

cjcobb23 left a comment

nbougalis left a comment

scottschurr left a comment

scottschurr Aug 18, 2020

gregtatcam Aug 24, 2020

scottschurr Aug 18, 2020

gregtatcam commented Aug 24, 2020

scottschurr left a comment

Currently, the ledger command only takes int for ledger_index - update the logic to allow string as well. #3582

Currently, the ledger command only takes int for ledger_index - update the logic to allow string as well. #3582

Conversation

gregtatcam commented Aug 13, 2020 • edited by intelliot Loading

cjcobb23 left a comment

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr Aug 18, 2020

Choose a reason for hiding this comment

gregtatcam Aug 24, 2020

Choose a reason for hiding this comment

scottschurr Aug 18, 2020

Choose a reason for hiding this comment

gregtatcam commented Aug 24, 2020

scottschurr left a comment

Choose a reason for hiding this comment

gregtatcam commented Aug 13, 2020 •

edited by intelliot

Loading