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

MonitoringService update + contracts recompilation #747

Closed
wants to merge 6 commits into from

Conversation

palango
Copy link
Contributor

@palango palango commented Mar 18, 2019

Based on #744

This contains an update of solc from 0.5.4 to 0.5.6 (because that's what I have installed locally).

Because the eth_tester dependency doesn't seem fit for the petersburg EVM I force solc to output byzantium compatible code, but this should be fixed later.

@palango palango requested a review from pirapira March 18, 2019 10:30
@palango palango force-pushed the monitoring-update2 branch from a2613f6 to 6c51b26 Compare March 18, 2019 10:34
@codecov
Copy link

codecov bot commented Mar 18, 2019

Codecov Report

Merging #747 into master will decrease coverage by 19.75%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #747       +/-   ##
===========================================
- Coverage   81.47%   61.72%   -19.76%     
===========================================
  Files          18       18               
  Lines        1139     1139               
  Branches      113      113               
===========================================
- Hits          928      703      -225     
- Misses        183      415      +232     
+ Partials       28       21        -7
Impacted Files Coverage Δ
raiden_contracts/contract_source_manager.py 72.22% <ø> (-18.06%) ⬇️
raiden_contracts/utils/events.py 16.66% <0%> (-72.73%) ⬇️
raiden_contracts/utils/pending_transfers.py 28.84% <0%> (-71.16%) ⬇️
raiden_contracts/utils/logs.py 20.19% <0%> (-67.31%) ⬇️
raiden_contracts/utils/merkle.py 26.47% <0%> (-55.89%) ⬇️
raiden_contracts/utils/proofs.py 45.94% <0%> (-45.95%) ⬇️
raiden_contracts/utils/signature.py 56% <0%> (-32%) ⬇️
raiden_contracts/contract_manager.py 87.09% <0%> (-6.46%) ⬇️
raiden_contracts/deploy/__main__.py 73.88% <0%> (-2.68%) ⬇️

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 ef561cb...58f49b9. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 18, 2019

Codecov Report

Merging #747 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #747   +/-   ##
=======================================
  Coverage   81.47%   81.47%           
=======================================
  Files          18       18           
  Lines        1139     1139           
  Branches      113      113           
=======================================
  Hits          928      928           
  Misses        183      183           
  Partials       28       28
Impacted Files Coverage Δ
raiden_contracts/contract_source_manager.py 90.27% <ø> (ø) ⬆️

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 ef561cb...58f49b9. Read the comment docs.

@@ -55,6 +55,7 @@ def relativise(path):
output_values=PRECOMPILED_DATA_FIELDS + ['ast'],
import_remappings=import_dir_map,
optimize=False,
evm_version='byzantium',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think evm_version should be recorded in contracts.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • change contract_manager.py to read/write the new field
  • set up a default value for evm_version.

Maybe I should do it later. When I modify contract_manager.py, I can also manually add the field to the existing contracts.jsons with the new solc versions.

(, channel_state) = token_network.getChannelInfo(
// Only allowed to claim, if channel is out of the settlement period
uint256 settle_block_number;
(settle_block_number,) = token_network.getChannelInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks tricky. settle_block_number is sometimes 0, and sometimes the length of the settlement period. I think both channel_state and settle_block_number are necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, we should at least check that the channel_state is closed.

Copy link
Contributor

Choose a reason for hiding this comment

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

That should work.

@pirapira
Copy link
Contributor

You can also tell me to implement it the way I like.

@pirapira
Copy link
Contributor

#749 took over the change about the MS rewards. #748 will keep track of the Solidity upgrade.

@pirapira pirapira closed this Mar 19, 2019
@palango palango deleted the monitoring-update2 branch December 10, 2021 12:58
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