Skip to content

TokenNetworkRegistry proxy checks against secret_registry_address zero#4883

Closed
pirapira wants to merge 3 commits into
raiden-network:developfrom
pirapira:zero-secret-registry
Closed

TokenNetworkRegistry proxy checks against secret_registry_address zero#4883
pirapira wants to merge 3 commits into
raiden-network:developfrom
pirapira:zero-secret-registry

Conversation

@pirapira
Copy link
Copy Markdown
Contributor

Fixes: #4882

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.

Before this PR, TokenNetworkRegistry sent a transaction even when secret_registry_address is zero, so the constructor of TokenNetwork would fail. After this PR, such transactions will not be sent. Moreover, when a sent transaction fails and secret_registry_address is zero, the proxy raises a RaidenUnrecoverableError because the proxy is probably talking to a wrong contract.

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 14, 2019

Codecov Report

Merging #4883 into develop will increase coverage by 0.04%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4883      +/-   ##
===========================================
+ Coverage    80.77%   80.81%   +0.04%     
===========================================
  Files          120      119       -1     
  Lines        14515    14461      -54     
  Branches      2238     2233       -5     
===========================================
- Hits         11724    11687      -37     
+ Misses        2133     2113      -20     
- Partials       658      661       +3
Impacted Files Coverage Δ
raiden/network/proxies/token_network_registry.py 72.47% <50%> (-4.67%) ⬇️
raiden/network/rpc/middleware.py 57.89% <0%> (-42.11%) ⬇️
raiden/exceptions.py 96.61% <0%> (-3.39%) ⬇️
raiden/tasks.py 68.08% <0%> (-2.51%) ⬇️
raiden/raiden_service.py 90.6% <0%> (-1.95%) ⬇️
raiden/utils/echo_node.py 67.79% <0%> (-1.7%) ⬇️
raiden/log_config.py 87.61% <0%> (-1.58%) ⬇️
raiden/utils/runnable.py 88.23% <0%> (-1.13%) ⬇️
raiden/network/rpc/client.py 75.24% <0%> (-0.91%) ⬇️
... and 37 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 b17fabd...b33222d. Read the comment docs.

Comment thread raiden/network/proxies/token_network_registry.py
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.

According to the contracts:

https://github.com/raiden-network/raiden-contracts/blob/94f1428d2fad4719efffb26be3d7731d7bb3f3da/raiden_contracts/data/source/raiden/TokenNetworkRegistry.sol#L70-L105

There is no require/assert for the secret registry address when createERC20TokenNetwork is called.

The assert exists in the constructor of the contract which means that if that fails there would be no usable instance of the contracts.

I don't think this change in this PR is required.

@pirapira
Copy link
Copy Markdown
Contributor Author

@rakanalh that function deploys TokenNetwork so you need to read the constructor of TokenNetwork as well. https://github.com/raiden-network/raiden-contracts/blob/94f1428d2fad4719efffb26be3d7731d7bb3f3da/raiden_contracts/data/source/raiden/TokenNetwork.sol#L229

When the require in TokenNetwork's constructor fails, TokenNetworkRegistry's createERC20TokenNetwork() function also fails.

Yoichi Hirai added 2 commits September 23, 2019 10:40
secret_registry_address being zero.
TokenNetworkRegistry's secret_registry_address changes to zero.
@pirapira pirapira force-pushed the zero-secret-registry branch from a089a68 to 172a391 Compare September 23, 2019 08:41
@pirapira pirapira requested a review from rakanalh September 23, 2019 08:51
@pirapira pirapira force-pushed the zero-secret-registry branch from 172a391 to b33222d Compare September 23, 2019 09:15
@pirapira
Copy link
Copy Markdown
Contributor Author

#4936 deals with the problem I was solving here.

@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's add_token() should check if secret_registry_address is zero

2 participants