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

removed properties from vm.header and block, all tests passed #1795

Merged

Conversation

Projects
None yet
2 participants
@dherykw
Copy link
Contributor

commented Jun 26, 2019

What was wrong?

class vm using properties to access and retreive values of block and header

How was it fixed?

changing from properties to getter and setter methods in files where were requiered.

Cute Animal Picture

put a cute animal picture link inside the parentheses

@pipermerriam
Copy link
Member

left a comment

Minor adjustment to remove the set_block method. Otherwise 👍

uncles_hash=keccak(rlp.encode(block.uncles)),
),
uncles=block.uncles,
self.set_block(

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Jun 26, 2019

Member

I'm inclined to advocate for removing this method in favor of directly setting via self._block = ...

The only place that set_block appears to be used is from the tests in tests/json-fixtures/test_virtual_machine.py which I'm fine with that accessing the private vm._block property for now.

This comment has been minimized.

Copy link
@dherykw

dherykw Jul 5, 2019

Author Contributor

this was solved in the 9bd96bb commit

@@ -640,7 +639,7 @@ def set_block_transactions(self,
#
def _assign_block_rewards(self, block: BaseBlock) -> None:
block_reward = self.get_block_reward() + (
len(block.uncles) * self.get_nephew_reward()
len(block.uncles) * self.get_nephew_reward()

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Jun 26, 2019

Member

This indentation change looks like it should be reverted.

This comment has been minimized.

Copy link
@dherykw

dherykw Jul 5, 2019

Author Contributor

this was solved in the 9bd96bb commit

german@hopu.eu
@dherykw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

any help? I am using pycharm CE

@pipermerriam

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

There are 4 linting errors you can see here: https://circleci.com/gh/ethereum/py-evm/132998

You should be able to see these locally if you run tox -e py36-lint which will run the full linter. Looks like just indentation changes that need to be fixed.

german@hopu.eu added some commits Jun 29, 2019

german@hopu.eu
german@hopu.eu
@dherykw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

It seems that linting errors have been successfully fixed :) sorry for the inconvenience.

Pipermerrian changes have been also satisfied.

@pipermerriam pipermerriam merged commit 898a2a6 into ethereum:master Jul 8, 2019

20 checks passed

ci/circleci: py36-benchmark Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py36-database Your tests passed on CircleCI!
Details
ci/circleci: py36-docs Your tests passed on CircleCI!
Details
ci/circleci: py36-lint Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-byzantium Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-constantinople Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-frontier Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-homestead Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-petersburg Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-spurious_dragon Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-tangerine_whistle Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-transition Your tests passed on CircleCI!
Details
ci/circleci: py36-transactions Your tests passed on CircleCI!
Details
ci/circleci: py36-vm Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details
ci/circleci: py37-database Your tests passed on CircleCI!
Details
ci/circleci: py37-lint Your tests passed on CircleCI!
Details
ci/circleci: py37-transactions Your tests passed on CircleCI!
Details
ci/circleci: py37-vm Your tests passed on CircleCI!
Details
@pipermerriam

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Thank you for the contribution!

@dherykw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

My pleasure. I am looking forward continue collaborating. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.