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

Poor definition of function visibility #11

Open
joshuahannan opened this issue Apr 25, 2018 · 0 comments
Open

Poor definition of function visibility #11

joshuahannan opened this issue Apr 25, 2018 · 0 comments

Comments

@joshuahannan
Copy link

All of WalletMainLib’s functions have public visibility. Functions like calcConfirmsNeeded or getAmount should be private, and functions that are only intended to be called from “the outside world” (i.e. not by a deployed contract) should be external. For example, functions that represent actions from the owners of the wallet, such as serveTx, addOwner, removeOwner, etc, should be external instead of public because they are not intended to be called from within the library functions themselves.
As a result, the code will not only be more explicit, but could also save gas. The reason for such lower gas costs is that external calls don’t need to copy arguments to memory and can directly read from calldata.
Consider reviewing the visibility of all the functions in the wallet contracts, as per solidity.readthedocs.io and implementing the recommendations and best practices were possible.

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

No branches or pull requests

1 participant