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

Fix/msg data #2419

Merged
merged 10 commits into from Aug 16, 2021
Merged

Fix/msg data #2419

merged 10 commits into from Aug 16, 2021

Conversation

skellet0r
Copy link
Contributor

What I did

Refactored the msg.data environment variable. Instead of handling each distinctive use case in expr.py, uses are handled in the respective functions which operate on msg.data.

  • Len builtin simply returns calldatasize
  • Slice builtin returns a bytes array containing the sliced portion of calldata
    • The length argument to slice should be a compile time literal as calldatasize is unbounded, and we need a maxsize
      on the return type of the slice function instead of defaulting to the maxsize of msg.data

How I did it

Followed charles' guidance. Mainly just made msg.data return an empty LLLnode with location='calldata', and then handled it in the builtin functions

How to verify it

The test suite for msg.data is unchanged, so passing tests verify this refactoring worked.

Description for the changelog

  • Fix: msg.data memory allocation removed in favor of builtin function handling

Cute Animal Picture

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

Since the length of msg.data is dynamic, we can't say for sure what it's
maxlength is. Even inside of a function with 3 word length arguments,
data can still be appended to a function's calldata. So we make length
a compile time requirement when using msg.data and slice, and we allow
start to be determined at runtime since we don't need it to determine
the size of the byte array.
@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2021

Codecov Report

Merging #2419 (531296e) into master (31dd776) will increase coverage by 0.89%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2419      +/-   ##
==========================================
+ Coverage   84.61%   85.51%   +0.89%     
==========================================
  Files          91       91              
  Lines        9034     9027       -7     
  Branches     2151     2151              
==========================================
+ Hits         7644     7719      +75     
+ Misses        882      804      -78     
+ Partials      508      504       -4     
Impacted Files Coverage Δ
vyper/builtin_functions/functions.py 88.82% <100.00%> (+5.48%) ⬆️
vyper/old_codegen/expr.py 80.11% <100.00%> (+4.12%) ⬆️
vyper/semantics/validation/annotation.py 95.52% <100.00%> (+0.55%) ⬆️
vyper/semantics/validation/local.py 89.92% <100.00%> (-0.08%) ⬇️
vyper/ast/nodes.py 93.58% <0.00%> (+1.46%) ⬆️
vyper/semantics/types/value/numeric.py 86.02% <0.00%> (+5.37%) ⬆️

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 31dd776...531296e. Read the comment docs.

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

Overall looks great, I suggested a couple style/defensive changes.

As an aside, it would be really nice if we could get rid of the copy altogether, and just pass around a pointer to calldata. But that requires some rethinking of how we handle bytestrings (i.e. handling the length section separately from the data section) in general, and at that point we could really revamp slice -- including the case where the bytestring is in memory.

vyper/old_codegen/expr.py Outdated Show resolved Hide resolved
vyper/old_codegen/expr.py Show resolved Hide resolved
vyper/builtin_functions/functions.py Show resolved Hide resolved
vyper/semantics/validation/annotation.py Show resolved Hide resolved
vyper/builtin_functions/functions.py Show resolved Hide resolved
vyper/semantics/validation/local.py Outdated Show resolved Hide resolved
Copy link
Member

@charles-cooper charles-cooper 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. Thanks

@charles-cooper charles-cooper merged commit f1c65b7 into vyperlang:master Aug 16, 2021
@skellet0r skellet0r deleted the fix/msg_data branch August 17, 2021 15:55
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.

None yet

3 participants