Add mediation fees for internal routing and maximum fee limit by LefterisJP · Pull Request #4760 · raiden-network/raiden

LefterisJP · 2019-09-05T14:16:12Z

Fixes: #4751

Description

Check the issue.

PR review check list

Quality check list that cannot be automatically verified.

Safety
- The changes respect the necessary conditions for safety (https://raiden-network-specification.readthedocs.io/en/latest/smart_contracts.html#protocol-values-constraints)
Code quality
- Error conditions are handled
- Exceptions are propagated to the correct parent greenlet
- Exceptions are correctly classified as recoverable or unrecoverable
Compatibility
- State changes are forward compatible
- Transport messages are backwards and forward compatible
Commits
- Have good messages
- Squashed unecessary commits
If it's a bug fix:
- Regression test for the bug
  - Properly covers the bug
  - If an integration test is used, it could not be written as a unit test
Documentation
- A new CLI flag was introduced, is there documentation that explains usage?
Specs
- If this is a protocol change, are the specs updated accordingly? If so, please link PR from the specs repo.
Is it a user facing feature/bug fix?
- Changelog entry has been added

codecov · 2019-09-06T23:01:46Z

Codecov Report

Merging #4760 into develop will decrease coverage by 0.11%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #4760      +/-   ##
===========================================
- Coverage    80.75%   80.63%   -0.12%     
===========================================
  Files          119      119              
  Lines        14373    14384      +11     
  Branches      2217     2219       +2     
===========================================
- Hits         11607    11599       -8     
- Misses        2111     2122      +11     
- Partials       655      663       +8

Impacted Files	Coverage Δ
raiden/settings.py	`100% <100%> (ø)`	⬆️
raiden/routing.py	`78.5% <100%> (+0.4%)`	⬆️
raiden/transfer/mediated_transfer/initiator.py	`98.7% <100%> (+0.06%)`	⬆️
raiden/storage/serialization/types.py	`59.7% <0%> (-8.96%)`	⬇️
raiden/tasks.py	`68.08% <0%> (-2.84%)`	⬇️
raiden/utils/echo_node.py	`67.79% <0%> (-1.7%)`	⬇️
raiden/raiden_event_handler.py	`91.5% <0%> (-0.66%)`	⬇️
raiden/network/transport/matrix/transport.py	`80.44% <0%> (-0.6%)`	⬇️
raiden/network/proxies/token_network.py	`49.75% <0%> (-0.5%)`	⬇️
raiden/transfer/channel.py	`89.14% <0%> (-0.23%)`	⬇️
... and 2 more

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 12b673d...33df00b. Read the comment docs.

LefterisJP · 2019-09-09T13:05:47Z

Rebased on latest develop and solved conflicts

Dominik1999 · 2019-09-09T18:50:32Z

Does the maximum fee limit also count for routes being provided by the pfs?
It should do so.

LefterisJP · 2019-09-09T19:20:34Z

Does the maximum fee limit also count for routes being provided by the pfs?
It should do so.

Yes the code for the check is after the routes have been acquired (by whatever means, pfs or internal routing)

Part of raiden-network#4751

LefterisJP · 2019-09-09T22:13:47Z

Hey @karlb

Rebasing on #4786 broke this PR unfortunately on the test_mediated_transfer_with_fees.

So had to spend some time figuring it out.

Turns out that since you were patching the fees and setting them to a standard 20% for each mediator, quite a lot of combinations of amount + fees would not work and end up not completing the transfer (due to less than the final amount reaching the target). So what I added is:

An exception that is thrown if gevent timeout kills the waiting for the transfer. This helps by giving a nice explanation to what happened. Before the test just failed with some big gevent traces which were impossible to read.
Wrote a check in the test to validate given amount/fee combinations and make sure that with the given 20% mediation fees per mediator of the test, the amount reaches the target (before trying the transfer)

karlb · 2019-09-10T09:39:51Z


+        # https://github.com/raiden-network/raiden/issues/4751
+        # Internal routing doesn't know about fee so it should set a percentage per hop
+        estimated_fee = FeeAmount(round(INTERNAL_ROUTING_DEFAULT_FEE_PERC * amount))


We're adding fees here even in case of a direct transfer.

Sorry for not being thorough, yesterday. It also looks like we only apply INTERNAL_ROUTING_DEFAULT_FEE_PERC once. Wasn't the intention of the ticket to do that once per hop?

I'll write a PR that changes these two things. If any of these statements is wrong, let me know!

@karlb

We're adding fees here even in case of a direct transfer.

It's correct. I did not notice that.

It also looks like we only apply INTERNAL_ROUTING_DEFAULT_FEE_PERC once. Wasn't the intention of the ticket to do that once per hop?

No, since we are not sure of the hops anyway in internal routing the intention (and it's also written in the issue) was to add a big percentage.

palango reviewed Sep 6, 2019

View reviewed changes

Comment thread raiden/tests/unit/transfer/mediated_transfer/test_initiatorstate.py Outdated

LefterisJP force-pushed the workon_4751 branch from 48cb80d to 099725e Compare September 6, 2019 17:22

LefterisJP commented Sep 6, 2019

View reviewed changes

Comment thread raiden/tests/integration/transfer/test_mediatedtransfer.py

LefterisJP assigned karlb Sep 6, 2019

LefterisJP requested a review from karlb September 6, 2019 23:11

LefterisJP unassigned karlb Sep 6, 2019

karlb reviewed Sep 9, 2019

View reviewed changes

Comment thread raiden/transfer/mediated_transfer/initiator.py

Comment thread raiden/tests/utils/transfer.py Outdated

Comment thread raiden/tests/integration/transfer/test_mediatedtransfer.py

karlb reviewed Sep 9, 2019

View reviewed changes

Comment thread raiden/tests/unit/transfer/mediated_transfer/test_initiatorstate.py Outdated

LefterisJP force-pushed the workon_4751 branch from f56e400 to a4bf9b9 Compare September 9, 2019 13:05

karlb approved these changes Sep 9, 2019

View reviewed changes

LefterisJP force-pushed the workon_4751 branch 2 times, most recently from 390130a to 410b6cd Compare September 9, 2019 14:50

LefterisJP added 14 commits September 10, 2019 00:08

Add a fee for internal routing that is a percentage to the amount

d32eb2b

Part of raiden-network#4751

Add a check for the maximum amount limit of transfer + fees

2430e3c

Part of raiden-network#4751

Use UNIT_TRANSFER_AMOUNT in tests instead of magic constants

fc2a01a

Set amount/fees in unit tests that agree with our defaults

446018f

Add a test for the maximum mediation fee limit

67d1022

Adjust test_mediated_transfer for the fee changes

c9c6bb0

Adjust test_restapi for the fee changes

43f4bbf

Adjust test_refund_transfer for fee changes

07b392e

Adjust test_regression.py for the fee changes

bd5899f

Adjust test_pending_transfers_endpoint for fees

eec83d3

Adjust test_settlement after fee changes

cd837e9

Disable max mediation fee checks for test_mediated_transfer_with_fee

8a7cec9

/s/ininitial/initial

1d0a279

Fixes after rebase

1c299c2

LefterisJP added 3 commits September 10, 2019 00:08

PR review fixes for readability

aff2ab7

Add exception with msg at timeout of wait for transfer in tests

5ddf099

Add proper checks for amount/fee ratio in the mediation fee test

33df00b

LefterisJP force-pushed the workon_4751 branch from 410b6cd to 33df00b Compare September 9, 2019 22:08

LefterisJP merged commit 4f5e8fc into raiden-network:develop Sep 10, 2019

LefterisJP deleted the workon_4751 branch September 10, 2019 06:42

karlb reviewed Sep 10, 2019

View reviewed changes

rakanalh mentioned this pull request Sep 10, 2019

Direct transfers should not pay mediation fees #4815

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mediation fees for internal routing and maximum fee limit#4760

Add mediation fees for internal routing and maximum fee limit#4760
LefterisJP merged 17 commits into
raiden-network:developfrom
LefterisJP:workon_4751

LefterisJP commented Sep 5, 2019

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 6, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LefterisJP commented Sep 9, 2019

Uh oh!

Dominik1999 commented Sep 9, 2019

Uh oh!

LefterisJP commented Sep 9, 2019

Uh oh!

LefterisJP commented Sep 9, 2019

Uh oh!

karlb Sep 10, 2019

Uh oh!

karlb Sep 10, 2019

Uh oh!

karlb Sep 10, 2019

Uh oh!

LefterisJP Sep 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

LefterisJP commented Sep 5, 2019

Description

PR review check list

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LefterisJP commented Sep 9, 2019

Uh oh!

Dominik1999 commented Sep 9, 2019

Uh oh!

LefterisJP commented Sep 9, 2019

Uh oh!

LefterisJP commented Sep 9, 2019

Uh oh!

karlb Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

karlb Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

karlb Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

LefterisJP Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Sep 6, 2019 •

edited

Loading