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.

Sign up for GitHub

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

Jump to bottom

Adjust regex to handle square brackets #774

Merged
merged 3 commits into from
Jun 6, 2024
Merged

Adjust regex to handle square brackets #774

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eadaa8b
adjust regex for function and event names
zoeyTM Jun 4, 2024
2c34ad9
update regex
zoeyTM Jun 5, 2024
4ac475e
test: tweak to regex
kanej Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/core/src/internal/utils/identifier-validators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ const solidityIdentifierRegex = /^[a-zA-Z$_][a-zA-Z0-9$_]*$/;
/**
* A regex capturing the solidity identifier rule but extended to support
* the `myfun(uint256,bool)` parameter syntax
*
* This *could* be even stricter, but it works for now and covers obvious mistakes
*/
const functionNameRegex = /^[a-zA-Z$_][a-zA-Z0-9$_,()]*$/;
const functionNameRegex = /^[a-zA-Z0-9$_]*(\([a-zA-Z0-9$_,\[\]]*\))?$/;
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch.
Are there any backwards compatibility concerns here?

Copy link
Member

Choose a reason for hiding this comment

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

Should the * be a +?
Would the following incorrectly pass (uint256)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the * be a +?

Ah that's a great catch, I just pushed an update to fix that (with a new test case to verify)

Are there any backwards compatibility concerns here?

I don't believe so. The biggest potential impacts I was aware of while writing this were:

  1. The rightmost * needs to stay an * and not be changed to a +. This change would break myFunc() which currently works
  2. Due to how the regex was previously written, 0-9 was allowed to be in the function name, which looks unintentional, but is actually correct as that is valid solidity. I added 0-9 to the first character set to continue this behavior.

I believe these are the only two potential breaking changes of things that should work before and after the change. This technically has a breaking change in that new invalid patterns will fail, but they would have failed later in the code anyways previously, so I don't believe this is actually a breaking change. They just properly throw where intended now.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I follow now. The move is all about reworking the parameter list.
We support [] in the parameter list and this is allows additional valid identifiers, so no breaking change there - agreed.
I sharpened up the first part in firstpart(secondparts[]), so that the first character can't be a number: https://docs.soliditylang.org/en/v0.8.26/grammar.html#a4.SolidityLexer.Identifier.
It could be argued that we could sharpen up the parameter list as well, but I am happy with the current level of precision. What do you think?


/**
* Does the identifier match Ignition's rules for ids. Specifically that they
Expand Down
21 changes: 21 additions & 0 deletions packages/core/test/validations/identifier-validators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { assert } from "chai";

import { isValidFunctionOrEventName } from "../../src/internal/utils/identifier-validators";

describe("isValidFunctionOrEventName", () => {
it("should return true for valid solidity function names", () => {
assert.isTrue(isValidFunctionOrEventName("myFunction"));
assert.isTrue(isValidFunctionOrEventName("myFunction()"));
assert.isTrue(isValidFunctionOrEventName("myFunction(uint256)"));
assert.isTrue(isValidFunctionOrEventName("myFunction(uint256)"));
assert.isTrue(isValidFunctionOrEventName("myFunction(uint256,bool)"));
assert.isTrue(isValidFunctionOrEventName("myFunction(uint256[],bool)"));
});

it("should return false for invalid solidity function names", () => {
assert.isFalse(isValidFunctionOrEventName("myFunction("));
assert.isFalse(isValidFunctionOrEventName("myFunction(uint)256"));
assert.isFalse(isValidFunctionOrEventName("myFunction(uint256"));
assert.isFalse(isValidFunctionOrEventName("myFunctionuint256)"));
});
});
Loading