change asset token definition #35
Conversation
WalkthroughThe changes update the Numscript grammar by revising the ASSET token definition to allow an optional numeric suffix. Lexer ATN data has been updated in the interpreter and Go lexer files to reflect new transitions and state changes that align with the grammar update. Additionally, new test functions have been added to validate that the parser correctly identifies and handles both valid and invalid asset names. Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Function
participant P as Parser
participant L as Lexer
T->>P: Submit input containing an ASSET token
P->>L: Tokenize input using updated ASSET rule
L-->>P: Return tokens with optional numeric part (if present)
P-->>T: Return error if asset name format is invalid
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 63.19% 63.87% +0.67%
==========================================
Files 29 29
Lines 6529 6651 +122
==========================================
+ Hits 4126 4248 +122
Misses 2217 2217
Partials 186 186 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Numscript.g4 (1)
43-43: LGTM! Consider adding documentation for the ASSET token format.The new pattern correctly restricts asset names to uppercase letters with an optional numeric suffix, preventing invalid formats like "/", "EUR/", and "/42". This is a breaking but necessary change.
Consider adding a comment above the ASSET token to document the expected format:
+// ASSET: Uppercase letters followed by an optional numeric suffix (e.g., EUR, USD/2) ASSET: [A-Z]+ ('/' [0-9]+)?;internal/parser/parser_test.go (1)
341-351: Consider adding more test cases.The test covers basic invalid cases. Consider adding more edge cases:
-for _, name := range []string{"", "/", "EUR/", "/42"} { +for _, name := range []string{ + "", "/", "EUR/", "/42", + "123", "EUR/ABC", "EUR//2", + "eur", "Eur", "EUR/0", + "EUR/01", "EUR/-1" +} {This would validate:
- Numeric-only names
- Invalid suffix formats
- Mixed case letters
- Leading zeros and negative numbers in suffix
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Numscript.g4(1 hunks)internal/parser/antlr/NumscriptLexer.interp(1 hunks)internal/parser/antlr/numscript_lexer.go(2 hunks)internal/parser/parser_test.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/parser/parser_test.go (1)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/antlr/numscript_parser.go:2786-2850
Timestamp: 2025-01-20T22:55:14.279Z
Learning: Do not suggest adding tests or improving test coverage for auto-generated code, especially files with headers indicating they are generated (e.g., "Code generated from X by Y. DO NOT EDIT.").
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
internal/parser/antlr/NumscriptLexer.interp (1)
127-127: Auto-generated file, no review needed.internal/parser/antlr/numscript_lexer.go (1)
1-305: Auto-generated file, no review needed.
Numscript.g4
Outdated
| @@ -40,7 +40,7 @@ IDENTIFIER: [a-z]+ [a-z_]*; | |||
| NUMBER: MINUS? [0-9]+ ('_' [0-9]+)*; | |||
| VARIABLE_NAME: '$' [a-z_]+ [a-z0-9_]*; | |||
| ACCOUNT: '@' [a-zA-Z0-9_-]+ (':' [a-zA-Z0-9_-]+)*; | |||
| ASSET: [A-Z/0-9]+; | |||
| ASSET: [A-Z]+ ('/' [0-9]+)?; | |||
There was a problem hiding this comment.
Is [A-Z]+ specific enough or should we maybe match exactly 3 with {3}. Not sure if currency is ever more or less than 3 (maybe user defined ones?)
There was a problem hiding this comment.
you mean the asset's length? Yeah It may be an arbitrary asset like GEM, COIN, etc
Potentially also something like some non fungible asset like TKEA2BC34EE0FC4E3DAD46874762279B13 - which I just realized we cannot express anymore after this commit. I already asked @altitude if this grammar change was correct but I'll double check explicitly this
There was a problem hiding this comment.
UPDATE: looking at the ledger's checks, I've relaxed this constraint so that assets can have numbers too (just not start with a number)
This is still more relaxed than ledger's (variables) checks, which allow a max of 16 chars for asset and 6 chars for the "/" part.
ledger's regex: [A-Z][A-Z0-9]{0,16}(/\d{1,6})?
@gfyrag @altitude should we lower down the characters numbers (this is a breaking change for numscript) or relax ledger's regex?
There was a problem hiding this comment.
The format used by the ledger and by the original machine is [A-Z][A-Z0-9]{0,16}(/\d{1,6})?
I guess Numscript should be aligned on this format.
There was a problem hiding this comment.
I realize the grammar allow ASSET: [A-Z/0-9]+;
But the code check the format at execution and check for the format [A-Z][A-Z0-9]{0,16}(/\d{1,6})?.
ascandone
force-pushed
the
fix/fix-asset-grammar
branch
from
ed13f3b to
9dab605
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/parser/parser_test.go (1)
349-359: LGTM! Consider adding more test cases.The test function effectively validates the parser's error handling for invalid asset names using a table-driven test pattern.
Consider adding these test cases to improve coverage:
- Asset names with multiple slashes (e.g., "EUR/42/10")
- Asset names with non-numeric suffix (e.g., "EUR/ABC")
- Asset names with leading/trailing spaces
- Asset names with lowercase letters (e.g., "eur/42")
Apply this diff to add more test cases:
- for _, name := range []string{"", "/", "EUR/", "/42"} { + for _, name := range []string{ + "", "/", "EUR/", "/42", + "EUR/42/10", // Multiple slashes + "EUR/ABC", // Non-numeric suffix + " EUR/42", // Leading space + "EUR/42 ", // Trailing space + "eur/42", // Lowercase letters + } {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Numscript.g4(1 hunks)internal/parser/antlr/NumscriptLexer.interp(1 hunks)internal/parser/antlr/numscript_lexer.go(2 hunks)internal/parser/parser_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Numscript.g4
- internal/parser/antlr/NumscriptLexer.interp
🧰 Additional context used
📓 Learnings (1)
internal/parser/parser_test.go (1)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/antlr/numscript_parser.go:2786-2850
Timestamp: 2025-01-20T22:55:14.279Z
Learning: Do not suggest adding tests or improving test coverage for auto-generated code, especially files with headers indicating they are generated (e.g., "Code generated from X by Y. DO NOT EDIT.").
🔇 Additional comments (1)
internal/parser/antlr/numscript_lexer.go (1)
69-104: LGTM! Auto-generated lexer changes.The changes to the serializedATN array correctly reflect the updated ASSET token definition from the Numscript grammar.
ascandone
force-pushed
the
fix/fix-asset-grammar
branch
from
9dab605 to
f039c11
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/parser/parser_test.go (1)
349-359: LGTM! Consider enhancing error assertions.The test function effectively validates that invalid asset names are rejected. Consider improving the assertions with descriptive error messages:
- require.NotEmpty(t, p.Errors) + require.NotEmpty(t, p.Errors, fmt.Sprintf("Expected parsing error for invalid asset name: %q", name))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Numscript.g4(1 hunks)internal/parser/antlr/NumscriptLexer.interp(1 hunks)internal/parser/antlr/numscript_lexer.go(2 hunks)internal/parser/parser_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Numscript.g4
- internal/parser/antlr/NumscriptLexer.interp
- internal/parser/antlr/numscript_lexer.go
🧰 Additional context used
🧠 Learnings (1)
internal/parser/parser_test.go (1)
Learnt from: ascandone
PR: formancehq/numscript#31
File: internal/parser/antlr/numscript_parser.go:2786-2850
Timestamp: 2025-01-20T22:55:14.279Z
Learning: Do not suggest adding tests or improving test coverage for auto-generated code, especially files with headers indicating they are generated (e.g., "Code generated from X by Y. DO NOT EDIT.").
🔇 Additional comments (2)
internal/parser/parser_test.go (2)
4-4: LGTM!The
fmtimport is correctly added to support string formatting in the new test functions.
361-371: LGTM! Consider adding edge cases.The test function effectively validates that valid asset names are accepted. Consider adding test cases for edge cases:
- Are there limits on asset name length?
- Are there restrictions on the numeric suffix range?
Run this script to check for any existing constraints:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for asset name length or numeric suffix constraints # Search for length constraints rg -A 5 'ASSET|asset.*length' # Search for numeric suffix validation rg -A 5 'ASSET|asset.*[0-9]|suffix'Length of output: 16383
Edge Case Tests for Asset Names
The asset name rule is defined inNumscript.g4as follows:ASSET: [A-Z][A-Z0-9]* ('/' [0-9]+)?;
- Asset Name Length: There’s no explicit maximum length restriction enforced by the grammar.
- Numeric Suffix Range: The optional suffix is defined by the simple regex
[0-9]+(and later converted viabig.Int), so there’s no range check beyond ensuring it is a valid number.The current test in
internal/parser/parser_test.govalidates several valid asset names, and additional tests for edge cases (like extremely long names or numeric suffix limits) would only be necessary if you plan to introduce further restrictions later on.
this PR narrows the available asset values, forbidding assets like "/", "EUR/", or "/42"
note this is a breaking change (but the old behaviour was not intented and therefore not documented)