Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix issue #450: buggy translation of constant clamp expressions #460

Merged
merged 3 commits into from
Nov 13, 2017

Conversation

DavidKnott
Copy link
Contributor

- What I did

Fixed clamps in compile_lll.py
Added a fixture to make it easier to test LLL
Created LLL clamp tests
In response to issue #450, thanks @daejunpark

- How I did it

Corrected clamp constant checks in compile_lll.py

- How to verify it

Look at the tests I created in tests/compiler/test_clamps.py.

- Description for the changelog

None

- Cute Animal Picture

image

@DavidKnott DavidKnott changed the title Fix clamps Fix issue #450: buggy translation of constant clamp expressions Nov 11, 2017
Copy link
Contributor

@jacqueswww jacqueswww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, only thing one might have to consider is that assert_tx_failed is used in away that doesn't imply 'a transaction failed', instead 'the compiler failed' - just a minor note ;)

lll = optimizer.optimize(LLLnode.from_list(lll))
byte_code = compile_lll.assembly_to_evm(compile_lll.compile_to_assembly(lll))
t.s.tx(to=b'', data=byte_code)
return lll_compiler
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for the fixture :)

@DavidKnott
Copy link
Contributor Author

@jacqueswww I created a fixture called assert_compile_failed which is just syntactic sugar for assert_tx_failed. Though I think this can be done better when we move a lot of setup_transaction_tests.py into conftest.py

@DavidKnott DavidKnott merged commit 17a153c into vyperlang:master Nov 13, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants