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

swap: use p2ptest for protocol tests #1934

Merged
merged 5 commits into from
Nov 18, 2019
Merged

swap: use p2ptest for protocol tests #1934

merged 5 commits into from
Nov 18, 2019

Conversation

ralph-pichler
Copy link
Member

@ralph-pichler ralph-pichler commented Nov 7, 2019

This PR

  • rewrites TestEmitCheque to use p2ptest to also verify that the correct message is being sent
  • rewrites TestTriggerPaymentThreshold to use p2ptest to also verify that the correct message is being sent
  • updates TestTriggerDisconnectThreshold to test the right thing for pending cheques

@ralph-pichler ralph-pichler added in progress incentives builds on open PR Builds on a PR that is not yet merged. It is blocked until then and the diff won't make sense yet. labels Nov 7, 2019
@ralph-pichler ralph-pichler self-assigned this Nov 7, 2019
@ralph-pichler ralph-pichler force-pushed the swap_proto_tests branch 3 times, most recently from 57ff853 to 49b2d83 Compare November 11, 2019 11:41
@ralph-pichler ralph-pichler added ready for review and removed builds on open PR Builds on a PR that is not yet merged. It is blocked until then and the diff won't make sense yet. labels Nov 12, 2019
swap/protocol_test.go Outdated Show resolved Hide resolved
case <-ctx.Done():
t.Fatal("Expected one cheque, but there is none")
default:
if creditor.getLastSentCheque() != nil {
Copy link
Contributor

@holisticode holisticode Nov 14, 2019

Choose a reason for hiding this comment

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

This actually means that if the remote peer does not confirm the cheque, the node remains "stuck" (lastSentCheque would not be released). So if a new cheque would be due and for some reason the confirmation msg never arrived, can the node process it based on the new cumulative amount or something? Or will it reject it

Copy link
Member Author

Choose a reason for hiding this comment

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

As long as there is no confirmation the node will try to resend the current pending cheque once the threshold is reached again (the recipient will send the confirm message even if it has already processed it in the past) and then afterwards send a new cheque for the amount of debt incurred in the meantime. One of the reasons why we don't want to send a new cheque with a higher amount before that is that if the old cheque never was processed correctly by the recipient, then the recipient will consider the difference in cumulativePayout compared to the last cheque it has seen as the payment for the honey amount specified in the latest cheque (which doesn't match the expected price and will therefore be rejected). If there is no honey-to-money anymore in the new swap pricing, maybe we can also allow trying to resend with a higher payout.

Copy link
Contributor

@holisticode holisticode left a comment

Choose a reason for hiding this comment

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

Builds failed but it seems unrelated.
Let's wait until core is aware of these failures (I already reported) and then restart the builds before merging, better to have had green builds before merging.
Great job.

@ralph-pichler ralph-pichler merged commit 0d02293 into master Nov 18, 2019
@ralph-pichler ralph-pichler deleted the swap_proto_tests branch November 18, 2019 17:28
@acud acud added this to the 0.5.3 milestone Nov 25, 2019
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.

4 participants