-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Iteration in fee approximation #565
Conversation
The PR needs the corresponding raiden PR merged first. I'll update it as soon as that PR is ready. |
@@ -8,7 +8,7 @@ | |||
|
|||
from pathfinding_service.constants import DEFAULT_REVEAL_TIMEOUT | |||
from pathfinding_service.exceptions import InvalidPFSFeeUpdate | |||
from raiden.exceptions import UndefinedMediationFee | |||
from raiden.tests.utils.mediation_fees import fee_receiver, fee_sender |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok what I understand is that you take the fee calculation from the Raiden repo to do it in the pfs, now. Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why do you take this crucial function from the Raiden tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy if that works, but we might need to refactor that anytime after the RC, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to do that (see commit message).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok that looks good to me
@@ -8,7 +8,7 @@ | |||
|
|||
from pathfinding_service.constants import DEFAULT_REVEAL_TIMEOUT | |||
from pathfinding_service.exceptions import InvalidPFSFeeUpdate | |||
from raiden.exceptions import UndefinedMediationFee | |||
from raiden.tests.utils.mediation_fees import fee_receiver, fee_sender |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why do you take this crucial function from the Raiden tests?
@@ -8,7 +8,7 @@ | |||
|
|||
from pathfinding_service.constants import DEFAULT_REVEAL_TIMEOUT | |||
from pathfinding_service.exceptions import InvalidPFSFeeUpdate | |||
from raiden.exceptions import UndefinedMediationFee | |||
from raiden.tests.utils.mediation_fees import fee_receiver, fee_sender |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy if that works, but we might need to refactor that anytime after the RC, right?
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(0)), (TA(200), FA(200))]) | ||
assert tn.estimate_fee(1, 3) == 10 | ||
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(0)), (TA(2000), FA(200))]) | ||
assert tn.estimate_fee(1, 3) == 10 + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment that + 1
is due to the iteration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote When approximation iterations matter, those are given as sums of the steps.
in line 125. But apparently this is easily missed.
Codecov Report
@@ Coverage Diff @@
## master #565 +/- ##
========================================
Coverage ? 90.7%
========================================
Files ? 36
Lines ? 2346
Branches ? 297
========================================
Hits ? 2128
Misses ? 163
Partials ? 55
Continue to review full report at Codecov.
|
Use
fee_receiver
andfee_sender
from raiden to get the latest fee calculation. Those are currently in a test utils package and should be moved in a following PR.Closes #563.