Upgrade contracts to v0.14.0 #3581

rakanalh · 2019-03-04T15:07:33Z

This is a pre-requisite for #3511.

Separated into it's own PR to simplify the review process

rakanalh · 2019-03-04T15:10:12Z

raiden/tests/fixtures/variables.py

@@ -333,7 +331,7 @@ def private_rooms():
 @pytest.fixture
 def environment_type():
    """Specifies the environment type"""
-    return Environment.PRODUCTION
+    return Environment.DEVELOPMENT


Use latest version of contracts in tests. This is needed since the production mode still uses an old version of the contract where the interfaces in the new contract version were broken.

Then this is a breaking change that is not backwards compatible. When will we release this? If we do, I think it's the right time to also do #3254

hmm... i think it is backwards compatible because for people running Red eyes, using newer version of Raiden with this change is not going to break their existing nodes as they use "production" mode and that uses a specific contract version which is defined in Raiden as 0.4.0

We need a definition for backwards incompatibility. I remember we did discuss this in person, but I'm lacking the vocabulary to talk about this.

My understand persists though, this will not work with previous clients so we might as well fix the messages.

As discussed in the office, this is only for the Development contracts. Which means not the ones deployed on the mainnet. The point of breaking compatibility with red eyes is when we switch the Environment.PRODUCTION variable to point to anything other than Red Eyes.

So that would be the best point to have the message changes and any other backwards compatibility breaking changes we can fit in.

pirapira · 2019-03-04T17:40:34Z

@rakanalh shall I take over, proceed, and ask you questions when things get foggy?

rakanalh · 2019-03-07T09:57:22Z

@pirapira thanks for suggesting to take over... there were only some CLI tests that need to be fixed by the time i left this PR. Most of the other work has already been done.

raiden/network/proxies/token_network_registry.py

codecov · 2019-03-07T11:53:02Z

Codecov Report

Merging #3581 into master will decrease coverage by 0.33%.
The diff coverage is 80.64%.

@@            Coverage Diff             @@
##           master    #3581      +/-   ##
==========================================
- Coverage   77.56%   77.22%   -0.34%     
==========================================
  Files         104      103       -1     
  Lines       13880    13713     -167     
  Branches     1942     1923      -19     
==========================================
- Hits        10766    10590     -176     
- Misses       2451     2460       +9     
  Partials      663      663

Impacted Files	Coverage Δ
raiden/ui/app.py	`16.98% <0%> (ø)`	⬆️
raiden/settings.py	`100% <100%> (ø)`	⬆️
raiden/api/rest.py	`74.7% <100%> (ø)`	⬆️
raiden/network/proxies/token_network_registry.py	`83.33% <100%> (+0.98%)`	⬆️
raiden/constants.py	`100% <100%> (ø)`	⬆️
raiden/api/python.py	`72.13% <73.68%> (-1.05%)`	⬇️
raiden/storage/sqlite.py	`86.5% <0%> (-3.53%)`	⬇️
raiden/storage/restore.py	`84.61% <0%> (-2.89%)`	⬇️
raiden/waiting.py	`85.86% <0%> (-1.64%)`	⬇️
raiden/network/rpc/client.py	`58.53% <0%> (-1.22%)`	⬇️
... and 18 more

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 4c38ec6...f36fe12. Read the comment docs.

hackaugusto · 2019-03-07T14:38:39Z

raiden/network/proxies/token_network_registry.py

@@ -104,18 +136,20 @@ def add_token(self, token_address: TokenAddress, given_block_identifier: BlockSp

        checking_block = self.client.get_checking_block()
        error_prefix = 'Call to createERC20TokenNetwork will fail'
+
+        arguments = [token_address] + additional_arguments
        gas_limit = self.proxy.estimate_gas(
            checking_block,
            'createERC20TokenNetwork',


I thought we would merge the two smart contracts into one, which would allows us to remove the limits and have an easy upgrade, did that change?

Related issue raiden-network/raiden-contracts#694

We always have limits. Sometimes it's MAX_UINT256, meaning no limits.

hackaugusto · 2019-03-07T14:39:36Z

raiden/api/python.py

@@ -452,6 +467,11 @@ def set_total_channel_deposit(
        if channel_state is None:
            raise InvalidAddress('No channel with partner_address for the given token')

+        if self.raiden.config['environment_type'] == Environment.PRODUCTION:
+            per_token_network_deposit_limit = RED_EYES_PER_TOKEN_NETWORK_LIMIT


Wasn't this going to be provided by the smart contract itself?

After the latest change in the contracts, limits are no longer hard coded there but rather, they're specified once the contract is instantiated on-chain. For the existing Red eyes contracts, the limits are hard coded and we need to work with them, so that's what i am doing here.
I am guessing that what you expect is for a query to fetch the limits for the Red eyes contracts and then just use those right? If that's the case, i thought about it and opted for defining the limits in Red eyes since they're not going to change in the contracts anyway so i would shave-off the additional query. Let me know if you think this must be changed.

For the existing Red eyes contracts, the limits are hard coded and we need to work with them, so that's what i am doing here.

So, maintaining support for old contracts versions, but we are not maintaining compatibility across contract networks? why don't we just drop support for the old red eyes altogether?

Let me know if you think this must be changed.

Not really, a comment would be helpful though. An alternative is to use inheritance and have that optimization hidden in a RedEyes subclass.

We can't drop support for Red Eyes at this point when we have no other mainnet release ready.

We can't drop support for Red Eyes at this point when we have no other mainnet release ready.

That's okay, but IMO we should not deploy the new smart contracts in the mainnet without the message fix, since the networks will be disjoint.

raiden/api/python.py

hackaugusto · 2019-03-07T14:47:14Z

raiden/tests/utils/smoketest.py

+    else:
+        registry.add_token_without_limits(
+            token_address=to_canonical_address(token.contract.address),
+            given_block_identifier='latest',


Note: The places which are using given_block_identifier='latest' will have to be upgraded to follow the consistent view

I believe we use latest only in tests... i have checked if i am using this in other places in the code but my code doesn't really change anything that wasn't already using "latest".

Well, but the problem persists, the tests are susceptible to the same race conditions, I would argue that in the tests because we use PoA it's even more likely to happen.

…rsion

LefterisJP · 2019-03-12T12:02:56Z

As discussed during standup, I am giving this a review.

raiden/network/proxies/token_network_registry.py

raiden/constants.py

raiden/api/python.py

LefterisJP · 2019-03-12T12:36:51Z

raiden/tests/fixtures/variables.py

@@ -333,7 +331,7 @@ def private_rooms():
 @pytest.fixture
 def environment_type():
    """Specifies the environment type"""
-    return Environment.PRODUCTION
+    return Environment.DEVELOPMENT


As discussed in the office, this is only for the Development contracts. Which means not the ones deployed on the mainnet. The point of breaking compatibility with red eyes is when we switch the Environment.PRODUCTION variable to point to anything other than Red Eyes.

So that would be the best point to have the message changes and any other backwards compatibility breaking changes we can fit in.

LefterisJP · 2019-03-12T12:38:38Z

raiden/tests/integration/fixtures/smartcontracts.py

+                    token_address=token,
+                    channel_participant_deposit_limit=RED_EYES_PER_CHANNEL_PARTICIPANT_LIMIT,
+                    token_network_deposit_limit=RED_EYES_PER_TOKEN_NETWORK_LIMIT,
+                    given_block_identifier='latest',


As also discussed in the office with @hackaugusto please replace all these 'latest' with appropriate blockhashes, or make an issue pointing to them and having as the issue's task to do it.

raiden/tests/integration/fixtures/smartcontracts.py

raiden/tests/utils/smoketest.py

palango · 2019-03-12T12:47:12Z

Can you directly update to 0.14? That fixes a problem in the services that we found in 0.13

rakanalh · 2019-03-12T20:48:45Z

@palango yup... i upgraded to 14

[skip ci]

LefterisJP

So looking a lot better!

I added a small commit to fix a typo.

Two remaining comments:

The development version of the contracts seems wrong
Some uses of 'latest' remain.

LefterisJP · 2019-03-13T09:00:00Z

raiden/settings.py

@@ -46,6 +46,6 @@
 ETHERSCAN_API = 'https://{network}.etherscan.io/api?module=proxy&action={action}'

 RED_EYES_CONTRACT_VERSION = '0.4.0'
-DEVELOPMENT_CONTRACT_VERSION = '0.3._'
+DEVELOPMENT_CONTRACT_VERSION = '0.10.0'


Shouldn't this be 0.14.0 ???

The pypi package's version is different than the ones we have for contracts-version.
pypi version: 0.14.0
contracts version: 0.10.0

Here: https://github.com/raiden-network/raiden-contracts/blob/v0.14.0/raiden_contracts/data/source/raiden/TokenNetwork.sol#L14

Duh ... 😅

I remember last week I was explaining that to someone and now even I got confused. We gotta do something about the drift between packages and contract version. If it confused us, imagine the users.

Yeah. Maybe contract_version should just look very different. raiden-network/raiden-contracts#584

LefterisJP · 2019-03-13T09:00:31Z

raiden/tests/integration/fixtures/smartcontracts.py

+            else:
+                registry.add_token_without_limits(
+                    token_address=token,
+                    given_block_identifier='latest',


In this file you seem to still have 'latest' blocks. Will you use blockhash as we discussed?

I plan to create a separate issue for converting tests to use the actual blockhash instead of latest... but that's going to be in a different PR/issue.

Fair enough! Please do though, I normally would ask for the issue to be created first so we don't forget but let's get this merged since it's open for a while.

Created: #3619

rakanalh added the dev: Please Review label Mar 4, 2019

rakanalh commented Mar 4, 2019

View reviewed changes

hackaugusto mentioned this pull request Mar 6, 2019

Notify users about insufficient RDN balance in UserDeposit #3511

Merged

rakanalh commented Mar 7, 2019

View reviewed changes

raiden/network/proxies/token_network_registry.py Show resolved Hide resolved

hackaugusto reviewed Mar 7, 2019

View reviewed changes

rakanalh added 12 commits March 10, 2019 20:06

Use v13 of contracts

03b33bd

Pass limit arguments to contract transaction calls

4cd8d29

Define unlimited deposits, token networks

5197078

Check limits against deposit and token network balance

f403143

Use red eyes max number of token networks for smoketest

701cf0f

Remove import of limits in rest.py

3f3da52

Fix assertion on error message in test_api_deposit_limit

8e0645d

Pass additional token networks limit for development environment

63ff8ac

Contracts path in tests should be determined by environment type

7b03ba3

None is not useful if comparing against contract_manager.contracts_ve…

0238277

…rsion

Decide whether to call add_token with limit arguments or not

b5c9ca4

Use f-strings

bf053ee

LefterisJP reviewed Mar 12, 2019

View reviewed changes

rakanalh added 3 commits March 12, 2019 21:46

Use version 14

eff9b3a

Replace UNLIMITED_* with UINT256_MAX

486dde9

Document add_token_ functions

a4d06cc

rakanalh changed the title ~~Upgrade contracts to v0.13.0~~ Upgrade contracts to v0.14.0 Mar 12, 2019

rakanalh requested a review from LefterisJP March 12, 2019 20:48

Add typing

a84b251

rakanalh and others added 2 commits March 12, 2019 22:40

Add development smoketest to circle CI

c0110d6

Fix typo

f36fe12

[skip ci]

LefterisJP reviewed Mar 13, 2019

View reviewed changes

LefterisJP approved these changes Mar 13, 2019

View reviewed changes

LefterisJP merged commit 2dd6224 into raiden-network:master Mar 13, 2019

rakanalh deleted the upgrade-contracts branch March 13, 2019 10:11

rakanalh mentioned this pull request Mar 13, 2019

User Policy - Raiden needs to choose a pathfinding service from ServiceRegistry #3467

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade contracts to v0.14.0 #3581

Upgrade contracts to v0.14.0 #3581

rakanalh commented Mar 4, 2019 •

edited

Loading

rakanalh Mar 4, 2019

hackaugusto Mar 7, 2019

rakanalh Mar 7, 2019 •

edited

Loading

hackaugusto Mar 7, 2019

LefterisJP Mar 12, 2019

pirapira commented Mar 4, 2019

rakanalh commented Mar 7, 2019

codecov bot commented Mar 7, 2019 •

edited

Loading

hackaugusto Mar 7, 2019

hackaugusto Mar 7, 2019

pirapira Mar 7, 2019

hackaugusto Mar 7, 2019

rakanalh Mar 7, 2019

hackaugusto Mar 7, 2019

LefterisJP Mar 8, 2019

hackaugusto Mar 11, 2019

hackaugusto Mar 7, 2019

rakanalh Mar 10, 2019

hackaugusto Mar 11, 2019 •

edited

Loading

LefterisJP commented Mar 12, 2019

LefterisJP Mar 12, 2019

LefterisJP Mar 12, 2019

palango commented Mar 12, 2019

rakanalh commented Mar 12, 2019

LefterisJP left a comment

LefterisJP Mar 13, 2019

rakanalh Mar 13, 2019

LefterisJP Mar 13, 2019

pirapira Mar 13, 2019

LefterisJP Mar 13, 2019

rakanalh Mar 13, 2019

LefterisJP Mar 13, 2019

rakanalh Mar 13, 2019

Upgrade contracts to v0.14.0 #3581

Upgrade contracts to v0.14.0 #3581

Conversation

rakanalh commented Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakanalh Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirapira commented Mar 4, 2019

rakanalh commented Mar 7, 2019

codecov bot commented Mar 7, 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

hackaugusto Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

LefterisJP commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palango commented Mar 12, 2019

rakanalh commented Mar 12, 2019

LefterisJP left a 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

rakanalh commented Mar 4, 2019 •

edited

Loading

rakanalh Mar 7, 2019 •

edited

Loading

codecov bot commented Mar 7, 2019 •

edited

Loading

hackaugusto Mar 11, 2019 •

edited

Loading