Conversation
m-Peter
requested review from
fxamacker,
janezpodhostnik,
joshuahannan,
turbolent and
zhangchiqing
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughTightens EVM address parsing to require a true Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
fvm/evm/evm_test.go (1)
5011-5016: Dead Cadenceassertin error-path test cases.In all three error-path sub-tests (
non-hex prefix,insufficient length,superfluous length), the Cadence script callsEVM.addressFromString(…)which panics before returning, so the subsequentassert(address.toString() == …)is unreachable dead code. The Go-side assertions (require.Error/require.ErrorContains) are the real validation and are correct. Consider removing the deadassertto avoid misleading readers.♻️ Example: clean up the unreachable assert (shown for lines 5011–5016)
access(all) fun main(): Bool { let address = EVM.addressFromString("2xE62340807933BCFC3b89FE121dDC0ae5DA9599a0") - assert( - address.toString() == "e62340807933bcfc3b89fe121ddc0ae5da9599a0", - message: "unexpected EVM address" - ) return true }Apply the same pattern to the other two error-path Cadence scripts.
Also applies to: 5050-5055, 5088-5093
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/evm_test.go` around lines 5011 - 5016, The Cadence error-path sub-tests call EVM.addressFromString(...) which panics, so the subsequent assert(address.toString() == "...") lines are unreachable and should be removed; update the three failing Cadence snippets (the ones that call EVM.addressFromString with non-hex prefix, insufficient length, and superfluous length) by deleting the assert(address.toString() == "...") and its message so the script only invokes EVM.addressFromString and lets the Go-side require.Error / require.ErrorContains validate the panic, leaving the rest of the test logic intact.fvm/evm/stdlib/contract.cdc (1)
243-244: Optional: align panic message format with existing pre-condition messages.The existing pre-condition message (line 241) includes the function name, actual value, and expected range. The new
panicmessage is terser and provides no context about the actual prefix characters.💬 Suggested more informative message
- if asHex.length == 42 && !(asHex[0] == "0" && asHex[1] == "x") { - panic("Invalid string with non-hex prefix") + if asHex.length == 42 && !(asHex[0] == "0" && asHex[1] == "x") { + panic("EVM.addressFromString(): Invalid string with non-hex prefix. Expected \"0x\", got \"".concat(asHex.slice(from: 0, upTo: 2)).concat("\"")) }Note: if you adopt this, the corresponding
require.ErrorContainssubstring inevm_test.gowould need to be updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract.cdc` around lines 243 - 244, The panic message in the conditional that checks asHex length/prefix is too terse; update the panic in the block that checks `if asHex.length == 42 && !(asHex[0] == "0" && asHex[1] == "x")` to mirror the existing precondition style (include the function/operation name, the actual `asHex` value or prefix, and the expected prefix "0x"), e.g. a descriptive message like "<function/operation> invalid prefix: got '<actual prefix>' for value '<asHex>'; expected '0x'"; also update the corresponding `require.ErrorContains` expectation in `evm_test.go` to match the new substring.
🤖 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/evm_test.go`:
- Around line 5011-5016: The Cadence error-path sub-tests call
EVM.addressFromString(...) which panics, so the subsequent
assert(address.toString() == "...") lines are unreachable and should be removed;
update the three failing Cadence snippets (the ones that call
EVM.addressFromString with non-hex prefix, insufficient length, and superfluous
length) by deleting the assert(address.toString() == "...") and its message so
the script only invokes EVM.addressFromString and lets the Go-side require.Error
/ require.ErrorContains validate the panic, leaving the rest of the test logic
intact.
In `@fvm/evm/stdlib/contract.cdc`:
- Around line 243-244: The panic message in the conditional that checks asHex
length/prefix is too terse; update the panic in the block that checks `if
asHex.length == 42 && !(asHex[0] == "0" && asHex[1] == "x")` to mirror the
existing precondition style (include the function/operation name, the actual
`asHex` value or prefix, and the expected prefix "0x"), e.g. a descriptive
message like "<function/operation> invalid prefix: got '<actual prefix>' for
value '<asHex>'; expected '0x'"; also update the corresponding
`require.ErrorContains` expectation in `evm_test.go` to match the new substring.
EVM addresses from StringEVM addresses from String
joshuahannan
approved these changes
Feb 20, 2026
vishalchangrani
approved these changes
Feb 20, 2026
fxamacker
approved these changes
Feb 20, 2026
zhangchiqing
approved these changes
Feb 20, 2026
turbolent
reviewed
Feb 20, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fvm/evm/stdlib/contract.cdc`:
- Around line 246-249: The assert for checking the string prefix (the asHex[0]
== "0" && asHex[1] == "x" check) uses the vague message "Invalid string with
non-hex prefix"; update it to follow the file-wide convention by prefixing the
message with the fully-qualified function name (e.g., "EVM.addressFromString():
") and include the expected prefix ("0x") and actual value context so callers
can identify the failure; locate the assertion in contract.cdc near the
addressFromString function and replace the message accordingly.
80a177d to
3abd6ea
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fvm/evm/evm_test.go (1)
4922-5112: Add a test case for valid-length strings containing non-hex characters.The current suite covers length errors and prefix errors, but there is no test for a 40- or 42-character string that satisfies the length and prefix checks yet contains non-hex characters (e.g.,
"0xGGGGGGGGGGGGGGGGGGGGGGGGGGGGGGGGGGGGGGGG"). Such input would pass the newasHex[0] == "0" && asHex[1] == "x"guard and the length precondition, but fail insidedecodeHex()with a generic runtime error rather than a descriptiveaddressFromStringmessage. A test case for this path documents the current behaviour and would catch any future regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/evm_test.go` around lines 4922 - 5112, Add a new subtest inside TestEVMaddressFromString that supplies a 40- or 42-character input containing non-hex characters (e.g., "0xGGGG...") to exercise EVM.addressFromString's path where length/prefix checks pass but decodeHex fails; in that subtest call vm.Run like the others, require output.Err is an error, and use require.ErrorContains to check the error message includes a hex-related substring (e.g., "hex" or "invalid") to document the current decodeHex failure behavior and catch regressions; reference TestEVMaddressFromString, EVM.addressFromString and decodeHex when locating where to add this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fvm/evm/stdlib/contract.cdc`:
- Around line 246-249: Update the assert message in EVM.addressFromString() to
follow the file-wide convention: include the fully-qualified function name and a
clear expected value. Replace the current message "Invalid string with non-hex
prefix" with something like "EVM.addressFromString(): Invalid hex string prefix
— expected '0x'". Ensure this change is made for the assert that checks asHex[0]
== "0" && asHex[1] == "x" inside the addressFromString function.
---
Nitpick comments:
In `@fvm/evm/evm_test.go`:
- Around line 4922-5112: Add a new subtest inside TestEVMaddressFromString that
supplies a 40- or 42-character input containing non-hex characters (e.g.,
"0xGGGG...") to exercise EVM.addressFromString's path where length/prefix checks
pass but decodeHex fails; in that subtest call vm.Run like the others, require
output.Err is an error, and use require.ErrorContains to check the error message
includes a hex-related substring (e.g., "hex" or "invalid") to document the
current decodeHex failure behavior and catch regressions; reference
TestEVMaddressFromString, EVM.addressFromString and decodeHex when locating
where to add this case.
m-Peter
force-pushed
the
mpeter/evm-address-parsing-add-hex-prefix-check
branch
from
3abd6ea to
2f73b54
Compare
This was referenced Feb 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #8436
Summary by CodeRabbit
Improvements
Tests