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

get_contract_deployment_info that returns both service and raiden deployment info by default #775

Merged
merged 6 commits into from
Mar 26, 2019

Conversation

pirapira
Copy link
Contributor

get_contracts_deployed() had two problems

This PR fixes both by introducing a new interface function get_contracts_deployment_info() that takes module: str instead of services: bool. Moreover, module defaults to all, meaning "give me deployment info for all modules".

Also get_contracts_deployed() is deprecated.

which is similar to get_contracts_deployed() but
* does not take `services:bool` argument
* takes `module:str` argument that can be 'raiden' 'services' or 'all'

The default is 'all' because in most uses, all deployment data are wanted.

This fixes raiden-network#774
@pirapira
Copy link
Contributor Author

I have to change README too.

@pirapira pirapira removed the request for review from LefterisJP March 26, 2019 11:28
@pirapira
Copy link
Contributor Author

I realized somehow deployment['contracts'] were just overwritten when I do deployment.update(another_deployment).

@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #775 into master will increase coverage by 0.18%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
+ Coverage   81.84%   82.02%   +0.18%     
==========================================
  Files          20       20              
  Lines        1190     1202      +12     
  Branches      115      119       +4     
==========================================
+ Hits          974      986      +12     
  Misses        186      186              
  Partials       30       30
Impacted Files Coverage Δ
raiden_contracts/contract_manager.py 94.59% <89.47%> (+1.04%) ⬆️

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 13b1fba...b816db6. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #775 into master will increase coverage by 0.32%.
The diff coverage is 92.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
+ Coverage   81.84%   82.17%   +0.32%     
==========================================
  Files          20       20              
  Lines        1190     1223      +33     
  Branches      115      123       +8     
==========================================
+ Hits          974     1005      +31     
- Misses        186      187       +1     
- Partials       30       31       +1
Impacted Files Coverage Δ
raiden_contracts/deploy/contract_verifyer.py 98.03% <100%> (-0.21%) ⬇️
raiden_contracts/contract_manager.py 94.39% <92.3%> (+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 13b1fba...389bbdf. Read the comment docs.

CHANGELOG.md Outdated Show resolved Hide resolved
raiden_contracts/contract_manager.py Outdated Show resolved Hide resolved
raiden_contracts/contract_manager.py Outdated Show resolved Hide resolved
raiden_contracts/contract_manager.py Outdated Show resolved Hide resolved
raiden_contracts/tests/test_deploy_data.py Outdated Show resolved Hide resolved
raiden_contracts/tests/test_deploy_data.py Outdated Show resolved Hide resolved
and make tests more deterministic.
def get_contracts_deployment_info(
chain_id: int,
version: Optional[str] = None,
module: str = 'all',
Copy link
Contributor

Choose a reason for hiding this comment

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

I had commented about it before but it somehow got lots. Can you change this module to an Enum? It will be much better typed, easier to check and a better interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There you go. I defined and used an Enum.

get_contracts_deployment_info() used to take a module name as a string,
but sinsce there are only a few valid values, Enum is more suitable.
@pirapira pirapira force-pushed the get_contract_deployment_info branch from 38452bf to 389bbdf Compare March 26, 2019 15:10
@pirapira
Copy link
Contributor Author

@LefterisJP merge?

@@ -125,6 +126,12 @@ def contracts_deployed_path(
return data_path.joinpath(f'deployment_{"services_" if services else ""}{chain_name}.json')


class DeploymentModule(Enum):
RAIDEN = 'raiden'
Copy link
Contributor

Choose a reason for hiding this comment

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

you can also have those as: RAIDEN = 1, SERVICES = 2, ALL = 3. No need to be strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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