Skip to content
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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 4, 2019
Merged

Iteration in fee approximation #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66679cb
Use `fee_receiver` from raiden
karlb Oct 4, 2019
ad83ecf
Use `fee_sender` from raiden and remove old code
karlb Oct 4, 2019
4490d28
Update raiden version
karlb Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
git+https://github.com/raiden-network/raiden.git@89b00a64878f536a2f2236eea6222378004fa14a
git+https://github.com/raiden-network/raiden.git@90f9b42ce467a4e8a32680212a5398d797129737
raiden-contracts==0.32.0

structlog==19.1.0
Expand Down
53 changes: 3 additions & 50 deletions src/pathfinding_service/model/channel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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).

from raiden.transfer.mediated_transfer.mediation_fee import FeeScheduleState as FeeScheduleRaiden
from raiden.utils.typing import (
Address,
Expand All @@ -33,32 +33,6 @@ def from_raiden(cls, fee_schedule: FeeScheduleRaiden, timestamp: datetime) -> "F
kwargs.pop("_penalty_func")
return FeeSchedule(timestamp=timestamp, **kwargs)

def imbalance_fee_receiver(self, amount: PaymentWithFeeAmount, balance: Balance) -> FeeAmount:
if not self._penalty_func:
return FeeAmount(0)

# Calculate the mediators balance
balance = self._penalty_func.x_list[-1] - balance
try:
return FeeAmount(
# Mediator is gaining balance on his channel side
round(self._penalty_func(balance + amount) - self._penalty_func(balance))
)
except ValueError:
raise UndefinedMediationFee()

def imbalance_fee_sender(self, amount: PaymentWithFeeAmount, balance: Balance) -> FeeAmount:
if not self._penalty_func:
return FeeAmount(0)

try:
return FeeAmount(
# Mediator is loosing balance on his channel side
round(self._penalty_func(balance - amount) - self._penalty_func(balance))
)
except ValueError:
raise UndefinedMediationFee()


@add_schema
@dataclass
Expand Down Expand Up @@ -170,33 +144,12 @@ def update_capacity(

def backwards_fee_sender(self, balance: Balance, amount: PaymentWithFeeAmount) -> FeeAmount:
"""Returns the mediation fee for this channel when transferring the given amount"""
imbalance_fee = self.fee_schedule_sender.imbalance_fee_sender(
amount=amount, balance=balance
)
flat_fee = self.fee_schedule_sender.flat
prop_fee = int(round(amount * self.fee_schedule_sender.proportional / 1e6))
return FeeAmount(flat_fee + prop_fee + imbalance_fee)
return fee_sender(self.fee_schedule_sender, balance, amount)

def backwards_fee_receiver(self, balance: Balance, amount: PaymentWithFeeAmount) -> FeeAmount:
"""Returns the mediation fee for this channel when receiving the given amount"""

def fee_in(imbalance_fee: FeeAmount) -> FeeAmount:
return FeeAmount(
round(
(
(amount + self.fee_schedule_receiver.flat + imbalance_fee)
/ (1 - self.fee_schedule_receiver.proportional / 1e6)
)
- amount
)
)

imbalance_fee = self.fee_schedule_receiver.imbalance_fee_receiver(
amount=PaymentWithFeeAmount(amount - fee_in(imbalance_fee=FeeAmount(0))),
balance=balance,
)

return fee_in(imbalance_fee=imbalance_fee)
return fee_receiver(self.fee_schedule_receiver, balance, amount)

def set_fee_schedule(self, fee_schedule: FeeSchedule) -> None:
if self.fee_schedule_sender.timestamp >= fee_schedule.timestamp:
Expand Down
60 changes: 31 additions & 29 deletions tests/pathfinding/test_fee_schedule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def a(int_addr) -> Address: # pylint: disable=invalid-name


class TokenNetworkForTests(TokenNetwork):
def __init__(self, channels: List[dict], default_capacity: TA = TA(100)):
def __init__(self, channels: List[dict], default_capacity: TA = TA(1000)):
super().__init__(token_network_address=TokenNetworkAddress(a(255)))

# open channels
Expand Down Expand Up @@ -79,7 +79,7 @@ def set_fee(self, node1: int, node2: int, **fee_params):
)
)

def estimate_fee(self, initator: int, target: int, value=PA(10), max_paths=1):
def estimate_fee(self, initator: int, target: int, value=PA(100), max_paths=1):
result = self.get_paths(
source=a(initator),
target=a(target),
Expand Down Expand Up @@ -122,51 +122,52 @@ def test_fees_in_balanced_routing():
tn.set_fee(2, 3)

# Let's try imbalance fees
# When approximation iterations matter, those are given as sums of the steps.

# Incoming channel
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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

assert tn.estimate_fee(3, 1) == -10

# The opposite fee schedule should give opposite results
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(200)), (TA(200), FA(0))])
assert tn.estimate_fee(1, 3) == -10
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(200)), (TA(2000), FA(0))])
assert tn.estimate_fee(1, 3) == -10 + 1
assert tn.estimate_fee(3, 1) == 10

# Outgoing channel
tn.set_fee(2, 1)
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(0)), (TA(200), FA(200))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(0)), (TA(2000), FA(200))])
assert tn.estimate_fee(1, 3) == -10
assert tn.estimate_fee(3, 1) == 10
assert tn.estimate_fee(3, 1) == 10 + 1

# The opposite fee schedule should give opposite results
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(200)), (TA(200), FA(0))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(200)), (TA(2000), FA(0))])
assert tn.estimate_fee(1, 3) == 10
assert tn.estimate_fee(3, 1) == -10
assert tn.estimate_fee(3, 1) == -10 + 1

# Combined fees cancel out
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(0)), (TA(200), FA(20))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(0)), (TA(200), FA(20))])
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(0)), (TA(2000), FA(20))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(0)), (TA(2000), FA(20))])
assert tn.estimate_fee(1, 3) == 0
assert tn.estimate_fee(3, 1) == 0

tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(20)), (TA(200), FA(0))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(20)), (TA(200), FA(0))])
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(20)), (TA(2000), FA(0))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(20)), (TA(2000), FA(0))])
assert tn.estimate_fee(1, 3) == 0
assert tn.estimate_fee(3, 1) == 0

# When the range covered by the imbalance_penalty does include the
# necessary balance values, the route should be considered invalid.
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(0)), (TA(80), FA(200))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(0)), (TA(800), FA(200))])
assert tn.estimate_fee(1, 3) is None


def test_fees_in_unbalanced_routing():
""" Tests fee estimation in a network where only one participant has funds in a channel. """
tn = TokenNetworkForTests(
channels=[
dict(participant1=1, participant2=2, capacity1=100, capacity2=0),
dict(participant1=2, participant2=3, capacity1=100, capacity2=0),
dict(participant1=1, participant2=2, capacity1=1000, capacity2=0),
dict(participant1=2, participant2=3, capacity1=1000, capacity2=0),
]
)

Expand All @@ -193,37 +194,38 @@ def test_fees_in_unbalanced_routing():
tn.set_fee(2, 1)
tn.set_fee(2, 3)

# Let's try imbalance fees
# Let's try imbalance fees!
# When approximation iterations matter, those are given as sums of the steps.

# Incoming channel
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(0)), (TA(100), FA(100))])
assert tn.estimate_fee(1, 3) == 10
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(0)), (TA(1000), FA(100))])
assert tn.estimate_fee(1, 3) == 10 + 1
assert tn.estimate_fee(3, 1) is None # no balance in channel

# The opposite fee schedule should give opposite results
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(100)), (TA(100), FA(0))])
assert tn.estimate_fee(1, 3) == -10
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(100)), (TA(1000), FA(0))])
assert tn.estimate_fee(1, 3) == -10 + 1
assert tn.estimate_fee(3, 1) is None # no balance in channel

# Outgoing channel
tn.set_fee(2, 1)
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(0)), (TA(100), FA(100))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(0)), (TA(1000), FA(100))])
assert tn.estimate_fee(1, 3) == -10
assert tn.estimate_fee(3, 1) is None # no balance in channel

# The opposite fee schedule should give opposite results
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(100)), (TA(100), FA(0))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(100)), (TA(1000), FA(0))])
assert tn.estimate_fee(1, 3) == 10
assert tn.estimate_fee(3, 1) is None # no balance in channel

# Combined fees cancel out
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(0)), (TA(100), FA(20))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(0)), (TA(100), FA(20))])
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(0)), (TA(1000), FA(20))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(0)), (TA(1000), FA(20))])
assert tn.estimate_fee(1, 3) == 0
assert tn.estimate_fee(3, 1) is None # no balance in channel

tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(20)), (TA(100), FA(0))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(20)), (TA(100), FA(0))])
tn.set_fee(2, 1, imbalance_penalty=[(TA(0), FA(20)), (TA(1000), FA(0))])
tn.set_fee(2, 3, imbalance_penalty=[(TA(0), FA(20)), (TA(1000), FA(0))])
assert tn.estimate_fee(1, 3) == 0
assert tn.estimate_fee(3, 1) is None # no balance in channel

Expand Down Expand Up @@ -315,7 +317,7 @@ def test_compounding_fees(flat_fee_cli, prop_fee_cli, estimated_fee):
(100, 500_000, 0, 967, 733),
# imbalance fee
(0, 0, 100, 1_000, 10),
(0, 0, 1_000, 1_000, 100),
(0, 0, 1_000, 1_000, 110),
],
)
def test_fee_estimate(flat_fee, prop_fee_cli, max_lin_imbalance_fee, target_amount, expected_fee):
Expand Down