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

BytesParsing - change handling of calldata slicing and checkLength #76

Merged
merged 2 commits into from
Dec 29, 2024

Conversation

nonergodic
Copy link
Collaborator

@nonergodic nonergodic commented Dec 28, 2024

Two changes:

First, the various calldata versions of slice now return bytes calldata instead of bytes memory.

This rectifies a previous mistake of mine where I didn't realize that I could use assembly to implement an unchecked version of slice that does what I want. I figured that creating a bytes memory is efficient enough since it can use CALLDATACOPY but failed to consider that pushing things from the calldata domain to the memory domain makes for ugly and inefficient code if subsequent parsing is necessary.

Second, checkLength now takes the encoded length as the first argument instead of the encoded bytes directly. The rationale is that it's way too easy to write someCalldataBytes.checkLength(expectedLength); instead of using the appropriate checkLengthCd version, which would quietly allocate memory and copy the entire someCalldataBytes just to do the length check. Additionally, it's also more consistent with how checkBound works.

It is now recommended to also have a using {BytesParsing.checkLength} for uint; line at the beginning of the contract (along with using BytesParsing for bytes;) and check via encoded.length.checkLength(expectedLength); independent of whether encoded has memory or calldata location.

@nonergodic nonergodic merged commit 1ec9d4c into main Dec 29, 2024
1 check passed
@nonergodic nonergodic deleted the bytes-parsing-keep-calldata branch December 29, 2024 21:56
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.

2 participants