Skip to content

Remove checks that are not in the onchain contract#4956

Merged
LefterisJP merged 1 commit into
raiden-network:developfrom
pirapira:remove-nonexistent-check
Sep 23, 2019
Merged

Remove checks that are not in the onchain contract#4956
LefterisJP merged 1 commit into
raiden-network:developfrom
pirapira:remove-nonexistent-check

Conversation

@pirapira
Copy link
Copy Markdown
Contributor

TokenNetworkRegistry.createERC20() succeeds even when the stored
chain_id value is different from the current chain_id of the blockchain.

Since the proxies should not check what's not checked onchain,
I'm removing the checks that do not exist in the onchain contracts.

This PR comes from this conversation #4936 (comment)

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.

It's a bug fix. Before this PR, the proxy failed even when the transaction would succeed.

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

@auto-assign auto-assign Bot requested a review from LefterisJP September 23, 2019 14:31
@pirapira pirapira mentioned this pull request Sep 23, 2019
13 tasks
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 23, 2019

Codecov Report

Merging #4956 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4956      +/-   ##
===========================================
- Coverage    80.47%   80.45%   -0.02%     
===========================================
  Files          120      120              
  Lines        14603    14599       -4     
  Branches      2265     2263       -2     
===========================================
- Hits         11752    11746       -6     
  Misses        2185     2185              
- Partials       666      668       +2
Impacted Files Coverage Δ
raiden/network/proxies/token_network_registry.py 53.76% <ø> (+1.13%) ⬆️
raiden/storage/serialization/types.py 59.7% <0%> (-8.96%) ⬇️
raiden/tasks.py 67.64% <0%> (-2.95%) ⬇️
raiden/utils/echo_node.py 67.79% <0%> (-1.7%) ⬇️
raiden/network/proxies/token_network.py 50.06% <0%> (-0.25%) ⬇️
raiden/transfer/channel.py 89.21% <0%> (-0.23%) ⬇️
raiden/transfer/mediated_transfer/mediator.py 90.44% <0%> (ø) ⬆️
raiden/network/transport/matrix/transport.py 80.64% <0%> (+0.3%) ⬆️
raiden/api/python.py 68.61% <0%> (+0.53%) ⬆️
raiden/transfer/node.py 94.79% <0%> (+0.54%) ⬆️
... 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 f660104...72f61b5. Read the comment docs.

TokenNetworkRegistry.createERC20() succeeds even when the stored
chain_id value is different from the current chain_id of the blockchain.

Since the proxies should not check what's not checked onchain,
I'm removing the checks that do not exist in the onchain contracts.
@pirapira pirapira force-pushed the remove-nonexistent-check branch from 43de8e2 to 72f61b5 Compare September 23, 2019 16:59
Copy link
Copy Markdown
Contributor

@LefterisJP LefterisJP left a comment

Choose a reason for hiding this comment

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

LGTM, I agree with this change

@LefterisJP LefterisJP merged commit 96672e3 into raiden-network:develop Sep 23, 2019
@pirapira pirapira deleted the remove-nonexistent-check branch September 24, 2019 07:48
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