Skip to content

execution/abi: fix regex submatch for parsing variable size in types#20823

Merged
yperbasis merged 3 commits intoerigontech:mainfrom
Sahil-4555:fix-abi-variable-size-parsing
Apr 28, 2026
Merged

execution/abi: fix regex submatch for parsing variable size in types#20823
yperbasis merged 3 commits intoerigontech:mainfrom
Sahil-4555:fix-abi-variable-size-parsing

Conversation

@Sahil-4555
Copy link
Copy Markdown
Contributor

This request fixes a parsing issue in the ABI package that occurs when evaluating types with multi-dimensional sizes. Previously, the regular expression used the wrong capture group when parsing the type string. For types such as fixed128x18, the code incorrectly captured the entire suffix "128x18" instead of just the leading number "128". This caused an invalid syntax error when the application attempted to parse that combined string as an integer.

By updating the regex submatch index, the parser now correctly isolates the first numeric value so it can be evaluated properly. Along with this fix, a regression test has been added to ensure that unsupported fixed-point types correctly return an "unsupported arg type" error rather than failing unexpectedly during the size calculation process.

@Sahil-4555 Sahil-4555 force-pushed the fix-abi-variable-size-parsing branch from 69a3276 to 418d230 Compare April 26, 2026 13:57
@yperbasis yperbasis added this to the 3.6.0 milestone Apr 26, 2026
@Sahil-4555 Sahil-4555 force-pushed the fix-abi-variable-size-parsing branch from 418d230 to 8df4c44 Compare April 27, 2026 03:28
@AskAlexSharov AskAlexSharov requested a review from Copilot April 27, 2026 03:42
@AskAlexSharov AskAlexSharov enabled auto-merge April 27, 2026 03:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes ABI type parsing for fixed-point-like type strings where the regex submatch used to compute the first numeric size was incorrect, preventing the intended “unsupported arg type” path from being reached.

Changes:

  • Correct NewType to parse the first numeric size from parsedType[3] (not parsedType[2]) when the matched suffix can include an x... component.
  • Add a regression test covering fixed128x18 / ufixed256x10 to ensure the bug doesn’t reappear.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
execution/abi/type.go Fixes the submatch index used for varSize parsing in NewType.
execution/abi/type_test.go Adds a regression test to guard against the prior size-parsing failure mode for fixed-point-like types.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread execution/abi/type_test.go Outdated
Copy link
Copy Markdown
Member

@yperbasis yperbasis left a comment

Choose a reason for hiding this comment

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

Copilot already flagged this and I agree: the regression test only asserts the error string does not contain "error parsing variable size". That's a weak negative assertion — any
other failure mode (e.g. an "invalid type" error from a future regex change) would still pass the test.

The simplest tightening matches what the PR description claims:

if !strings.Contains(err.Error(), "unsupported arg type") {
t.Fatalf("type %q: got %v; want error containing %q", s, err, "unsupported arg type")
}

auto-merge was automatically disabled April 28, 2026 09:55

Head branch was pushed to by a user without write access

@Sahil-4555 Sahil-4555 force-pushed the fix-abi-variable-size-parsing branch from 63b5948 to 799a959 Compare April 28, 2026 09:55
@yperbasis yperbasis enabled auto-merge April 28, 2026 12:46
@yperbasis yperbasis added this pull request to the merge queue Apr 28, 2026
Merged via the queue into erigontech:main with commit c425787 Apr 28, 2026
35 of 36 checks passed
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.

3 participants