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

Merge arbitrum branch #1160

Merged
merged 11 commits into from
Feb 24, 2022
Merged

Merge arbitrum branch #1160

merged 11 commits into from
Feb 24, 2022

Conversation

karlb
Copy link
Contributor

@karlb karlb commented Feb 24, 2022

The arbitrum changes have reached a consistent working state. Let's merge it into master to prevent further divergence.

Fernando Cezar and others added 10 commits February 15, 2022 14:29
When the user does not manually define a number of confirmation blocks for the
PFS or MS and the chain is detected to be Arbitrum, it will automatically set
the number to zero. On Arbitrum based chains there are no confirmation blocks
necessary.
When the scheduled event with the lowest timestamp fails with a
TransactionTooEarlyException, then we know that all other events would
do that, too. So there is no reason to continue executing scheduled
events at the moment.
We don't want to put too much load on the ETH-RPC node and risk getting
blocked or rate limited.
This would have helped when working on the arbitrum branches.
@karlb karlb requested a review from weilbith February 24, 2022 15:53
@auto-assign auto-assign bot requested a review from fredo February 24, 2022 15:53
@karlb karlb removed the request for review from fredo February 24, 2022 15:53
@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #1160 (21e3b48) into master (661e18b) will increase coverage by 0.13%.
The diff coverage is 98.05%.

❗ Current head 21e3b48 differs from pull request most recent head 2ecb055. Consider uploading reports for the commit 2ecb055 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1160      +/-   ##
==========================================
+ Coverage   88.81%   88.95%   +0.13%     
==========================================
  Files          43       44       +1     
  Lines        3461     3494      +33     
  Branches      493      498       +5     
==========================================
+ Hits         3074     3108      +34     
+ Misses        281      280       -1     
  Partials      106      106              
Impacted Files Coverage Δ
src/monitoring_service/states.py 98.23% <ø> (-0.02%) ⬇️
src/pathfinding_service/model/channel.py 97.02% <ø> (-0.12%) ⬇️
src/raiden_libs/blockchain.py 84.21% <ø> (ø)
src/raiden_libs/service_registry.py 81.67% <50.00%> (ø)
src/monitoring_service/cli.py 82.53% <80.00%> (-0.52%) ⬇️
src/monitoring_service/constants.py 100.00% <100.00%> (ø)
src/monitoring_service/database.py 100.00% <100.00%> (ø)
src/monitoring_service/events.py 100.00% <100.00%> (ø)
src/monitoring_service/exceptions.py 100.00% <100.00%> (ø)
src/monitoring_service/handlers.py 90.94% <100.00%> (+1.35%) ⬆️
... and 12 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 661e18b...2ecb055. Read the comment docs.

Copy link
Contributor

@weilbith weilbith left a comment

Choose a reason for hiding this comment

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

I think you forgot this commit here.
As CI runs through successfully: optimistic approval after changes ✔️

Importing anything from eth-tester does not work in production as it is a test
dependency that is not available. In result the monitoring service was failing
to start.
The import was used to test if a failed update balance proof transaction should
be retried or not. The challenge is that in test mode, web3 forwards eth-tester
exceptions untouched. That is not the same in production mode. Thereby the error
looks slightly different. But both are based on the BaseException class. So it
is possible to just verify their error message without matching the exact error
class.

This bug got introduced with a recent feature implementation. Unfortunately the
CI pipeline was not properly working, which allowed to introduce this mistake.
@karlb karlb merged commit 7fa5ffe into raiden-network:master Feb 24, 2022
@karlb karlb deleted the merge-arbitrum branch February 24, 2022 16:37
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.

3 participants