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.

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

Fixed calculation of deposit_amount #249

Merged
merged 6 commits into from
Aug 21, 2019
Merged

Conversation

eorituz
Copy link
Contributor

@eorituz eorituz commented Aug 19, 2019

Since jacobs scenario is the first one to use balance_per_node
We never tested the calculation for depositing more tokens.

Since the UDC contract requires a deposit greater than the total_deposit

function deposit(address beneficiary, uint256 new_total_deposit)
        external
    {
        require(new_total_deposit > total_deposit[beneficiary], "deposit not increasing");

        // Calculate the actual amount of tokens that will be transferred
        uint256 added_deposit = new_total_deposit - total_deposit[beneficiary];

        balances[beneficiary] += added_deposit;
        total_deposit[beneficiary] += added_deposit;

        // Update whole_balance, but take care against overflows.
        require(whole_balance + added_deposit >= whole_balance, "overflowing deposit");
        whole_balance += added_deposit;

        // Decline deposit if the whole balance is bigger than the limit.
        require(whole_balance <= whole_balance_limit, "too much deposit");

        // Actual transfer.
        require(token.transferFrom(msg.sender, address(this), added_deposit), "tokens didn't transfer");

I needed to change the calculation of the amount that needs to be deposited

This is a fix for Issue #238

@eorituz eorituz requested a review from deepbrook August 19, 2019 12:03
@@ -409,6 +414,6 @@ def deposit(self, target_address) -> Union[str, None]:
return

log.debug("deposit call required - insufficient funds")
deposit_amount = max_funding - balance
deposit_amount = total_deposit + max(max_funding, min_deposit) - balance

Choose a reason for hiding this comment

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

max() should not be necessary here - there should be a checksuggestion

in this class instead, that ensures that max_funding >= balance_per_node when loading the scenario yaml.

Otherwise, this would silently use a value that may not be expected by the user.


Suggested validation::

def validate(self):
    self.assert_option(self.max_funding < self.balance_per_node, "max_funding must be >= balance_per_node value!")

I trust you can write a test for that just fine. ;P

@deepbrook
Copy link

deepbrook commented Aug 20, 2019

That would've taken me days to figure out - good catch! Just a small change required, then we're good to merge this.

Copy link

@deepbrook deepbrook left a comment

Choose a reason for hiding this comment

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

Some leftovers, then you're good.

scenario_player/scenario.py Outdated Show resolved Hide resolved
scenario_player/utils/configuration/base.py Outdated Show resolved Hide resolved
scenario_player/utils/configuration/settings.py Outdated Show resolved Hide resolved
tests/unittests/utils/configuration/test_settings.py Outdated Show resolved Hide resolved
Copy link

@deepbrook deepbrook left a comment

Choose a reason for hiding this comment

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

Geht doch! >:D

@deepbrook deepbrook merged commit 56ec926 into raiden-network:dev Aug 21, 2019
@eorituz eorituz deleted the issue_238 branch August 21, 2019 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants