Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

swap: query cheque information through RPC calls #1842

Merged
merged 27 commits into from
Oct 2, 2019
Merged

Conversation

mortelli
Copy link
Contributor

@mortelli mortelli commented Sep 30, 2019

This PR implements functions + RPC calls for querying:

  • the last cheque sent to a given peer
  • the last cheques sent to all known swap peers (including nil as a valid answer)
  • the last cheque received from given peer
  • the last cheques received from all known swap peers (including nil as a valid answer)

and also removes the unneeded api field from the Swap struct.

@mortelli mortelli changed the title Swap query cheques swap: query cheque information through RPC calls Sep 30, 2019
@santicomp2014 santicomp2014 added this to the 0.5.1 milestone Sep 30, 2019
@mortelli mortelli marked this pull request as ready for review October 1, 2019 17:21
Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

exemplary PR.

Copy link
Member

@ralph-pichler ralph-pichler left a comment

Choose a reason for hiding this comment

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

The TestReceivedCheque and TestSentCheque tests seem overly complex considering what they are supposed to verify. We already have tests that check that if a cheque is sent the correct fields are updated (e.g. the test this was copied from). Instead of actually sending cheques you could just use the setter function on peer or the save functions on swap similar to how you are already doing it in the TestAllCheques function.

Also I don't think you're testing the code path for non-added peers. For that you should call the swap.save* functions directly (see TestPeerBalance for reference).

Copy link
Contributor

@Eknir Eknir left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@mortelli
Copy link
Contributor Author

mortelli commented Oct 2, 2019

The TestReceivedCheque and TestSentCheque tests seem overly complex considering what they are supposed to verify. We already have tests that check that if a cheque is sent the correct fields are updated (e.g. the test this was copied from). Instead of actually sending cheques you could just use the setter function on peer or the save functions on swap similar to how you are already doing it in the TestAllCheques function.

Also I don't think you're testing the code path for non-added peers. For that you should call the swap.save* functions directly (see TestPeerBalance for reference).

agree completely.

@santicomp2014 and i have addressed these changes, and re-written both of these tests.

thank you for pointing this out, as these new tests allowed us to squash a bug when retrieving cheques from the store for the RPC call.

Copy link
Member

@ralph-pichler ralph-pichler left a comment

Choose a reason for hiding this comment

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

The TestAllCheques test is still missing cheques from disconnected peers. Here it's even more important than in the other tests because the logic for them is a lot more complex.

@mortelli
Copy link
Contributor Author

mortelli commented Oct 2, 2019

thanks @ralph-pichler, addressed.

Copy link
Member

@ralph-pichler ralph-pichler left a comment

Choose a reason for hiding this comment

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

LGTM

@mortelli mortelli merged commit f8e2fe3 into master Oct 2, 2019
@mortelli mortelli deleted the swap-query-cheques branch November 14, 2019 18:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants