Skip to content

Checks against chain_id == 0 in TokenNetworkRegistry#4887

Closed
pirapira wants to merge 2 commits into
raiden-network:developfrom
pirapira:chain-id-zero
Closed

Checks against chain_id == 0 in TokenNetworkRegistry#4887
pirapira wants to merge 2 commits into
raiden-network:developfrom
pirapira:chain-id-zero

Conversation

@pirapira
Copy link
Copy Markdown
Contributor

Fixes: #4886

Description

Please, describe what this PR does in detail:

  • If it's a bug fix, detail the root cause of the bug and how this PR fixes it.

A proxy should try to avoid sending a transaction that's going to fail. Before this PR, TokenNetworkRegistry didn't check a condition: namely, chain_id state variable must hold a non-zero value. Otherwise, createERC20TokenNetwork() calls will fail.

This PR adds a precondition check and a post-failure check against this case.

PR review check list

Quality check list that cannot be automatically verified.

  • Safety
  • Code quality
    • Error conditions are handled
    • Exceptions are propagated to the correct parent greenlet
    • Exceptions are correctly classified as recoverable or unrecoverable
  • Compatibility
    • State changes are forward compatible
    • Transport messages are backwards and forward compatible
  • Commits
    • Have good messages
    • Squashed unecessary commits
  • If it's a bug fix:
    • Regression test for the bug
      • Properly covers the bug
      • If an integration test is used, it could not be written as a unit test
  • Documentation
    • A new CLI flag was introduced, is there documentation that explains usage?
  • Specs
    • If this is a protocol change, are the specs updated accordingly? If so, please link PR from the specs repo.
  • Is it a user facing feature/bug fix?
    • Changelog entry has been added

Yoichi Hirai added 2 commits September 14, 2019 17:51
This getter will be useful for precondition checks.
When TokenNetworkRegistry's createERC20TokenNetwork() tries to
deploy a TokenNetwork contract, the constructor of TokenNetwork
fails if chain_id stored in TokenNetworkRegistry is zero.

This commit adds checks against this case in the proxy of
TokenNetworkRegistry.
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 14, 2019

Codecov Report

Merging #4887 into develop will decrease coverage by 0.12%.
The diff coverage is 42.85%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4887      +/-   ##
===========================================
- Coverage    80.83%   80.71%   -0.13%     
===========================================
  Files          119      119              
  Lines        14454    14461       +7     
  Branches      2231     2233       +2     
===========================================
- Hits         11684    11672      -12     
- Misses        2116     2128      +12     
- Partials       654      661       +7
Impacted Files Coverage Δ
raiden/network/proxies/token_network_registry.py 72.47% <42.85%> (-2.04%) ⬇️
raiden/storage/serialization/types.py 64.17% <0%> (-4.48%) ⬇️
raiden/waiting.py 81.54% <0%> (-4.17%) ⬇️
raiden/tasks.py 68.08% <0%> (-2.84%) ⬇️
raiden/utils/echo_node.py 67.79% <0%> (-1.7%) ⬇️
raiden/network/transport/matrix/transport.py 80.29% <0%> (-0.9%) ⬇️
raiden/raiden_event_handler.py 91.41% <0%> (-0.62%) ⬇️
raiden/transfer/mediated_transfer/mediator.py 90.05% <0%> (-0.2%) ⬇️
raiden/network/proxies/token_network.py 50.24% <0%> (+0.49%) ⬆️
raiden/api/python.py 68.86% <0%> (+0.52%) ⬆️
... and 1 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 1edc704...a361a52. Read the comment docs.

Copy link
Copy Markdown
Contributor

@rakanalh rakanalh left a comment

Choose a reason for hiding this comment

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

Wrote why those changes are not required somewhere else

@pirapira
Copy link
Copy Markdown
Contributor Author

@rakanalh says #4936 is about the same issue.

@pirapira pirapira closed this Sep 23, 2019
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.

TokenNetworkRegistry should check chain_id before calling createERC20TokenNetwork()

2 participants