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

Fix eth private func calls #1306

Merged
merged 6 commits into from Dec 18, 2018

Conversation

Projects
None yet
2 participants
@disconnect3d
Copy link
Contributor

disconnect3d commented Dec 17, 2018

Proper fix for the (#1303) bug where we couldn't call a contract function that was prefixed with an underscore.

Before this PR we checked for attributes starting with an underscore in __getattribute__. If we found one - we used object.__getattribute__(self, name).

This PR removes the check and uses __getattr__ instead. This way, we don't have to handle the cases when we want to access EVMContract attributes as they just work as is.

Side note: https://stackoverflow.com/questions/3278077/difference-between-getattr-vs-getattribute


This change is Reviewable

if func_name.startswith('__') or func_name in {'add_function', 'address', 'name_'}:
# TODO(mark): is this actually true? is there anything actually wrong with a solidity name beginning w/ an underscore?
raise EthereumError("Function name ({}) is internally reserved".format(func_name))
if func_name.startswith('_EVMContract__') or func_name in {'add_function', 'address', 'name_'}:

This comment has been minimized.

@disconnect3d

disconnect3d Dec 17, 2018

Contributor

Hmm this doesn't protect against making a function named: _default_caller, _hashes, _initialized.

I think I will make those protected (use __ instead of _) so they are not accessible from the outside at all (and so their real attribute name starts with _EVMContract__).

This comment has been minimized.

@mossberg

mossberg Dec 17, 2018

Contributor

maybe this check should simply check that func_name not in self.__dict__ or something? this way we can automatically ensure that there are none of these collisions and not have to update the list if we add attributes.

This comment has been minimized.

@disconnect3d

disconnect3d Dec 17, 2018

Contributor

image
Yes! This + checking for attributes that matches the function names.

@mossberg
Copy link
Contributor

mossberg left a comment

great work, this makes this whole module so much better ^_^

overall looks great, but had a few comments

@@ -461,6 +461,25 @@ def test_create_contract_two_instances(self):
self.assertEqual(len(contracts), len(set(c.address for c in contracts)))
self.assertEqual(len(contracts), len(set(c.name_ for c in contracts)))

def test_contract_create_and_call_underscore_function(self):

This comment has been minimized.

@mossberg

mossberg Dec 17, 2018

Contributor

you are a hero for including these tests!! :D

"""
if not name.startswith('_'):
if not self._initialized:
self.__init_hashes()

This comment has been minimized.

@mossberg

mossberg Dec 17, 2018

Contributor

minor concern, calling __init_hashes does not necessarily guarantee that self._hashes is initialized to a dict, so the below name in self._hashes check could crash. this would only happen is self._manticore was None which probably won't happen, but still wanted to mention it. may be worth keeping the self._hashes is not None check.

alternate idea. simply initialize _hashes to an empty dict? not exactly clear on why we need to separately init it to None, and then to a dict.

third idea. we modify __init_hashes to ensure that _hashes is ALWAYS set to a dict for all paths through it, then we don't need the is not None check.

This comment has been minimized.

@disconnect3d

disconnect3d Dec 17, 2018

Contributor

Hmm I think we can assume it is initialized. I will add an assertion to EVMAccount whether self._manticore is a ManticoreEVM instance.

This comment has been minimized.

@disconnect3d

disconnect3d Dec 17, 2018

Contributor

I also think that self.__hashes = {} should be in __init__.

Show resolved Hide resolved manticore/ethereum/account.py
Show resolved Hide resolved manticore/ethereum/account.py Outdated
if func_name.startswith('__') or func_name in {'add_function', 'address', 'name_'}:
# TODO(mark): is this actually true? is there anything actually wrong with a solidity name beginning w/ an underscore?
raise EthereumError("Function name ({}) is internally reserved".format(func_name))
if func_name.startswith('_EVMContract__') or func_name in {'add_function', 'address', 'name_'}:

This comment has been minimized.

@mossberg

mossberg Dec 17, 2018

Contributor

maybe this check should simply check that func_name not in self.__dict__ or something? this way we can automatically ensure that there are none of these collisions and not have to update the list if we add attributes.

@disconnect3d disconnect3d merged commit 94991c8 into master Dec 18, 2018

5 checks passed

codeclimate All good!
Details
codeclimate/total-coverage 70% (0.0% change)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@disconnect3d disconnect3d deleted the fix-eth-private-func-calls branch Dec 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment