-
Notifications
You must be signed in to change notification settings - Fork 244
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
cleans up a number of visibility warnings #235
Conversation
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.
🤘 Awesome!
It'd be awesome if solidity had a way to ignore warnings from certain files / directories (e.g. the /libs
folder), but fixing old contracts is good too :).
@@ -36,7 +36,7 @@ contract EVMScriptRunner is AppStorage, EVMScriptRegistryConstants { | |||
/** | |||
* @dev copies and returns last's call data. Needs to ABI decode first | |||
*/ | |||
function returnedDataDecoded() internal view returns (bytes ret) { | |||
function returnedDataDecoded() internal pure returns (bytes ret) { |
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.
I'm not 100% sure, but I think this should be a view
function since it's relying on manipulating memory. Correct me if I'm wrong though; my understanding was that pure
is for functions that only use their params.
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.
Note to self: clearly I was wrong. view
is only necessary for SLOAD
and SSTORE
.
@@ -51,7 +51,7 @@ contract DelegateScript is IEVMScriptExecutor { | |||
/** | |||
* @dev copies and returns last's call data | |||
*/ | |||
function returnedData() internal view returns (bytes ret) { | |||
function returnedData() internal pure returns (bytes ret) { |
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.
See above.
.travis.yml
Outdated
@@ -8,6 +8,7 @@ node_js: | |||
- '8' | |||
env: | |||
- TASK=lint | |||
- TASK=test |
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.
Why has this been added in this PR? Until we merge #186, running the test suite isn't relevant, as if some test fails, coverage will error.
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.
I'll remove.
So it turns out we can't really use Some of the previous changes still pass, since we're only using them with |
No description provided.