-
-
Notifications
You must be signed in to change notification settings - Fork 788
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
Corrected abi_decode function, used it when compiling function calls and changed how we were handling the tuple/struct return #2164
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2164 +/- ##
==========================================
+ Coverage 84.94% 85.18% +0.24%
==========================================
Files 83 83
Lines 8334 8337 +3
Branches 2013 2012 -1
==========================================
+ Hits 7079 7102 +23
+ Misses 753 733 -20
Partials 502 502
Continue to review full report at Codecov.
|
This pull request introduces 3 alerts and fixes 1 when merging aacff09 into 39cbd40 - view on LGTM.com new alerts:
fixed alerts:
|
80f416b
to
2769955
Compare
This pull request fixes 1 alert when merging 2769955 into 39cbd40 - view on LGTM.com fixed alerts:
|
@charles-cooper this mucks with your abi generator code, can you take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saikat041 I think this is a good approach. Thanks for fixing some things and simplifying the code structure. Since this modifies the return format for some kinds of things, I think we could do with some more tests though. Would you be able to add some tests? I think, specifically, we want to test nested dynamic data. For instance,
struct X1:
a: uint256
x1: Bytes[10]
struct Y1:
a: uint256
y1: X1
This should be tested for: external call, self-call to public, self-call to private. We also might want to test bytes with len > 31 because those cases are sometimes handled differently. Maybe @fubuloubu can help with this.
Also, could you please change the title of the pull request to summarize what the PR does? Thanks again!!!
There was a problem hiding this 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, I appreciate the comments.
Please add like 10ish new cases as @charles-cooper described, then we are good to go here!
This pull request fixes 1 alert when merging 69fa68e into 39cbd40 - view on LGTM.com fixed alerts:
|
@fubuloubu @charles-cooper I have added more test cases for internal/external calls returning struct, But the test cases for nested structs are failing(as before). They are failing because of this function: Line 267 in 39cbd40
The corrected version should be something like this:
We are using the above function for calculating offsets of keys in a normal struct but there should be a different function for that. Because normal structs in memory are not stored in abi-encoded form. vyper/vyper/parser/parser_utils.py Line 323 in 39cbd40
This function is being called in some other places and also being tested in some individual test cases. We have to make changes in those places too if we want to change this function. I think there should be a separate issue and pull request for changing this get_size_of_type function as this pull request is already making a lot of changes. And also nested structs were failing before this pull request. |
What I did
I fixed the issue: #2155
How I did it
vyper/vyper/codegen/abi.py
Line 408 in 39cbd40
vyper/vyper/parser/parser_utils.py
Line 713 in 39cbd40
vyper/vyper/codegen/return_.py
Line 67 in 39cbd40
How to verify it
Run the following test case:
Description for the changelog