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.

Sign up for GitHub

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

Jump to bottom

feat: add abi_decode builtin #2882

Merged
merged 62 commits into from
Jun 8, 2022
Merged

feat: add abi_decode builtin #2882

merged 62 commits into from
Jun 8, 2022

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented May 29, 2022 •
edited by charles-cooper
Loading

What I did

Add _abi_decode builtin.

  • Added TupleDefinition to get_type_from_annotation

How I did it

Add _abi_decode to builtin functions module.

How to verify it

See new tests

Commit message

feat: abi_decode builtin

add `_abi_decode` builtin for decoding ABIv2 values from a byte array.
to keep typechecking more strict, `_abi_decode` takes as the second
argument, a type to decode into. this also required a couple minor
changes to the grammar and type annotator.
       
this commit also slightly optimizes the `clamp2` utility routine.

Description for the changelog

Add _abi_decode builtin.

Cute Animal Picture

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

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2022 •
edited
Loading

Codecov Report

Merging #2882 (ca856df) into master (ac8cb6e) will increase coverage by 0.08%.
The diff coverage is 95.77%.

❗ Current head ca856df differs from pull request most recent head fdcd775. Consider uploading reports for the commit fdcd775 to get more accurate results

@@            Coverage Diff             @@
##           master    #2882      +/-   ##
==========================================
+ Coverage   87.96%   88.04%   +0.08%     
==========================================
  Files          95       95              
  Lines       10389    10435      +46     
  Branches     2502     2511       +9     
==========================================
+ Hits         9139     9188      +49     
+ Misses        794      790       -4     
- Partials      456      457       +1
Impacted Files Coverage Δ
vyper/semantics/types/utils.py 88.80% <88.88%> (+<0.01%) ⬆️
vyper/builtin_functions/functions.py 91.98% <96.15%> (+0.30%) ⬆️
vyper/builtin_functions/signatures.py 90.00% <100.00%> (+1.36%) ⬆️
vyper/codegen/core.py 84.46% <100.00%> (ø)
vyper/semantics/validation/annotation.py 89.50% <100.00%> (+0.11%) ⬆️
vyper/abi_types.py 82.73% <0.00%> (+1.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac8cb6e...fdcd775. Read the comment docs.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 29, 2022

This pull request introduces 1 alert when merging 893cd1e into dcf65b6 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 30, 2022

This pull request introduces 1 alert when merging 769a4b7 into 687ef4a - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@charles-cooper charles-cooper marked this pull request as ready for review June 2, 2022 16:17
@charles-cooper
Copy link
Member

we also need a unwrap_tuple=True kwarg. it is the inverse of ensure_tuple=True in _abi_encode().

@charles-cooper @fubuloubu
fix signatures in docs
1097b83 
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jun 8, 2022

This pull request introduces 1 alert when merging 1097b83 into 6b4d8ff - view on LGTM.com

new alerts:

  • 1 for Unused import

charles-cooper
charles-cooper reviewed Jun 8, 2022
View reviewed changes
tests/functional/codegen/test_abi_decode.py Outdated
"len,output_typ,input",
[
(160, "DynArray[uint256, 3]", b""),
(160, "DynArray[uint256, 3]", b"\x01" * 192),
Copy link
Member

Choose a reason for hiding this comment

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

hmm, i think this fails because of the input arg clamper, not because of the clamper in abi_decode

Copy link
Member

Choose a reason for hiding this comment

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

but maybe we can't actually hit the upper bound clamper?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, and because the typechecker prevents the input arg from exceeding the upper bound as well?

Copy link
Member

Choose a reason for hiding this comment

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

right, because we already check bytearray_len <= 160 when we deserialize the function args

charles-cooper
charles-cooper reviewed Jun 8, 2022
View reviewed changes
tests/functional/codegen/test_abi_decode.py Outdated
@pytest.mark.parametrize(
"len,output_typ1,output_typ2,input",
[
(192, "DynArray[uint256, 3]", "uint256", b""),
(192, "DynArray[uint256, 3]", "int128", b"\x01" * 224),
Copy link
Member

Choose a reason for hiding this comment

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

i think we should still have these test cases

charles-cooper
charles-cooper reviewed Jun 8, 2022
View reviewed changes
tests/functional/codegen/test_abi_decode.py Outdated
(160, "DynArray[uint256, 3]", b""),
(160, "DynArray[uint256, 3]", b"\x01" * 192),
(96, "Bytes[5]", b""),
(96, "Bytes[5]", b"\x01" * 128),
Copy link
Member

Choose a reason for hiding this comment

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

this now fails for the wrong reason - 128 > 96.

@charles-cooper
fix: make clamper tests fail for the right reason
fce0225 
fail at decoding, not argument deserialization

also rename some variables which shadow python builtins:
    len -> len_, input -> input_
@charles-cooper
Merge branch 'master' into feat/abi_decode
fdcd775
@charles-cooper charles-cooper enabled auto-merge (squash) June 8, 2022 20:48
@charles-cooper charles-cooper merged commit f52b811 into vyperlang:master Jun 8, 2022
@charles-cooper charles-cooper mentioned this pull request Jun 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fubuloubu fubuloubu fubuloubu approved these changes

@charles-cooper charles-cooper charles-cooper approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tserg @codecov-commenter @charles-cooper @fubuloubu