ServiceRegistry (without parameter reconfiguration) #1116

pirapira · 2019-07-08T15:06:54Z

This is a part of #1087 .

Getting and Extending the Membership

There is always a deposit amount for six-month of membership.

a new member can join the pool for making the deposit.
an existing member can extend their membership for six months, for the same amount of deposit

The Change of Deposit Amounts

whenever somebody joins or extends the membership, the price increases by 20%
while nobody joins, the price decreases 0.5% per day.

Terms of the Deposits

The deposit is held until the six months period.

A new member's deposit is held for six months.
When an existing member extends the membership,
- the new deposit is held until the end of the extended period
- the old deposits' conditions stay unchanged

raiden_contracts/data/source/services/ServiceRegistry.sol

raiden_contracts/tests/test_deploy_script.py

codecov · 2019-07-08T17:50:27Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6ee58d4). Click here to learn what that means.
The diff coverage is 66.66%.

@@            Coverage Diff            @@
##             master    #1116   +/-   ##
=========================================
  Coverage          ?   80.96%           
=========================================
  Files             ?       21           
  Lines             ?     1408           
  Branches          ?      178           
=========================================
  Hits              ?     1140           
  Misses            ?      228           
  Partials          ?       40

Impacted Files	Coverage Δ
raiden_contracts/deploy/contract_verifier.py	`87.32% <0%> (ø)`
raiden_contracts/deploy/contract_deployer.py	`91.4% <100%> (ø)`
raiden_contracts/deploy/__main__.py	`93.58% <100%> (ø)`

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 6ee58d4...1fc9dee. Read the comment docs.

codecov · 2019-07-08T17:50:28Z

Codecov Report

Merging #1116 into master will increase coverage by 0.13%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #1116      +/-   ##
==========================================
+ Coverage      81%   81.14%   +0.13%     
==========================================
  Files          21       21              
  Lines        1411     1432      +21     
  Branches      179      186       +7     
==========================================
+ Hits         1143     1162      +19     
- Misses        228      229       +1     
- Partials       40       41       +1

Impacted Files	Coverage Δ
raiden_contracts/deploy/contract_verifier.py	`87.58% <100%> (+0.17%)`	⬆️
raiden_contracts/utils/versions.py	`100% <100%> (ø)`	⬆️
raiden_contracts/deploy/__main__.py	`93.58% <100%> (+0.04%)`	⬆️
raiden_contracts/constants.py	`100% <100%> (ø)`	⬆️
raiden_contracts/deploy/contract_deployer.py	`90% <50%> (-1.41%)`	⬇️
raiden_contracts/contract_source_manager.py	`89.15% <0%> (+0.84%)`	⬆️

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 463b5e5...a7051ee. Read the comment docs.

raiden_contracts/data/source/services/ServiceRegistry.sol

karlb · 2019-07-16T08:42:15Z

raiden_contracts/data/source/services/ServiceRegistry.sol

+        require(now >= release_at);
+        require(balance > 0);
+        require(token.transfer(_to, balance));
+        return true;


Why does this function have a return value? It always returns true if it does not fail on a require.

I think currently it's safer to always return a boolean (even when it's always true), and always check the return value.

Solidity compiler doesn't warn when we forget to check the return value of a function call, so sticking with "always check return values" is a safe convention to follow.

I'll document this in the contribution guide.

I've written this in CONTRIBUTING.md.

raiden_contracts/data/source/services/ServiceRegistry.sol

raiden_contracts/tests/test_service_registry.py

palango · 2019-07-16T09:52:09Z

raiden_contracts/data/source/services/ServiceRegistry.sol

@@ -3,49 +3,167 @@ pragma solidity 0.5.4;
 import "raiden/Token.sol";
 import "raiden/Utils.sol";

+contract Deposit {


Why is this it's own contract? Couldn't you also model this as a struct inside the ServiceRegistry contract?

Yes, everything could be in ServiceRegistry and that would be gas-efficient.

The current choice follows security-in-depth. Even if ServiceRegistry is compromized, the deposits made earlier would not be affected.

Is this conceptual security worth more than the additional gas? I don't know for sure, but the deposits are rarer and bigger than usual Raiden transfers.

That argument makes sense to me, but I don't have much experience. The question was aimed at understanding the trade-off better!

palango · 2019-07-16T09:54:18Z

raiden_contracts/data/source/services/ServiceRegistry.sol

+        withdrawer = _withdrawer;
+    }
+
+    // In order to make a deposit, transfer the ERC20 token into this contract.


Is it possible to forbid this behaviour and only allow deposits with a deposit function, where one could check the token?

Unfortunately, contracts cannot refuse incoming ERC20 tokens.

In the beginning, I had a deposit() function, but it did nothing useful. So I removed it.

raiden_contracts/data/source/services/ServiceRegistry.sol

palango · 2019-07-16T10:00:04Z

raiden_contracts/data/source/services/ServiceRegistry.sol


    // @param _token_for_registration The address of the ERC20 token contract that services use for registration fees
-    constructor(address _token_for_registration) public {
+    constructor(address _token_for_registration, uint256 _initial_price) public {


Should min_price be settable as well?

This follows in the next PR. min_price, 6/5 bumping ratio and 200 days will be configurable.

raiden_contracts/data/source/services/ServiceRegistry.sol

palango · 2019-07-16T10:06:04Z

raiden_contracts/data/source/services/ServiceRegistry.sol

+        // Check against overflow.
+        require(valid_till < valid_till + 180 days);
+        valid_till = valid_till + 180 days;
+        assert(valid_till > service_valid_till[msg.sender]);


I'm not sure about low-level differences, but why do you use assert here?

Because I know the assert is always true. So I use assert and then the SMT solver in solc tries to find a counterexample.

To say it in a different way: assert should only fail for bugs in the contract, require is for all other cases.

palango · 2019-07-16T10:07:45Z

raiden_contracts/data/source/services/ServiceRegistry.sol

+        require(token.transfer(address(depo), amount));
+
+        // Fire event
+        emit RegisteredService(msg.sender, valid_till, amount, depo);


Above you write that all functions should return true, so this one as well?

I believe so. I'll add return true.

palango · 2019-07-16T10:11:24Z

raiden_contracts/data/source/services/ServiceRegistry.sol

        require(bytes(new_url).length != 0, "new url is empty string");
-        if (bytes(urls[msg.sender]).length == 0) {
-            service_addresses.push(msg.sender);
-        }
        urls[msg.sender] = new_url;
    }


Same here, should this return true?

That's also true.

and the new value is small enough. ``` >>> A = 200 >>> X = 2 ** 60 - 1 >>> P = 24 * (A ** 4) >>> Q = P + 24*(A**3)*X + 12*(A**2)*(X**2) + 4*A*(X**3) + X**4 >>> Q < (2 ** 256 - 1) True ```

palango · 2019-07-16T10:14:18Z

raiden_contracts/data/source/services/ServiceRegistry.sol

+        // We are here trying to approximate some exponential decay.
+        // exp(- X / A) where
+        //   X is the number of seconds since the last price change
+        //   A is the decay constant (A = 200 days correspnods to 0.5% decrease per day)


s/correspnods/corresponds

palango · 2019-07-16T10:18:55Z

raiden_contracts/data/source/services/ServiceRegistry.sol

+        uint256 P = 24 * (A ** 4);
+        uint256 Q = P + 24*(A**3)*X + 12*(A**2)*(X**2) + 4*A*(X**3) + X**4;
+
+        uint256 price = _set_price * P / Q;


Once you make A configurable, you might have to check for an overflow here as well.

Yes, the next PR will contain lots of those checks. I hope nobody needs the price decaying over billions of years.

raiden_contracts/deploy/__main__.py

raiden_contracts/deploy/contract_verifier.py

palango · 2019-07-16T10:35:21Z

raiden_contracts/tests/test_service_registry.py

+def test_deposit_contract(
+    get_deposit_contract: Callable, custom_token: Contract, get_accounts: Callable
+) -> None:
+    (A,) = get_accounts(1)


nit: You don't need braces here

I like it without the braces, but it's with the braces everywhere in the codebase. For now I just follow the local convention.

raiden_contracts/data/source/services/ServiceRegistry.sol

amount of deposits required for a service registration.

require() statements.

because it `selfdestruct()`s.

pirapira commented Jul 8, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

pirapira commented Jul 8, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

pirapira commented Jul 8, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

pirapira commented Jul 8, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Show resolved Hide resolved

pirapira commented Jul 8, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

pirapira commented Jul 8, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

pirapira commented Jul 8, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

pirapira commented Jul 8, 2019

View reviewed changes

raiden_contracts/tests/test_deploy_script.py Outdated Show resolved Hide resolved

pirapira added the dev: Work In Progress label Jul 8, 2019

pirapira force-pushed the membership branch 2 times, most recently from aa61897 to 0c98616 Compare July 9, 2019 10:02

pirapira commented Jul 9, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Show resolved Hide resolved

pirapira commented Jul 9, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

pirapira commented Jul 9, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

pirapira commented Jul 9, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Show resolved Hide resolved

pirapira commented Jul 9, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

pirapira force-pushed the membership branch 3 times, most recently from 0afaadb to d4aa7e8 Compare July 9, 2019 16:32

pirapira commented Jul 9, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

pirapira force-pushed the membership branch 3 times, most recently from 4140515 to 1983528 Compare July 12, 2019 10:44

pirapira added dev: Please Review and removed dev: Work In Progress labels Jul 12, 2019

pirapira mentioned this pull request Jul 12, 2019

Add gas estimates for the ServiceRegistry #1122

Closed

2 tasks

pirapira force-pushed the membership branch 2 times, most recently from 8182ef4 to 2a6831d Compare July 15, 2019 08:18

pirapira force-pushed the membership branch from 4b39115 to 95b333e Compare July 15, 2019 16:17

karlb reviewed Jul 16, 2019

View reviewed changes

palango reviewed Jul 16, 2019

View reviewed changes

Document the return value policy

137aba2

palango reviewed Jul 16, 2019

View reviewed changes

raiden_contracts/data/source/services/ServiceRegistry.sol Outdated Show resolved Hide resolved

palango reviewed Jul 16, 2019

View reviewed changes

The old overflow check was not effective

dfbbc1c

and the new value is small enough. ``` >>> A = 200 >>> X = 2 ** 60 - 1 >>> P = 24 * (A ** 4) >>> Q = P + 24*(A**3)*X + 12*(A**2)*(X**2) + 4*A*(X**3) + X**4 >>> Q < (2 ** 256 - 1) True ```

palango reviewed Jul 16, 2019

View reviewed changes

Fix a typo

f5e16ca

palango reviewed Jul 16, 2019

View reviewed changes

karlb approved these changes Jul 16, 2019

View reviewed changes

pirapira added 11 commits July 16, 2019 14:30

Add a test for a too late withdraw from Deposit

8e64cce

Test extension of ServiceRegistry registration

b16bfa5

Document the event RegisteredService

f085044

Add error messages to ServiceRegistry

0143fdc

Add boolean success flags

fcb2d56

Change the name of option for setting the initial

5f59f7a

amount of deposits required for a service registration.

Deposit contract can disappear after withdrawal

d002a6b

Add error messages to Deposit contract's

8da6fba

require() statements.

Compile contracts

9ec2205

ServiceRegistry.withdraw() cannot return anything

fc68cc1

because it `selfdestruct()`s.

Camel-case ServiceRegistry

a7051ee

pirapira merged commit 1d1168a into raiden-network:master Jul 16, 2019

pirapira deleted the membership branch July 16, 2019 15:46

pirapira mentioned this pull request Sep 3, 2019

Make sure Raiden Network Smart Contracts Specification has the correct information, data fields and tables raiden-network/spec#253

Closed

pirapira mentioned this pull request Sep 13, 2019

Make sure Smart Contracts for Raiden Services has the correct information, data fields and tables raiden-network/spec#254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ServiceRegistry (without parameter reconfiguration) #1116

ServiceRegistry (without parameter reconfiguration) #1116

pirapira commented Jul 8, 2019

codecov bot commented Jul 8, 2019

codecov bot commented Jul 8, 2019 •

edited

Loading

karlb Jul 16, 2019

pirapira Jul 16, 2019

pirapira Jul 16, 2019

palango Jul 16, 2019

pirapira Jul 16, 2019

palango Jul 16, 2019

palango Jul 16, 2019

pirapira Jul 16, 2019

palango Jul 16, 2019

pirapira Jul 16, 2019

palango Jul 16, 2019

pirapira Jul 16, 2019

karlb Jul 16, 2019

palango Jul 16, 2019

pirapira Jul 16, 2019

palango Jul 16, 2019

pirapira Jul 16, 2019

palango Jul 16, 2019

palango Jul 16, 2019

pirapira Jul 16, 2019

palango Jul 16, 2019

pirapira Jul 16, 2019

ServiceRegistry (without parameter reconfiguration) #1116

ServiceRegistry (without parameter reconfiguration) #1116

Conversation

pirapira commented Jul 8, 2019

Getting and Extending the Membership

The Change of Deposit Amounts

Terms of the Deposits

codecov bot commented Jul 8, 2019

Codecov Report

codecov bot commented Jul 8, 2019 • edited Loading

Codecov Report

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 8, 2019 •

edited

Loading