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

allow any ABI-encodable type as args #2398

Closed
wants to merge 11 commits into from

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Jul 27, 2021

What I did

Among other things, fixes #2154 and #2190

How I did it

I replaced the ad-hoc code in pack_arguments with a call to the generic abi_encode function.

How to verify it

Tests pass .. we might need some more tests now though

Description for the changelog

Allow any ABI-encodable type as args, including nested structs and structs with dynamic data

Cute Animal Picture

image

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2021

Codecov Report

Merging #2398 (db573a8) into master (cff69d6) will decrease coverage by 19.23%.
The diff coverage is 83.14%.

❗ Current head db573a8 differs from pull request most recent head d76c56f. Consider uploading reports for the commit d76c56f to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2398       +/-   ##
===========================================
- Coverage   85.73%   66.50%   -19.24%     
===========================================
  Files          91       91               
  Lines        9101     8993      -108     
  Branches     2176     2139       -37     
===========================================
- Hits         7803     5981     -1822     
- Misses        796     2301     +1505     
- Partials      502      711      +209     
Impacted Files Coverage Δ
vyper/semantics/namespace.py 79.68% <ø> (-17.19%) ⬇️
vyper/semantics/types/function.py 66.07% <ø> (-19.95%) ⬇️
vyper/builtin_functions/functions.py 76.27% <66.66%> (-12.43%) ⬇️
vyper/old_codegen/parser_utils.py 65.80% <72.41%> (-12.93%) ⬇️
vyper/old_codegen/keccak256_helper.py 78.57% <73.33%> (-0.38%) ⬇️
vyper/old_codegen/events.py 93.33% <92.59%> (-5.88%) ⬇️
vyper/old_codegen/abi.py 69.04% <100.00%> (-4.84%) ⬇️
vyper/old_codegen/stmt.py 64.01% <100.00%> (-20.49%) ⬇️
vyper/semantics/types/user/event.py 78.33% <100.00%> (+0.74%) ⬆️
vyper/ast/natspec.py 12.34% <0.00%> (-86.42%) ⬇️
... and 58 more

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 cff69d6...d76c56f. Read the comment docs.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 27, 2021

This pull request introduces 1 alert when merging d92d73c into cbc9e65 - view on LGTM.com

new alerts:

  • 1 for Unused import

@charles-cooper charles-cooper marked this pull request as ready for review July 27, 2021 22:51
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.

Add some test cases showing that the functionality works

vyper/old_codegen/parser_utils.py Outdated Show resolved Hide resolved
@charles-cooper
Copy link
Member Author

Add some test cases showing that the functionality works

@fubuloubu blocked on #2394

@charles-cooper
Copy link
Member Author

This PR corrects the packing code but the unpacking code is still incomplete. (Meaning you could call out to a function which accepts nested structs but you couldn't implement one).

I'm going to work on this more. I think I need to do a larger refactor around our internal calling convention otherwise this functionality will result in really bloated code.

@charles-cooper charles-cooper marked this pull request as draft July 30, 2021 16:37
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 3, 2021

This pull request introduces 12 alerts when merging d76c56f into cff69d6 - view on LGTM.com

new alerts:

  • 8 for Unused import
  • 1 for First parameter of a method is not named 'self'
  • 1 for Constant in conditional expression or statement
  • 1 for Unused local variable
  • 1 for Unreachable code

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 17, 2021

This pull request introduces 12 alerts when merging ce916c4 into ed59064 - view on LGTM.com

new alerts:

  • 8 for Unused import
  • 1 for First parameter of a method is not named 'self'
  • 1 for Constant in conditional expression or statement
  • 1 for Unused local variable
  • 1 for Unreachable code

@charles-cooper
Copy link
Member Author

superseded by #2447

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.

[Compiler Error] vyper.exceptions.TypeCheckFailure: pack_arguments did not return a value
3 participants