Fix bug settled channel #512
Conversation
The problem was that we changed the `last_known_block` in the `Context`
from an independent value towards a a property returning the value of
`latest_known_block` of `BlockchainState`, which is only updated when
all events for a given block range are processed.
This led to the following condition to always be `False`.
```py
settle_period_end_block = event.block_number + channel.settle_timeout
settle_period_over = settle_period_end_block < context.latest_confirmed_block
if not settle_period_over:
...
```
This in turn let the MS to try to send an update to the blockchain for a
closed channel.
We have tests for that behavior, however we wrongly updated the
`BlockchainState` as well, which led to the tests not catching this.
In `monitor` and `claim` we don't convert to hex addresses either.
- Add some type annotations - reorder some functions - Rename some variables
Codecov Report
@@ Coverage Diff @@
## master #512 +/- ##
=========================================
+ Coverage 91.21% 91.31% +0.1%
=========================================
Files 36 36
Lines 2255 2258 +3
Branches 287 287
=========================================
+ Hits 2057 2062 +5
+ Misses 149 148 -1
+ Partials 49 48 -1
Continue to review full report at Codecov.
|
| @@ -41,16 +41,23 @@ | |||
| class Context: | |||
| ms_state: MonitoringServiceState | |||
| db: Database | |||
| w3: Web3 | |||
| web3: Web3 | |||
There was a problem hiding this comment.
This rename is something I also wanted to do for a while.
|
I relied to much an the tests when doing the change. Can you suggest a new test to catch this? |
Yeah, so if I undo the change at https://github.com/raiden-network/raiden-services/pull/512/files#diff-6d7bdd065a1e0f2407c94540508e8707R123 then actually the test That's why I chose not to add more tests, but we could do something like |
OK, that's a really unlucky case. Then the tests are probably fine as they are. |
The problem was that we changed the
last_known_blockin theContextfrom an independent value towards a a property returning the value oflatest_known_blockofBlockchainState, which is only updated when all events for a given block range are processed.This led to the following condition to always be
False.This in turn let the MS to try to send an update to the blockchain for a closed channel.
We have tests for that behavior, however we wrongly updated the
BlockchainStateas well, which led to the tests not catching this.This also includes some cleanups I did while fixing the bug.
Fixes #511