Idempotency of MS deposits - closes #76 #80

err508 · 2018-05-18T14:04:15Z

No description provided.

czepluch · 2018-05-18T14:19:59Z

raiden_contracts/contracts/MonitoringService.sol

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


This is not good. uint256 can only be positive integers, and if the new total_deposit is lower than the current balance you have a problem. You must add a check that doesn't allow for this to happen. Ideally you also add a small test to show that it works correctly.

loredanacirstea · 2018-05-18T14:23:58Z

.gitignore

@@ -12,6 +12,9 @@ venv/
 # ignore private keys
 *.pem

+# virtualenv
+.venv


We already have env/ for this purpose.

loredanacirstea · 2018-05-22T15:55:05Z

raiden_contracts/contracts/MonitoringService.sol

-        require(amount > 0);
+    /// @param beneficiary The account benefiting from the deposit
+    /// @param total_deposit Idempotent function that sets the total_deposit of
+    //// tokens of the beneficiary


//// -> ///

loredanacirstea · 2018-05-22T15:57:12Z

raiden_contracts/contracts/MonitoringService.sol

+    //// tokens of the beneficiary
+    function deposit(address beneficiary, uint256 total_deposit) public
+    {
+        require(total_deposit != 0);


Consider using total_deposit > 0 . One might say it's redundant, because we are talking about a uint, but more complete and we keep consistency with the rest of the contracts code.

I changed it, as I found that the > 0 looks like a wrong int-underflow safeguard, as it actually does not check what it states, i.e. the require will not throw when .deposit(Uint(-1)) is called. Does that make sense?

When .deposit(-1) is called, there should be a type error thrown before it even gets to the require. Just because we are using uint as the input type. The check's scope is actually >0 and not only !=0.

Ah, ok. I understand what you mean. But I see it as: the scope is to make sure total_deposit > 0. Because Solidity is still a young language that can still behave in unwanted ways, I would not completely trust it especially if it doesn't really cost us much. Plus, this > 0 pattern was agreed upon in a previous smart contracts review session for uRaiden. I would rather keep it like this for now. You can open an issue to discuss adv/disadv if you want.

Changed it back to the way it was. I was referring to UINT(-1) above, but doesn't make a difference I think.

czepluch · 2018-05-22T15:53:27Z

raiden_contracts/contracts/MonitoringService.sol

-    function deposit(address beneficiary, uint amount) public {
-        require(amount > 0);
+    /// @param beneficiary The account benefiting from the deposit
+    /// @param total_deposit Idempotent function that sets the total_deposit of


This should explain what the parameter is, not what the function does. The @notice is meant for explaining what the function does.

czepluch · 2018-05-22T16:00:33Z

raiden_contracts/contracts/MonitoringService.sol


-        emit NewDeposit(beneficiary, amount);
+        require(balances[beneficiary] >= added_deposit);


Couldn't we cover both require(total_deposit != 0); and require(balances[beneficiary] >= added_deposit); by doing total_deposit > balances[beneficary] instead?

I used the pattern that was also used here in TokenNetwork.setDeposit because I was also trying to keep the checks grouped, as agreed. In this case, require(total_deposit > balances[beneficary]) can work.

require(total_deposit > balances[beneficary]) Doesn't conflict with keeping the checks grouped, since there's only one check, but I get what you mean. I am fine with either. Just thought that Tobias might find it interesting to think about both options.

I think require(total_deposit > balances[beneficiary]) solves under/overflow the shortest way, not sure though which conventions Loredana refers too.

@err508 #47. In TokenNetwork.setDeposit it wasn't so straight forward to get the participant deposit and do the check at the beginning of the function, so I opted for a check at the end. As said, for this case here, require(total_deposit > balances[beneficiary]) does the job.

As Loredana says, require(total_deposit > balances[beneficiary]) does the trick and we get the same checks guarantees from one line rather than two. So please use that instead.

loredanacirstea

From my side, it looks good.

czepluch · 2018-05-25T11:36:57Z

raiden_contracts/contracts/MonitoringService.sol


-        emit NewDeposit(beneficiary, amount);
+        require(balances[beneficiary] >= added_deposit);


As Loredana says, require(total_deposit > balances[beneficiary]) does the trick and we get the same checks guarantees from one line rather than two. So please use that instead.

Changes are adressed!

err508 requested a review from czepluch May 18, 2018 14:04

czepluch suggested changes May 18, 2018

View reviewed changes

loredanacirstea reviewed May 18, 2018

View reviewed changes

err508 force-pushed the issue_#76 branch from 6b3e74b to 39c44f3 Compare May 22, 2018 13:22

loredanacirstea reviewed May 22, 2018

View reviewed changes

err508 force-pushed the issue_#76 branch from 39c44f3 to 86545a2 Compare May 22, 2018 15:57

czepluch reviewed May 22, 2018

View reviewed changes

err508 force-pushed the issue_#76 branch from 86545a2 to d265f3b Compare May 22, 2018 16:25

loredanacirstea approved these changes May 22, 2018

View reviewed changes

czepluch previously requested changes May 25, 2018

View reviewed changes

err508 force-pushed the issue_#76 branch 3 times, most recently from b8f3225 to 0422674 Compare May 25, 2018 14:26

Idempotency of MS deposits - closes raiden-network#76

51a3f03

err508 force-pushed the issue_#76 branch from 0422674 to 51a3f03 Compare May 25, 2018 14:55

err508 merged commit 6682275 into raiden-network:master May 25, 2018

err508 deleted the issue_#76 branch May 25, 2018 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Idempotency of MS deposits - closes #76 #80

Idempotency of MS deposits - closes #76 #80

err508 commented May 18, 2018

czepluch May 18, 2018

err508 May 22, 2018

loredanacirstea May 18, 2018

loredanacirstea May 22, 2018

loredanacirstea May 22, 2018

err508 May 22, 2018 •

edited

Loading

loredanacirstea May 22, 2018 •

edited

Loading

loredanacirstea May 22, 2018 •

edited

Loading

err508 May 22, 2018

czepluch May 22, 2018

czepluch May 22, 2018

loredanacirstea May 22, 2018

czepluch May 22, 2018 •

edited

Loading

err508 May 22, 2018 •

edited

Loading

loredanacirstea May 22, 2018

czepluch May 25, 2018

loredanacirstea left a comment

czepluch May 25, 2018


		emit NewDeposit(beneficiary, amount);
		require(balances[beneficiary] >= added_deposit);

Idempotency of MS deposits - closes #76 #80

Idempotency of MS deposits - closes #76 #80

Conversation

err508 commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

err508 May 22, 2018 • edited Loading

Choose a reason for hiding this comment

loredanacirstea May 22, 2018 • edited Loading

Choose a reason for hiding this comment

loredanacirstea May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czepluch May 22, 2018 • edited Loading

Choose a reason for hiding this comment

err508 May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loredanacirstea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

err508 May 22, 2018 •

edited

Loading

loredanacirstea May 22, 2018 •

edited

Loading

loredanacirstea May 22, 2018 •

edited

Loading

czepluch May 22, 2018 •

edited

Loading

err508 May 22, 2018 •

edited

Loading