-
Notifications
You must be signed in to change notification settings - Fork 49
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
Simplify contract manager #169
Simplify contract manager #169
Conversation
777381b
to
25bbca7
Compare
25bbca7
to
962c6bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Left some small comments.
raiden_contracts/contract_manager.py
Outdated
for contracts_dir in self._contracts_source_dirs.values(): | ||
res = compile_files( | ||
[str(file) for file in contracts_dir.glob('*.sol')], | ||
output_values=('abi', 'bin', 'ast'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we added the AST in the first place. @pcppcp any idea? We can remove this otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, maybe some weird solc
behavior that required enabling this option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ulope Can you check if it works without ast
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's 'required' (i.e. crashes when ast
isn't included) by py-solc.
Filed an issue for it.
raiden_contracts/contract_manager.py
Outdated
raise Exception('Could not compile the contract. Check that solc is available.') | ||
raise RuntimeError( | ||
'Could not compile the contract. Check that solc is available.', | ||
) from FileNotFoundError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that be or does it work your was as well?
except FileNotFoundError as e:
raise Exception('Could not compile the contract. Check that solc is available.')
raise RuntimeError(
'Could not compile the contract. Check that solc is available.',
) from e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, yeah... thanks for noticing.
962c6bb
to
30dbfee
Compare
This is a small refactor of the
ContractManager
in preparation for raiden-network/raiden#1550. Also fixes #125.