Minor fixes to payment channels commands (RIPD-1466 and RIPD-1467) #2192

seelabs · 2017-07-28T18:43:21Z

No description provided.

scottschurr

All I saw was one line with questionable indentation... 👍

scottschurr · 2017-07-28T23:57:58Z

src/ripple/rpc/handlers/PayChanClaim.cpp

@@ -90,15 +90,31 @@ Json::Value doChannelVerify (RPC::Context& context)
 {
    auto const& params (context.params);
    for (auto const& p :
-        {jss::public_key, jss::channel_id, jss::amount, jss::signature})
+                 {jss::public_key, jss::channel_id, jss::amount, jss::signature})


Micro nit: is the indentation here intentional? Looks like it overshot...

seelabs · 2017-08-01T13:33:28Z

Rebased onto 0.80.0-b4

codecov-io · 2017-08-01T14:07:17Z

Codecov Report

Merging #2192 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2192   +/-   ##
========================================
  Coverage    71.01%   71.01%           
========================================
  Files          691      691           
  Lines        52320    52320           
========================================
  Hits         37153    37153           
  Misses       15167    15167

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc9e9f4...cdcf1e6. Read the comment docs.

miguelportilla · 2017-08-11T14:25:27Z

src/ripple/rpc/handlers/PayChanClaim.cpp

+            pk.emplace(makeSlice(pkHex.first));
+        }
+
+        if (!pk)


This if can be moved to after the emplace.

It's not needed. Fixed.

miguelportilla · 2017-08-11T21:34:50Z

src/ripple/net/impl/RPCCall.cpp

+
+            std::pair<Blob, bool> pkHex(strUnHex (strPk));
+            if (!pkHex.second)
+                return false;


Suggestion: This code path doesn't seem to be covered by unit test. We can improve the coverage and test.

miguelportilla · 2017-08-11T21:35:12Z

src/ripple/net/impl/RPCCall.cpp

+                return false;
+
+            if (!publicKeyType(makeSlice(pkHex.first)))
+                return false;


miguelportilla · 2017-08-11T22:01:16Z

src/ripple/net/impl/RPCCall.cpp

+            if (!publicKeyType(makeSlice(pkHex.first)))
+                return false;
+
+            return true;


miguelportilla · 2017-08-11T22:02:17Z

src/ripple/rpc/handlers/PayChanClaim.cpp

+        {
+            std::pair<Blob, bool> pkHex(strUnHex (strPk));
+            if (!pkHex.second)
+                return rpcError(rpcPUBLIC_MALFORMED);


Suggestion: This code path doesn't seem to be covered by unit test. We can improve the coverage and test.

miguelportilla · 2017-08-11T22:02:25Z

src/ripple/rpc/handlers/PayChanClaim.cpp

+                return rpcError(rpcPUBLIC_MALFORMED);
+            auto const pkType = publicKeyType(makeSlice(pkHex.first));
+            if (!pkType)
+                return rpcError(rpcPUBLIC_MALFORMED);


seelabs · 2017-12-08T20:47:19Z

Added a commit to fix an issue with std::stoul and add tests. Also rebased to 0.90.0.

@miguelportilla The places you tagged in an earlier appear to be tested even before this commit. I'm guessing github put the comment on the wrong line of code (I've seen it do this in other commits). I'm assuming you meant to tag the untested code that the new commit now tests.

ripplelabs-jenkins · 2017-12-08T21:10:22Z

Jenkins Build Summary

Built from this commit

Built at 20171218 - 22:04:44

Test Results

Build Type	Result	Status
clang.debug.unity	970 cases, 0 failed, t: 387s	PASS ✅
coverage	970 cases, 0 failed, t: 602s	PASS ✅
clang.debug.nounity	968 cases, 0 failed, t: 294s	PASS ✅
gcc.debug.nounity	968 cases, 0 failed, t: 310s	PASS ✅
gcc.debug.unity	970 cases, 0 failed, t: 426s	PASS ✅
clang.release.unity	969 cases, 0 failed, t: 462s	PASS ✅
gcc.release.unity	969 cases, 0 failed, t: 495s	PASS ✅

scottschurr

👍 Nice change. Good tests.

You might consider testing with a few other malformed integers besides "x1000", "1000x" and "x". For example, I believe leading spaces are stripped by std::stoul, so " 1000" would not be malformed. But "1,000" should be malformed as should "1000 ". I would expect whitespace and separators to be much more common error cases than miscellaneous xs. Just a thought.

seelabs · 2017-12-12T18:21:45Z

@scottschurr Good idea. Pushed tests with leading/trailing/middle spaces and comma

scottschurr

👍 New tests look great, thanks.

So, just to make sure we're on good footing, do we want leading spaces to be okay in this parse? Do the other quoted number parsers allow leading spaces? It's probably not that big a deal if they are not consistent... But it feels like it would be nice if it were consistent. Just a thought.

scottschurr

Changes look good. I just left one thought that may help future maintenance. 👍

scottschurr · 2017-12-18T18:22:42Z

src/ripple/rpc/handlers/PayChanClaim.cpp

 namespace ripple {

+static
+boost::optional<std::uint64_t>


Since there are now two identical implementations of this function, how would you feel about putting it in basics/StringUtilities? That way if there are future bugs to fix they will only need to be fixed in one place.

I considered that, but I decided they shouldn't be project-wide utilities. I agree code duplication like this is bad, but too many project-wide utility functions has a cost too. It makes finding the right utility function hard to find because the useful ones are mixed in with ones like this that are only used in two files.

If the two places were in the same subsystem, I'd look for a place in that subsystem to put them. As is, they are in ripple/net and ripple/rpc, so the util would have to live in ripple.

I'm OK promoting this project-wide if you feel strongly, but I still vote for the two (small) duplicated functions.

I don't feel strongly about it. I have a lot of sympathy for the costs associated with too many project-wide utility functions. I regularly thrash about trying to find where some utility function is hidden. I also probably accidentally create duplicate code, since I have no idea that a function already exists that does what I want to do. I'm not sure precisely what to do about either of those problems.

At any rate, my personal take is that this is not an overly specialized function, so I'd be fine with promoting it as a general facility (since it took you a couple of tries to get it to its current state of correctness). But I'm also okay with duplicating it.

miguelportilla

👍

seelabs · 2017-12-18T21:38:32Z

Squashed and rebased onto 0.90.0-b2

seelabs · 2018-01-16T17:29:36Z

In 0.90.0-b3

seelabs assigned miguelportilla and scottschurr Jul 28, 2017

seelabs requested review from miguelportilla and scottschurr July 28, 2017 18:43

scottschurr approved these changes Jul 29, 2017

View reviewed changes

seelabs force-pushed the ac-validated branch from f4ed894 to f19185e Compare August 1, 2017 13:33

miguelportilla reviewed Aug 11, 2017

View reviewed changes

src/ripple/net/impl/RPCCall.cpp

if (!publicKeyType(makeSlice(pkHex.first)))

return false;

return true;

Copy link

Contributor

miguelportilla Aug 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

miguelportilla reviewed Aug 11, 2017

View reviewed changes

seelabs force-pushed the ac-validated branch from 9959b35 to 825ab8c Compare December 8, 2017 20:44

scottschurr approved these changes Dec 11, 2017

View reviewed changes

scottschurr approved these changes Dec 12, 2017

View reviewed changes

scottschurr approved these changes Dec 18, 2017

View reviewed changes

miguelportilla approved these changes Dec 18, 2017

View reviewed changes

seelabs added 2 commits December 18, 2017 16:35

Add validated field to account_channels command (RIPD-1466)

df8ac42

Allow channel_verify to specify public key in hex (RIPD-1467)

cdcf1e6

seelabs force-pushed the ac-validated branch from 04d014d to cdcf1e6 Compare December 18, 2017 21:37

seelabs added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Dec 18, 2017

nbougalis mentioned this pull request Jan 10, 2018

Proposed 0.90.0-b3 #2328

Merged

seelabs closed this Jan 16, 2018

mDuo13 mentioned this pull request Mar 14, 2018

Update channel API methods with 0.90.0 fixes XRPLF/xrpl-dev-portal#322

Merged

seelabs deleted the ac-validated branch July 11, 2018 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor fixes to payment channels commands (RIPD-1466 and RIPD-1467) #2192

Minor fixes to payment channels commands (RIPD-1466 and RIPD-1467) #2192

seelabs commented Jul 28, 2017

scottschurr left a comment

scottschurr Jul 28, 2017

seelabs Jul 31, 2017

seelabs commented Aug 1, 2017

codecov-io commented Aug 1, 2017 •

edited

Loading

miguelportilla Aug 11, 2017 •

edited

Loading

seelabs Aug 11, 2017

miguelportilla Aug 11, 2017 •

edited

Loading

miguelportilla Aug 11, 2017 •

edited

Loading

miguelportilla Aug 11, 2017

miguelportilla Aug 11, 2017 •

edited

Loading

miguelportilla Aug 11, 2017 •

edited

Loading

seelabs commented Dec 8, 2017

ripplelabs-jenkins commented Dec 8, 2017 •

edited

Loading

scottschurr left a comment

seelabs commented Dec 12, 2017

scottschurr left a comment

scottschurr left a comment

scottschurr Dec 18, 2017

seelabs Dec 18, 2017

scottschurr Dec 18, 2017

miguelportilla left a comment

seelabs commented Dec 18, 2017

seelabs commented Jan 16, 2018

Minor fixes to payment channels commands (RIPD-1466 and RIPD-1467) #2192

Minor fixes to payment channels commands (RIPD-1466 and RIPD-1467) #2192

Conversation

seelabs commented Jul 28, 2017

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr Jul 28, 2017

Choose a reason for hiding this comment

seelabs Jul 31, 2017

Choose a reason for hiding this comment

seelabs commented Aug 1, 2017

codecov-io commented Aug 1, 2017 • edited Loading

Codecov Report

miguelportilla Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

seelabs Aug 11, 2017

Choose a reason for hiding this comment

miguelportilla Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

miguelportilla Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

miguelportilla Aug 11, 2017

Choose a reason for hiding this comment

miguelportilla Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

miguelportilla Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

seelabs commented Dec 8, 2017

ripplelabs-jenkins commented Dec 8, 2017 • edited Loading

Jenkins Build Summary

Test Results

scottschurr left a comment

Choose a reason for hiding this comment

seelabs commented Dec 12, 2017

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr Dec 18, 2017

Choose a reason for hiding this comment

seelabs Dec 18, 2017

Choose a reason for hiding this comment

scottschurr Dec 18, 2017

Choose a reason for hiding this comment

miguelportilla left a comment

Choose a reason for hiding this comment

seelabs commented Dec 18, 2017

seelabs commented Jan 16, 2018

codecov-io commented Aug 1, 2017 •

edited

Loading

miguelportilla Aug 11, 2017 •

edited

Loading

miguelportilla Aug 11, 2017 •

edited

Loading

miguelportilla Aug 11, 2017 •

edited

Loading

miguelportilla Aug 11, 2017 •

edited

Loading

miguelportilla Aug 11, 2017 •

edited

Loading

ripplelabs-jenkins commented Dec 8, 2017 •

edited

Loading