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

Don't try to find service contracts for RedEyes #809

Merged
merged 2 commits into from
Apr 2, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
23 changes: 21 additions & 2 deletions raiden_contracts/contract_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from copy import deepcopy
from json import JSONDecodeError
from pathlib import Path
from semver import compare
from typing import Any, Dict, List, Optional

from deprecated import deprecated
Expand Down Expand Up @@ -164,6 +165,16 @@ def merge_deployment_data(dict1: Dict, dict2: Dict) -> Dict:
return result


def version_provides_services(version: Optional[str]) -> bool:
if version is None:
return True
if version == '0.3._':
return False
if version == '0.8.0_unlimited':
return True
return compare(version, '0.8.0') > -1
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this is confusing. What exactly do you want to achieve with the last check.

Looking at the docs of the semver module this will return true for versions >= 0.8.0.

If this is what you want to achieve then perhaps do compare(version, '0.8.0') >= 0 since this way it will be a bit more obvious what is happening. The -1 had me confused and I had to check the docs. And perhaps also add a docstring as not everyone knows how this compare function looks like.

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'll change to >= 0. I agree it's easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks man



def get_contracts_deployment_info(
chain_id: int,
version: Optional[str] = None,
Expand All @@ -172,7 +183,8 @@ def get_contracts_deployment_info(
"""Reads the deployment data.

Parameter:
module The name of the module. ALL means deployed contracts from all modules.
module The name of the module. ALL means deployed contracts from all modules that are
available for the version.
"""
if module not in DeploymentModule:
raise ValueError(f'Unknown module {module} given to get_contracts_deployment_info()')
Expand All @@ -186,7 +198,14 @@ def get_contracts_deployment_info(
services=False,
))

if module == DeploymentModule.SERVICES or module == DeploymentModule.ALL:
if module == DeploymentModule.SERVICES and not version_provides_services(version):
raise ValueError(
f'SERVICES module queried for version {version}, but {version}'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A space is missing.

'does not provide service contracts.',
)

if (module == DeploymentModule.SERVICES or module == DeploymentModule.ALL) and \
Copy link
Contributor

Choose a reason for hiding this comment

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

style nitpick:

We never use \ to prolong a line in raiden. Better use an extra variable for readability

module_match = module == DeploymentModule.SERVICES or module == DeploymentModule.ALL
if module_match and version_provides_services(version):
    file.append(....

Also wouldn't the check if version_provides_services(version) be enough here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't the check if version_provides_services(version) be enough here?

When people ask for module=RAIDEN, the service contracts should not be included in the result.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yes. My bad!

version_provides_services(version):
files.append(contracts_deployed_path(
chain_id=chain_id,
version=version,
Expand Down
25 changes: 25 additions & 0 deletions raiden_contracts/tests/test_deploy_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
get_contracts_deployed,
get_contracts_deployment_info,
merge_deployment_data,
version_provides_services,
)


Expand Down Expand Up @@ -112,3 +113,27 @@ def test_deploy_data_all(
def test_deploy_data_unknown_module():
with pytest.raises(ValueError):
get_contracts_deployment_info(3, None, module=None) # type: ignore


@pytest.mark.parametrize('chain_id', [3, 4, 42])
def test_deploy_data_for_redeyes_succeed(chain_id):
""" get_contracts_deployment_info() on RedEyes version should return a non-empty dictionary """
assert get_contracts_deployment_info(chain_id, '0.4.0')


@pytest.mark.parametrize('chain_id', [3, 4, 42])
def test_service_deploy_data_for_redeyes_fail(chain_id):
""" get_contracts_deployment_info() on RedEyes version should return a non-empty dictionary """
with pytest.raises(ValueError):
assert get_contracts_deployment_info(chain_id, '0.4.0', DeploymentModule.SERVICES)


def test_version_provides_services():
assert not version_provides_services('0.3._')
assert not version_provides_services('0.4.0')
assert version_provides_services('0.8.0')
assert version_provides_services('0.8.0_unlimited')
assert version_provides_services('0.10.1')
assert version_provides_services('0.11.0')
with pytest.raises(ValueError):
assert version_provides_services('not a semver')