Skip to content

Checking if Solc satisfies version requirements#1394

Closed
Arbitrage0 wants to merge 13 commits intoraiden-network:masterfrom
Arbitrage0:Arbitrage0-compare_solc_version_requirements
Closed

Checking if Solc satisfies version requirements#1394
Arbitrage0 wants to merge 13 commits intoraiden-network:masterfrom
Arbitrage0:Arbitrage0-compare_solc_version_requirements

Conversation

@Arbitrage0
Copy link
Copy Markdown

Solves issue #1365

A few things, firstly I'm new to open-source contributions and I've been dabbling with Python for a bit, so I wanted to put my skills to use on Raiden! Please do tell me what I'm doing wrong (or right).

Secondly, this relies on the compiler_version() method of _solidity.py in ethereum.tools but I have a feeling that the Regex on line 127 might be a bit messed up, since I tried testing it and it doesn't come out right sometimes. I'll defer to their expertise though, might just be me.

I've also imported the inbuilt LooseVersion to save time on developing a version comparison function from scratch.

Solves raiden-network#1365

A few things, firstly I'm new to open-source contributions and I've been dabbling with Python for a bit, so I wanted to put my skills to use on Raiden. Please do tell me what I'm doing wrong (or right). 

Secondly, this relies on the compiler_version() method of _solidity.py in ethereum.tools but I have a feeling that the Regex on line 127 might be a bit messed up, since I tried testing it and it doesn't come out right sometimes. I'll defer to their expertise though, might just be me. 

I've also imported the inbuilt LooseVersion to save time on developing a version comparison function from scratch.
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 4, 2018

CLA assistant check
All committers have signed the CLA.

Comment thread raiden/blockchain/abi.py Outdated

elif LooseVersion(_solidity.compiler_version()) < LooseVersion(MIN_REQUIRED_SOLC):
raise RuntimeError(
"You are currently using the solidity compiler version {}.\n".format{_solidity.compiler_version())
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sorry, this is meant to be .format(_solidity.compiler_version())

@LefterisJP
Copy link
Copy Markdown
Contributor

Hey @Arbitrage0 you have style violations. Run flake8 on the code to see what they are or just see where the travis job failed

@Arbitrage0
Copy link
Copy Markdown
Author

Arbitrage0 commented May 4, 2018

So I think now everything is running great from my end, however the AttributeError that gets thrown up is because the Regex in the _solidity.py compiler_version() method probably isn't working (there's an extra '/' in the end which messes everything up), so it returns None instead of the actual version because there isn't any match, and None has no version attribute.

Should I create another method from scratch? I've sent a pull request to the guys at pyethereum about this as well.

Comment thread raiden/blockchain/abi.py Outdated

elif LooseVersion(_solidity.compiler_version()) < LooseVersion(MIN_REQUIRED_SOLC[1:]):
raise RuntimeError(
'You are currently using the solidity compiler version {0}.\n'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no need to add numbers inside the {} for the format string

Comment thread raiden/blockchain/abi.py Outdated
elif LooseVersion(_solidity.compiler_version()) < LooseVersion(MIN_REQUIRED_SOLC[1:]):
raise RuntimeError(
'You are currently using the solidity compiler version {0}.\n'
'Please upgrade to the compatible version {1}.'.format(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please upgrade to the compatible version -> Please upgrade to the minimum compatible version

@LefterisJP
Copy link
Copy Markdown
Contributor

@Arbitrage0 Hello again. Thank you for your contribution! Please address my 2 comments and also squash all your 6 commits into one.

Arbitrage0 and others added 7 commits May 8, 2018 04:05
Solves raiden-network#1365

A few things, firstly I'm new to open-source contributions and I've been dabbling with Python for a bit, so I wanted to put my skills to use on Raiden. Please do tell me what I'm doing wrong (or right).

Secondly, this relies on the compiler_version() method of _solidity.py in ethereum.tools but I have a feeling that the Regex on line 127 might be a bit messed up, since I tried testing it and it doesn't come out right sometimes. I'll defer to their expertise though, might just be me.

I've also imported the inbuilt LooseVersion to save time on developing a version comparison function from scratch.

Update abi.py

Update abi.py

Update abi.py

Update abi.py

Update abi.py
@palango
Copy link
Copy Markdown
Contributor

palango commented May 31, 2018

Hi @Arbitrage0 , we updated some code related to your change. In order to merge your PR you need to update this PR. Thank you.

@palango
Copy link
Copy Markdown
Contributor

palango commented Jun 13, 2018

Hi @Arbitrage0 . We haven't heard back from you in two weeks so I'm closing this PR. Feel free to reopen when you revisit this.

@palango palango closed this Jun 13, 2018
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.

5 participants