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

Refactor ABI encoder #1723

Merged
merged 29 commits into from
Dec 9, 2019
Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Nov 12, 2019

What I did

Refactor the ABI encoder/decoder so that the code is clearer and more extensible. This should make it easier to implement ABI-related features.

There's still a lot to do (ultimately I want to replace our entire calling convention and event generation code with abi_encode/abi_decode, for now it's just returndata with abi_encode) but it's at a good self-contained stage where we can review and move further work into another PR. Tote that abi_decode and lazy_abi_decode are unused right now but they will be useful in the future.

How I did it

See commit messages and inline comments

How to verify it

Tests pass

Description for the changelog

Refactor ABI encoder for return data.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@charles-cooper charles-cooper force-pushed the abi_refactor branch 3 times, most recently from a85571b to 2eaf3d3 Compare November 28, 2019 01:17
define types, utility functions
leaf bytes are encoded as-is (+ padding), tuples with child bytes are
encoded <ofst> <len> <data>.
instead of storing just the pointers themselves.
destination should always be memory otherwise make_setter returns None.
make_return_stmt jumps out of the with statement, leaving the with
variables on the stack (when they shouldn't be).
by using make_setter for its typechecking side effects.
The idea is that in a lot of cases (e.g. decoding calldata), we don't
actually need to copy the data from the abi-encoded buffer into a
destination buffer, but instead we can "leave it where it is" and
instead just generate loader statements for the data.
(if it is not already)
@charles-cooper
Copy link
Member Author

this is ready for a look over. there's still a lot to do (ultimately i want to replace our entire calling convention and event generation code with abi_encode/abi_decode and now it's just generating returndata with abi_encode) but it's at a good self-contained stage where we can review and move further work into another PR. note that abi_decode and lazy_abi_decode are unused right now but they will be .. useful :)

@charles-cooper
Copy link
Member Author

requesting comments from @fubuloubu @jacqueswww and/or @iamdefinitelyahuman

@charles-cooper charles-cooper changed the title WIP ABI refactor Refactor ABI encoder Dec 2, 2019
@charles-cooper charles-cooper marked this pull request as ready for review December 2, 2019 00:20
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Gave it a first pass. Overall, looks very promising.

I would like the use of more descriptive variable names in the codegen modules. Obviously needs a series of test cases. Also, if it's not too much trouble, add typing to it.

vyper/codegen/abi.py Outdated Show resolved Hide resolved
vyper/types/check.py Outdated Show resolved Hide resolved
@fubuloubu
Copy link
Member

And note, by testing I meant for any functionality that's changed, which doesn't appear to be much (yet)

@charles-cooper
Copy link
Member Author

And note, by testing I meant for any functionality that's changed, which doesn't appear to be much (yet)

no functionality has changed (theoretically)

Co-Authored-By: Bryant Eisenbach <3859395+fubuloubu@users.noreply.github.com>
@fubuloubu
Copy link
Member

Merge on CI pass

@fubuloubu fubuloubu merged commit 34a899b into vyperlang:master Dec 9, 2019
This was referenced Jan 24, 2020
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

Successfully merging this pull request may close these issues.

3 participants