[Flow EVM] Fix padding logic on EncodeBytes for data with multiple chunks#8425
[Flow EVM] Fix padding logic on EncodeBytes for data with multiple chunks#8425
EncodeBytes for data with multiple chunks#8425Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughModified ABI encoding logic to add buffer size validation and refactored padding calculation for multi-chunk data encoding. The EncodeBytes function now validates buffer capacity upfront and computes padded size using a chunks-based approach. Corresponding test cases verify buffer underflow errors and correct multi-chunk encoding output. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fvm/evm/precompiles/abi.go (1)
209-218:SizeNeededForBytesEncodingstill usespaddedSizewhere it means chunk count — same naming pattern as the original bug.The fix in
EncodeBytesrenames the intermediate variable tochunksfor clarity, butSizeNeededForBytesEncodingstill usespaddedSizeto hold the chunk count before multiplying byFixedSizeUnitDataReadSize. The result is correct, but the inconsistency between the two functions could confuse future readers.♻️ Suggested rename for consistency
func SizeNeededForBytesEncoding(data []byte) int { if len(data) == 0 { return EncodedUint64Size + EncodedUint64Size + FixedSizeUnitDataReadSize } - paddedSize := (len(data) / FixedSizeUnitDataReadSize) + chunks := (len(data) / FixedSizeUnitDataReadSize) if len(data)%FixedSizeUnitDataReadSize != 0 { - paddedSize += 1 + chunks += 1 } - return EncodedUint64Size + EncodedUint64Size + paddedSize*FixedSizeUnitDataReadSize + return EncodedUint64Size + EncodedUint64Size + chunks*FixedSizeUnitDataReadSize }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/precompiles/abi.go` around lines 209 - 218, SizeNeededForBytesEncoding uses the variable name paddedSize to represent the number of FixedSizeUnitDataReadSize chunks (same concept as in EncodeBytes), which is inconsistent and confusing; rename paddedSize to chunks in SizeNeededForBytesEncoding and update its calculation and use (the return expression that multiplies by FixedSizeUnitDataReadSize) to match EncodeBytes' naming for clarity and maintainability while keeping the existing logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fvm/evm/precompiles/abi.go`:
- Around line 209-218: SizeNeededForBytesEncoding uses the variable name
paddedSize to represent the number of FixedSizeUnitDataReadSize chunks (same
concept as in EncodeBytes), which is inconsistent and confusing; rename
paddedSize to chunks in SizeNeededForBytesEncoding and update its calculation
and use (the return expression that multiplies by FixedSizeUnitDataReadSize) to
match EncodeBytes' naming for clarity and maintainability while keeping the
existing logic intact.
fxamacker
left a comment
fxamacker
left a comment
There was a problem hiding this comment.
Nice! I left some comments and suggestions.
| if len(data) == 0 { | ||
| return EncodedUint64Size + EncodedUint64Size + FixedSizeUnitDataReadSize | ||
| } |
There was a problem hiding this comment.
This PR doesn't modify SizeNeededForBytesEncoding function.
Unless I missed something, it looks like empty data should be encoded to 64 bytes (instead of 96 bytes).
encoded empty data = encoded offset (32 bytes) +
encoded data length (32 bytes) +
data (0 bytes because data is empty and there is no padding)
| chunks := (dataSize / FixedSizeUnitDataReadSize) | ||
| if dataSize%FixedSizeUnitDataReadSize != 0 { | ||
| paddedSize += FixedSizeUnitDataReadSize | ||
| chunks += 1 | ||
| } | ||
| paddedSize := chunks * FixedSizeUnitDataReadSize |
There was a problem hiding this comment.
Maybe extract computation of paddedSize into its own function.
SizeNeededForBytesEncoding() also has logic to compute padded size. So we can reuse the extracted function in both SizeNeededForBytesEncoding() and EncodeBytes().
| if len(buffer) < payloadIndex+EncodedUint64Size+paddedSize { | ||
| return ErrBufferTooSmall | ||
| } |
There was a problem hiding this comment.
This check can be removed since we check for buffer size against encoded data at the beginning of the function.
| if len(buffer) < SizeNeededForBytesEncoding(data) { | ||
| return ErrBufferTooSmall | ||
| } |
There was a problem hiding this comment.
Maybe include headerIndex here in case headerIndex isn't 0.
| if len(buffer) < SizeNeededForBytesEncoding(data) { | |
| return ErrBufferTooSmall | |
| } | |
| if len(buffer) < headerIndex + SizeNeededForBytesEncoding(data) { | |
| return ErrBufferTooSmall | |
| } |
| if len(buffer) < headerIndex+EncodedUint64Size { | ||
| return ErrBufferTooSmall | ||
| } |
There was a problem hiding this comment.
I think this check can be removed because it is redundant with previous check len(buffer) < headerIndex + SizeNeededForBytesEncoding(data).
4 participants
bufferhas the necessary size to fit the encoded dataSummary by CodeRabbit
Release Notes
Bug Fixes
Tests