Color of money base implementation #55
Conversation
WalkthroughThis change introduces the concept of "asset colors" into the system, allowing assets to be associated with an optional color attribute via a new grammar construct. The lexer and parser grammars are extended to recognize a backslash ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parser
participant AST
participant Interpreter
participant Reconciler
participant Balances
User->>Parser: send 10 COIN from @alice \ "red" to @bob
Parser->>AST: Parse source with colorConstraint
AST->>Interpreter: SourceAccount{Account: alice, Color: "red"}
Interpreter->>Balances: coloredAsset("COIN", "red")
Balances-->>Interpreter: Return colored asset balance
Interpreter->>Reconciler: pushSender(alice, amount, color="red")
Reconciler->>Reconciler: Merge postings by account, destination, and colored asset
Reconciler-->>Interpreter: Return postings with colored asset
Interpreter-->>User: Execution result (postings, errors)
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 65.19% 65.52% +0.32%
==========================================
Files 39 39
Lines 7674 7895 +221
==========================================
+ Hits 5003 5173 +170
- Misses 2416 2459 +43
- Partials 255 263 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (4)
internal/interpreter/batch_balances_query.go (1)
93-117:⚠️ Potential issueEmpty string vs. nil pointer can create dangling
*suffix
evaluateOptExprAsmay return a pointer to an empty string ("").
coloredAssetwill then append"*"yieldingCOIN*, which is an invalid asset name and will poison balance keys.Consider normalising:
if color != nil && *color == "" { color = nil }immediately after evaluation (or inside
coloredAsset).Add a unit test for:
send 10 COIN from $acc \ ""to make sure it behaves as expected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 95-96: internal/interpreter/batch_balances_query.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 113-114: internal/interpreter/batch_balances_query.go#L113-L114
Added lines #L113 - L114 were not covered by testsinternal/interpreter/interpreter.go (2)
565-599: 💡 Verification agent🧩 Analysis chain
Correctly implemented color handling for account sending
The
trySendingToAccountfunction has been properly updated to support asset colors, with appropriate feature flag checking and balance retrieval using the colored asset.Similar to the
sendAllToAccountfunction, some error paths forevaluateOptExprAsaren't covered by tests.
🌐 Web query:
What are best practices for testing error paths in Go?💡 Result:
Best Practices for Testing Error Paths in Go
Testing error paths effectively in Go demands both thoughtful code structure and robust, clear test assertions. Below are key practices based on Go community experience and reputable guidance:
1. Factor Out IO and Side Effects
- Structure code so that external dependencies (files, databases, network calls) are abstracted behind interfaces or passed in as parameters. This enables you to inject test doubles (mocks, stubs) that can simulate error conditions, making error-path testing straightforward and reliable[1].
2. Check for Unexpected Errors Immediately
Always assign error returns to variables and check them right after function calls. If an error is unexpected, use
t.Fatalf(not justt.Error) to stop the test immediately. This prevents tests from continuing with invalid (possibly nil) values, which avoids misleading panics and makes failures clearer to diagnose[3][5].got, err := DoSomething() if err != nil { t.Fatalf("unexpected error: %v", err) }3. Assert and Verify Specific Error Conditions
Where errors are expected, assert both their presence and that they match the anticipated type, value, or message. Use Go’s error wrapping and unwrapping tools (
errors.Is,errors.As) to check for specific error types or sentinel errors._, err := SomeFuncThatFails() if err == nil { t.Fatal("expected error, got nil") } if !errors.Is(err, ErrExpected) { t.Fatalf("unexpected error: %v", err) }4. Create and Test with Custom Error Types
For richer error tests, use custom error types that add context or structure. In your tests, verify specific fields or behaviors of those custom errors, not just their general presence[2].
type MyError struct { Code int Msg string } func (e *MyError) Error() string { return e.Msg } // In tests, assert type and details: var myErr *MyError if !errors.As(err, &myErr) || myErr.Code != 123 { t.Errorf("unexpected error details: %v", err) }5. Keep Tests Isolated and Deterministic
- Avoid tests that depend on real IO, external services, or non-deterministic behaviors. Use dependency injection or stubs to simulate error conditions under controlled circumstances, keeping test runs fast and reliable[1].
6. Use Table-Driven Tests for Exhaustive Error Cases
Structure tests with slices of input/output/error expectations to systematically cover normal and failure cases, making error-path coverage thorough and easy to extend.
cases := []struct { input string wantErr bool errMatch error }{ {"bad-input", true, ErrValidation}, // more cases... }7. Add Context to Error Messages
- When propagating errors, wrap them with context to clarify failure origins. In tests, verify that relevant context appears in error messages where appropriate[2].
Summary Table: Key Practices
Practice Why It Matters Factor out IO/side effects Enables mock/test injection of errors[1] Use t.Fatalffor unexpected errorsStops on error, avoids misleading panics[3][5] Assert specific error types/values Ensures error-path correctness[2][3] Create and test custom errors Adds detail/context; easy to assert[2] Use dependency injection and stubs Keeps tests fast, isolated, repeatable[1] Table-driven tests for coverage Exhaustive, readable, maintainable Add context to errors Easier test failure diagnosis[2] By following these practices, Go developers can achieve reliable and maintainable error-path testing, greatly improving code robustness.
Citations:
- 1: https://fossa.com/blog/golang-best-practices-testing-go
- 2: https://www.jetbrains.com/guide/go/tutorials/handle_errors_in_go/best_practices/
- 3: https://bitfieldconsulting.com/posts/testing-errors
- 4: https://forum.golangbridge.org/t/go-unit-testing-how-to-specify-path-to-tests-or-package-under-test/20768
- 5: https://www.cloudbees.com/blog/testing-in-go
Add Error-Path Tests for evaluateOptExprAs in trySendingToAccount
While the color‐aware logic in
trySendingToAccountlooks solid, we need to cover its error branches forevaluateOptExprAsjust like insendAllToAccount. Please add tests that:
- Inject a failing
colorExprto forceevaluateOptExprAsinto its error path.- Use table‑driven tests to list both valid and invalid
colorExprinputs, expected errors, and messages.- Assert the specific error type or sentinel using
errors.Isorerrors.Asrather than just checking non‑nil.- Leverage mocks or stubs if necessary to isolate side effects (e.g., by passing a dummy interpreter state).
- Fail fast on unexpected errors with
t.Fatalfso you don’t hide panics later in the test.Locations to update:
- internal/interpreter/interpreter_test.go (add cases for
trySendingToAccount)- (Optional) mirror patterns from existing
sendAllToAccounttests for consistency.This will ensure all failure modes of
evaluateOptExprAsare exercised and your test suite remains robust.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 583-584: internal/interpreter/interpreter.go#L583-L584
Added lines #L583 - L584 were not covered by tests
458-487: 💡 Verification agent🧩 Analysis chain
Well-implemented color support with feature flag validation
The function now correctly handles color expressions for source accounts, including feature flag validation and error handling. The balance is fetched using the colored asset determined by the new
coloredAssethelper.However, some error paths aren't covered by tests:
🏁 Script executed:
#!/bin/bash # Check test coverage for the error paths in evaluateOptExprAs grep -A 10 "TestColor.*MissingFunds\|TestInvalidColor" internal/interpreter/interpreter_test.go || echo "No tests for evaluateOptExprAs error paths found"Length of output: 341
Approve color support—but add missing invalid-color test
The new
sendAllToAccountimplementation correctly handles colored assets with feature‑flag checks, and I see that the missing‑funds error path is already covered byTestColorRestrictBalanceWhenMissingFunds. However, we still need to cover the error path whencolorExpris provided but isn’t a string.Please add a test in
internal/interpreter/interpreter_test.go—for example:
TestSendAllToAccount_InvalidColorType(passing a non‑stringcolorExpr)
to verify thatevaluateOptExprAsreturns the expected type‑error.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 479-480: internal/interpreter/interpreter.go#L479-L480
Added lines #L479 - L480 were not covered by testsinternal/parser/antlrParser/numscript_parser.go (1)
3671-3728:⚠️ Potential issuePotential nil–pointer panic in bounded‑overdraft branch
Inside the bounded‑overdraft path the code assigns
maxOvedraftafter parsing
valueExpr(0)(line 3724) but does not checkHasError()as is done for
the earlier matches.
IfvalueExprthrows a recognition error the subsequent struct field
assignment will dereferenceniland panic.Add the same pattern used elsewhere:
- var _x = p.valueExpr(0) - localctx.(*SrcAccountBoundedOverdraftContext).maxOvedraft = _x + var _x = p.valueExpr(0) + if p.HasError() { goto errorExit } + localctx.(*SrcAccountBoundedOverdraftContext).maxOvedraft = _x🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3680-3680: internal/parser/antlrParser/numscript_parser.go#L3680
Added line #L3680 was not covered by tests
🧹 Nitpick comments (10)
internal/parser/parser.go (1)
229-244: Same concern for overdraft sources
SourceOverdraftnow carries color information – implementation mirrors the regular account path, all good.
Again, please ensure empty‑string colors do not turn into anASSET*artefact later on (see comment in interpreter file).internal/interpreter/batch_balances_query.go (2)
34-36: Save‑statement path passesnil– add a testGood catch updating the call site. The static‑analysis warning flags these lines as uncovered; a tiny unit test that executes a
savescript and asserts no panic / correct query batching would raise coverage and avoid regressions.
53-58: Avoid mutating the param for clarityRe‑assigning the
assetparameter obscures the intent and slightly hurts debuggability. Prefer a separate variable (or an earlyreturn).func (st *programState) batchQuery(account, asset string, color *string) { if account == "world" { return } - asset = coloredAsset(asset, color) + colored := coloredAsset(asset, color) - previousValues := st.CurrentBalanceQuery[account] - if !slices.Contains(previousValues, asset) { - st.CurrentBalanceQuery[account] = append(previousValues, asset) + prev := st.CurrentBalanceQuery[account] + if !slices.Contains(prev, colored) { + st.CurrentBalanceQuery[account] = append(prev, colored) } }internal/parser/antlrParser/Lexer.interp (1)
1-145: Generated artefact – consider excluding from VCS
.interpfiles are ANTLR‑generated and can bloat diffs / cause merge conflicts. Most Go projects only commit the*.gooutput.
If reproducible withgo generate(ormake antlr), think about adding them to.gitignore.internal/parser/antlrParser/Numscript.interp (1)
1-115: Same note on generated parser artefactsSee comment for
Lexer.interp; committing these huge binary‑encoded tables is optional if CI regenerates them.internal/interpreter/reconciler.go (1)
83-91: Cleaner subtraction with direct big.Int operationsThe replacement of temporary variables with direct
new(big.Int).Sub(...)calls improves code clarity and maintainability.internal/interpreter/interpreter_test.go (1)
4222-4244: Test function name has a typoThe function name
TestColorSendOverdratis missing an 'f' and should beTestColorSendOverdraft.-func TestColorSendOverdrat(t *testing.T) { +func TestColorSendOverdraft(t *testing.T) {Otherwise, the test correctly verifies that color restrictions work with overdraft functionality.
internal/parser/antlrParser/numscript_parser.go (3)
3617-3657: High duplication insourcealternatives – factor the optional color clauseEach of the three account‑based alternatives now repeats the same optional
if _la == RESTRICT { ColorConstraint() }block. This is easy to forget when future variants are added (e.g.,
ALLOWING ...).
Instead, make thecolorConstraintpart of the shared sub‑rule in the grammar, e.g.:addressWithColor : valueExpr (colorConstraint)? ;and reuse
addressWithColorwherever an account address is expected.
That removes the Go‑side duplication automatically after regeneration.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3631-3631: internal/parser/antlrParser/numscript_parser.go#L3631
Added line #L3631 was not covered by tests
2885-2898: Generated code committed – beware of merge noise & accidental editsThis entire hunk is antlr‑generated. Keeping generated artefacts under VCS is
fine, but manual tweaks (or unresolved merge conflicts) easily sneak in and are
hard to spot during reviews. To minimise risk:
- Treat
internal/parser/antlrParseras write‑only by the build pipeline.- Add a CI check (
git diff --exit-code $(git ls-files -m internal/parser/antlrParser)) to fail when these files differ from a clean regeneration.- Document the
go generate/antlrcommand inCONTRIBUTING.md.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2885-2889: internal/parser/antlrParser/numscript_parser.go#L2885-L2889
Added lines #L2885 - L2889 were not covered by tests
[warning] 2892-2894: internal/parser/antlrParser/numscript_parser.go#L2892-L2894
Added lines #L2892 - L2894 were not covered by tests
[warning] 2897-2897: internal/parser/antlrParser/numscript_parser.go#L2897
Added line #L2897 was not covered by tests
2880-2980: Missing unit‑test coverage for the new rule
codecov/patchflags every added line incolorConstraintas uncovered.
A minimal happy‑path and a few negative tests will lock behaviour and prevent
future regressions, e.g.:func TestParseColorConstraint(t *testing.T) { script := `send 1 COIN from vault \ "stripe" to reserve` ast, err := parser.Parse(script) require.NoError(t, err) require.Equal(t, `"stripe"`, ast.Statements[0].Source.Color) }Please add parser‑level tests covering:
- valid quoted color
- invalid expression after
\- absence of color constraint for backward compatibility.
Let me know if you’d like a PR with sample tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2885-2889: internal/parser/antlrParser/numscript_parser.go#L2885-L2889
Added lines #L2885 - L2889 were not covered by tests
[warning] 2892-2894: internal/parser/antlrParser/numscript_parser.go#L2892-L2894
Added lines #L2892 - L2894 were not covered by tests
[warning] 2897-2897: internal/parser/antlrParser/numscript_parser.go#L2897
Added line #L2897 was not covered by tests
[warning] 2910-2910: internal/parser/antlrParser/numscript_parser.go#L2910
Added line #L2910 was not covered by tests
[warning] 2912-2913: internal/parser/antlrParser/numscript_parser.go#L2912-L2913
Added lines #L2912 - L2913 were not covered by tests
[warning] 2926-2927: internal/parser/antlrParser/numscript_parser.go#L2926-L2927
Added lines #L2926 - L2927 were not covered by tests
[warning] 2932-2933: internal/parser/antlrParser/numscript_parser.go#L2932-L2933
Added lines #L2932 - L2933 were not covered by tests
[warning] 2936-2937: internal/parser/antlrParser/numscript_parser.go#L2936-L2937
Added lines #L2936 - L2937 were not covered by tests
[warning] 2940-2943: internal/parser/antlrParser/numscript_parser.go#L2940-L2943
Added lines #L2940 - L2943 were not covered by tests
[warning] 2946-2949: internal/parser/antlrParser/numscript_parser.go#L2946-L2949
Added lines #L2946 - L2949 were not covered by tests
[warning] 2960-2961: internal/parser/antlrParser/numscript_parser.go#L2960-L2961
Added lines #L2960 - L2961 were not covered by tests
[warning] 2971-2976: internal/parser/antlrParser/numscript_parser.go#L2971-L2976
Added lines #L2971 - L2976 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
internal/parser/__snapshots__/parser_fault_tolerance_test.snapis excluded by!**/*.snap,!**/*.snapinternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (23)
Lexer.g4(1 hunks)Numscript.g4(1 hunks)internal/analysis/check.go(2 hunks)internal/analysis/check_test.go(1 hunks)internal/flags/flags.go(2 hunks)internal/interpreter/balances.go(1 hunks)internal/interpreter/batch_balances_query.go(4 hunks)internal/interpreter/evaluate_expr.go(1 hunks)internal/interpreter/interpreter.go(9 hunks)internal/interpreter/interpreter_test.go(1 hunks)internal/interpreter/reconciler.go(3 hunks)internal/interpreter/reconciler_test.go(9 hunks)internal/parser/antlrParser/Lexer.interp(4 hunks)internal/parser/antlrParser/Lexer.tokens(2 hunks)internal/parser/antlrParser/Numscript.interp(4 hunks)internal/parser/antlrParser/Numscript.tokens(2 hunks)internal/parser/antlrParser/lexer.go(2 hunks)internal/parser/antlrParser/numscript_base_listener.go(1 hunks)internal/parser/antlrParser/numscript_listener.go(2 hunks)internal/parser/antlrParser/numscript_parser.go(67 hunks)internal/parser/ast.go(2 hunks)internal/parser/parser.go(4 hunks)internal/parser/parser_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
internal/interpreter/evaluate_expr.go (2)
internal/parser/ast.go (1)
ValueExpr(8-11)internal/interpreter/interpreter.go (1)
InterpreterError(70-73)
internal/parser/antlrParser/numscript_base_listener.go (1)
internal/parser/antlrParser/numscript_parser.go (2)
ColorConstraintContext(2880-2883)ColorConstraintContext(2897-2897)
internal/parser/antlrParser/numscript_listener.go (1)
internal/parser/antlrParser/numscript_parser.go (2)
ColorConstraintContext(2880-2883)ColorConstraintContext(2897-2897)
internal/analysis/check.go (4)
internal/parser/ast.go (8)
ValueExpr(8-11)AccountInterpLiteral(16-16)AccountInterpLiteral(53-56)Variable(13-13)Variable(64-67)Variable(81-81)StringLiteral(19-19)StringLiteral(42-45)internal/analysis/diagnostic_kind.go (4)
InvalidUnboundedAccount(244-244)InvalidUnboundedAccount(250-252)EmptiedAccount(254-256)EmptiedAccount(262-264)internal/interpreter/value.go (2)
String(17-17)String(27-27)internal/parser/range.go (1)
Range(13-16)
internal/parser/parser.go (3)
internal/parser/antlrParser/numscript_parser.go (1)
IColorConstraintContext(2866-2878)internal/parser/ast.go (1)
ValueExpr(8-11)internal/lsp/lsp_types/bindings.go (1)
Color(622-639)
internal/interpreter/reconciler_test.go (3)
internal/interpreter/reconciler.go (3)
Sender(25-29)Receiver(31-34)Posting(9-14)internal/interpreter/interpreter_test.go (1)
Posting(198-198)numscript.go (1)
Posting(52-52)
internal/parser/ast.go (1)
internal/lsp/lsp_types/bindings.go (1)
Color(622-639)
internal/analysis/check_test.go (3)
internal/analysis/check.go (1)
Diagnostic(119-123)internal/parser/range.go (2)
Range(13-16)RangeOfIndexed(155-164)internal/analysis/diagnostic_kind.go (4)
UnboundVariable(91-94)UnboundVariable(101-103)EmptiedAccount(254-256)EmptiedAccount(262-264)
internal/interpreter/interpreter.go (4)
internal/interpreter/reconciler.go (1)
Sender(25-29)internal/parser/ast.go (4)
ValueExpr(8-11)Source(122-125)SourceAccount(130-130)SourceAccount(135-138)internal/flags/flags.go (1)
ExperimentalAssetColors(12-12)internal/utils/utils.go (2)
MaxBigInt(25-35)MinBigInt(13-23)
🪛 GitHub Check: codecov/patch
internal/interpreter/batch_balances_query.go
[warning] 95-96: internal/interpreter/batch_balances_query.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 113-114: internal/interpreter/batch_balances_query.go#L113-L114
Added lines #L113 - L114 were not covered by tests
internal/parser/antlrParser/numscript_base_listener.go
[warning] 156-156: internal/parser/antlrParser/numscript_base_listener.go#L156
Added line #L156 was not covered by tests
[warning] 159-159: internal/parser/antlrParser/numscript_base_listener.go#L159
Added line #L159 was not covered by tests
internal/interpreter/interpreter.go
[warning] 479-480: internal/interpreter/interpreter.go#L479-L480
Added lines #L479 - L480 were not covered by tests
[warning] 583-584: internal/interpreter/interpreter.go#L583-L584
Added lines #L583 - L584 were not covered by tests
internal/parser/antlrParser/numscript_parser.go
[warning] 2885-2889: internal/parser/antlrParser/numscript_parser.go#L2885-L2889
Added lines #L2885 - L2889 were not covered by tests
[warning] 2892-2894: internal/parser/antlrParser/numscript_parser.go#L2892-L2894
Added lines #L2892 - L2894 were not covered by tests
[warning] 2897-2897: internal/parser/antlrParser/numscript_parser.go#L2897
Added line #L2897 was not covered by tests
[warning] 2910-2910: internal/parser/antlrParser/numscript_parser.go#L2910
Added line #L2910 was not covered by tests
[warning] 2912-2913: internal/parser/antlrParser/numscript_parser.go#L2912-L2913
Added lines #L2912 - L2913 were not covered by tests
[warning] 2926-2927: internal/parser/antlrParser/numscript_parser.go#L2926-L2927
Added lines #L2926 - L2927 were not covered by tests
[warning] 2932-2933: internal/parser/antlrParser/numscript_parser.go#L2932-L2933
Added lines #L2932 - L2933 were not covered by tests
[warning] 2936-2937: internal/parser/antlrParser/numscript_parser.go#L2936-L2937
Added lines #L2936 - L2937 were not covered by tests
[warning] 2940-2943: internal/parser/antlrParser/numscript_parser.go#L2940-L2943
Added lines #L2940 - L2943 were not covered by tests
[warning] 2946-2949: internal/parser/antlrParser/numscript_parser.go#L2946-L2949
Added lines #L2946 - L2949 were not covered by tests
[warning] 2960-2961: internal/parser/antlrParser/numscript_parser.go#L2960-L2961
Added lines #L2960 - L2961 were not covered by tests
[warning] 2971-2976: internal/parser/antlrParser/numscript_parser.go#L2971-L2976
Added lines #L2971 - L2976 were not covered by tests
[warning] 3631-3631: internal/parser/antlrParser/numscript_parser.go#L3631
Added line #L3631 was not covered by tests
[warning] 3680-3680: internal/parser/antlrParser/numscript_parser.go#L3680
Added line #L3680 was not covered by tests
[warning] 3741-3741: internal/parser/antlrParser/numscript_parser.go#L3741
Added line #L3741 was not covered by tests
🔇 Additional comments (48)
Lexer.g4 (1)
34-34: Good addition of the RESTRICT token for asset coloringThe backslash character (
'\\') has been added as a new lexer token to implement the "restriction" operator mentioned in the PR objectives. This token will be used to introduce color constraints in source specifications.internal/flags/flags.go (1)
12-12: Feature flag properly added for experimental asset colorsGood practice to gate this new functionality behind a feature flag, allowing for controlled rollout and maintaining backward compatibility. The flag is appropriately added to both the constants section and the
AllFlagsslice.Also applies to: 22-22
internal/parser/ast.go (3)
136-137: AST updated to support color constraints on source accountsThe addition of the
Colorfield to theSourceAccountstruct enables representing color constraints in the AST. This is a crucial part of the implementation for the asset coloring feature.
169-170: AST updated to support color constraints on overdraftsThe addition of the
Colorfield to theSourceOverdraftstruct enables representing color constraints for overdrafts, which aligns with the PR objective of allowing different maximum amounts and overdraft permissions for colored sub-assets.
136-137:✅ Verification successful
Verify type compatibility with existing Color struct
I notice there's a
Colorstruct in the codebase (internal/lsp/lsp_types/bindings.go) with different fields. While they're in different packages, it's worth verifying this won't cause any confusion.Also applies to: 169-170
🏁 Script executed:
#!/bin/bash # Check for any imports of lsp_types in files that might now use the new Color field rg "import.*internal/lsp/lsp_types" -A 10 # Check for any methods or functions that might be working with both types of Color rg "func.*Color" --type goLength of output: 3936
No conflicts between parser’s Color and lsp_types.Color
The new
Color ValueExprfield ininternal/parser/ast.golives entirely within theparserpackage and never imports or referencesinternal/lsp/lsp_types.Color. No compatibility issues detected—no further action needed.internal/parser/antlrParser/numscript_listener.go (2)
76-77: Implementation follows interface pattern correctly.The addition of
EnterColorConstraintlistener method correctly matches the pattern established throughout the interface for handling grammar productions.
208-209: Implementation follows interface pattern correctly.The addition of
ExitColorConstraintlistener method correctly matches the pattern established throughout the interface for handling grammar productions.internal/parser/antlrParser/numscript_base_listener.go (1)
155-157:✅ Verification successful
Implementation follows base listener pattern correctly.
The empty implementation of
EnterColorConstraintmatches the pattern used throughout the base listener class.Static analysis indicates this line isn't covered by tests. Consider adding tests for color constraint parsing.
🏁 Script executed:
#!/bin/bash # Check for test coverage of color constraint parsing cd internal/parser grep -r "TestParse.*Color" --include="*.go" .Length of output: 68
🏁 Script executed:
#!/bin/bash # Search for any tests covering ColorConstraint parsing under internal/parser grep -Rni "ColorConstraint" --include="*_test.go" internal/parser grep -Rni "colorConstraint" --include="*_test.go" internal/parser grep -Rni "color" --include="*_test.go" internal/parserLength of output: 452
#!/bin/bash # Display context around TestColorRestriction tests sed -n '470,530p' internal/parser/parser_test.go
Approve EnterColorConstraint and existing test coverage
The empty
EnterColorConstraintimplementation correctly follows the base listener pattern, and the parser already includes tests for color constraints:
TestColorRestriction,TestColorRestrictionBoundedOverdraft, andTestColorRestrictionUnboundedOverdraftininternal/parser/parser_test.gocover color constraint parsing.No additional tests are required.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 156-156: internal/parser/antlrParser/numscript_base_listener.go#L156
Added line #L156 was not covered by testsinternal/interpreter/evaluate_expr.go (1)
104-109: Well-designed helper function for optional expression evaluation.This new generic helper function elegantly handles nil expressions by returning nil without error, otherwise delegating to the existing
evaluateExprAsfunction. This simplifies code in callers that need to evaluate optional color constraints.internal/analysis/check.go (3)
479-479: Properly validates color expression type.The color constraint is correctly validated as a string type, which is appropriate for this feature.
487-495: Color-aware account tracking.The implementation creates a unique identifier for colored accounts by appending a representation of the color constraint to the account name. This approach correctly distinguishes between the same account with different color constraints.
521-521: Properly validates color expression for overdraft sources.The color constraint is correctly validated as a string type for overdraft sources as well.
internal/parser/parser.go (2)
163-171: Helper is concise & nil‑safe – looks goodThe
parseColorConstrainthelper cleanly encapsulates the optional parse logic and already copes with a missing node (nil).
No functional or stylistic issues spotted here.
180-186: Color now parsed forSourceAccount– check downstream handlingThe new
Colorfield is correctly filled, but make sure every downstream consumer (analysis, interpreter, reconciler, etc.) gracefully handles anilvalue. A quickgrepshows at least the interpreter usesevaluateOptExprAs, which is 👍. Just double‑check tests for weird inputs like\ "".internal/parser/antlrParser/Numscript.tokens (2)
32-42: Token addition correctly implements the asset coloring featureThe introduction of the
RESTRICTtoken at position 32 with the backslash character appropriately supports the new asset coloring feature. The shifting of subsequent token values maintains the lexer's integrity.
70-72: Character token assignments properly updatedThe character token assignments have been properly updated to include the new backslash character and adjust the position of subsequent tokens.
internal/interpreter/reconciler.go (3)
25-29: Sender struct extension looks goodThe addition of the
Color *stringfield to the Sender struct is appropriate for carrying color information throughout the reconciliation process.
87-91: Color preservation in leftover sender entriesGood implementation of preserving the color attribute when appending a leftover sender entry back to the senders stack. This ensures color information isn't lost during reconciliation.
103-109: Asset coloring in posting creation and mergingThe implementation correctly integrates color information into the asset string for postings and updates the merging logic to consider colored assets as distinct entities.
internal/parser/parser_test.go (3)
478-485: Basic color restriction test looks goodThis test appropriately verifies the parsing of a simple color restriction with a literal string value.
487-494: Color restriction with bounded overdraft testGood test coverage for the combined use of color restrictions with bounded overdraft functionality.
496-503: Variable color and unbounded overdraft testExcellent coverage of the variable color expression with unbounded overdraft, ensuring the parser correctly handles these complex combinations.
internal/parser/antlrParser/Lexer.tokens (2)
32-42: Token addition for RESTRICT correctly implementedThe Lexer tokens have been properly updated to include the new RESTRICT token for the backslash character, with appropriate adjustments to token numbering.
70-72: Character-to-token mappings properly updatedThe character-to-token mappings have been correctly updated to include the backslash character and adjust subsequent mappings.
Numscript.g4 (2)
44-44: The newcolorConstraintrule enhances expressivenessThis new parser rule introduces asset coloring functionality, enabling the restriction operator (
\) to specify a sub-asset from which to pull assets.
46-50: Well-structured source rule modificationsThe changes to the
sourcerule properly integrate the optionalcolorConstraintinto the three appropriate alternatives. This implementation is clean and maintains backward compatibility by making the color constraint optional.internal/parser/antlrParser/lexer.go (2)
49-50: Correct addition of the RESTRICT token literalThe backslash character has been properly added to the
LiteralNamesslice to represent theRESTRICTtoken.
304-314: Token indices properly updatedAll token constants following the new
LexerRESTRICT = 32have been correctly incremented by one to maintain proper token indexing.internal/analysis/check_test.go (3)
865-890: Comprehensive test for color variablesThis test properly verifies that unbound string-typed variables used as colors produce appropriate diagnostics with the correct variable names and types. The test covers both regular account sources and those with unbounded overdraft.
892-907: Good test for colored account differentiationThis test confirms an important aspect of the feature: accounts with different colors are treated as distinct assets, even when they share the same base account. This allows for proper tracking of sub-assets.
909-929: Important edge case for empty color stringsThis test validates the correct handling of empty color strings, ensuring they are treated as equivalent to no color and generate appropriate warnings for redundant accounts. The expectation of an
EmptiedAccountdiagnostic is appropriate for this case.internal/interpreter/reconciler_test.go (5)
39-39: Updated Sender initializationsAll existing
Senderstruct initializations have been properly updated to include the newColorfield set tonil, maintaining compatibility with the updated struct definition.Also applies to: 48-48, 62-62, 70-70, 91-92, 106-107, 125-126, 144-146, 160-160
171-180: Good basic test for colored asset reconciliationThis test verifies the fundamental behavior of colored asset reconciliation, ensuring that a colored asset in the source results in a posting with the proper colored asset string format (
COIN*x).
182-197: Complete testing of multiple destinations with colored assetsThis test correctly verifies that a colored source can distribute to multiple destinations while maintaining the color information in all resulting postings.
199-214: Important test for multiple colored sourcesThis test verifies a critical functionality: when multiple sources with different colors send to the same destination, the resulting postings correctly preserve each color's identity rather than merging them.
216-218: Useful generic helper functionThe
pointergeneric function is a clean utility that simplifies creating pointers to string literals for the color field. This approach is more concise than repeatedly using address-of operators with variables.internal/interpreter/interpreter.go (6)
304-309: Modified signature now includes color parameterThe
pushSendermethod has been updated to include acolor *stringparameter and store it in the Sender struct, which supports the new asset coloring feature.
494-495: Correctly passes color parameter from source accountThe
sendAllfunction now correctly passes theColorfield from theSourceAccounttosendAllToAccount.
505-506: Correctly passes color parameter from source overdraftThe
sendAllfunction now correctly passes theColorfield from theSourceOverdrafttosendAllToAccount.
607-608: Correctly passes color parameter from source accountThe color field is now properly passed to
trySendingToAccountfromSourceAccount.
618-619: Correctly passes color parameter from source overdraftThe color field is now properly passed to
trySendingToAccountfromSourceOverdraft.
878-879: Updated to support colored assets in balance queriesThe
getBalancefunction call tobatchQuerynow includes anilthird parameter to match the updated signature that supports colored assets.internal/interpreter/interpreter_test.go (5)
4198-4220: Good basic test for color sending functionalityThis test verifies that using the
\operator with a color string on a source account results in a posting with the asset formatted asASSET*color. The test is well-structured and comprehensive.
4246-4271: Well-designed test for balance restrictions with colorsThis test correctly verifies that the interpreter uses the balance of the colored asset when fetching from a source with a color restriction, even when the uncolored balance is insufficient.
4297-4319: Comprehensive test for colored assets with send all operationThis test correctly verifies that the send all operation works with colored assets, checking both the source restriction and the resulting posting.
4321-4363: Thorough test for multiple colored sourcesThis test effectively verifies the correct behavior when multiple color restrictions are used in order, including both colored and uncolored assets from the same account.
4365-4390: Important edge case test for empty color stringsThis test handles the important edge case of empty string colors, verifying that they behave the same as uncolored assets. This is a good defensive programming practice.
internal/parser/antlrParser/numscript_parser.go (1)
38-48:✅ Verification successful
Token index shift – please confirm all downstream usages were regenerated
RESTRICTis appended at index 32 inside bothLiteralNamesandSymbolicNames.
Because these constant values are referenced everywhere (manual traversals, unit tests, interpreter switch‑statements, etc.), any place that was not regenerated from the grammar will now be off‑by‑one starting at the new token.Please double‑check that:
Lexer.g4/Numscript.g4were updated (not just the generated Go file).- All consumers were re‑generated (
go generate,antlr, etc.) instead of hand‑edited – otherwise the shift silently corrupts parsing.- Custom assertions in tests (e.g., hard‑coded numeric token values) were adapted.
Would you run a quick search to make sure no magic numbers
>=32are still hard‑coded?
🏁 Script executed:
#!/bin/bash # Look for suspicious numeric token literals >= 32 used outside generated files fd -t f -e go | grep -v "internal/parser/antlrParser" | xargs rg --line-number '\b3[2-9]\b|\b[4-9][0-9]\b'Length of output: 10955
No hard‑coded token indices detected outside the generated parser
I ran the specified search for numeric literals ≥ 32 and found only domain‑specific values (version numbers, line/character positions, monetary/test data, etc.)—no instances of manual token index references. All parser tokens appear to be consumed via the regenerated constants.
| // ExitColorConstraint is called when production colorConstraint is exited. | ||
| func (s *BaseNumscriptListener) ExitColorConstraint(ctx *ColorConstraintContext) {} | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Implementation follows base listener pattern correctly.
The empty implementation of ExitColorConstraint matches the pattern used throughout the base listener class.
Static analysis indicates this line isn't covered by tests. Consider adding tests for color constraint parsing.
🏁 Script executed:
#!/bin/bash
# Check for test coverage of color constraint exiting
cd internal/parser
grep -r "ExitColorConstraint" --include="*.go" . | grep -v "numscript_base_listener\|numscript_listener"Length of output: 194
🏁 Script executed:
#!/bin/bash
# Locate Go test files under internal/parser
find internal/parser -type f -name '*_test.go'Length of output: 216
Add test coverage for the colorConstraint rule
The empty listener override in BaseNumscriptListener.ExitColorConstraint correctly follows the base‑listener pattern, but we couldn’t find any existing test exercising the colorConstraint production. Please add a parser test that parses an input containing a color constraint (e.g. color = “red”) and verifies the rule is invoked.
• No occurrences of ColorConstraint in any *_test.go under internal/parser
• Suggest creating a new color_constraint_test.go (or augmenting parser_test.go) to cover this case
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 159-159: internal/parser/antlrParser/numscript_base_listener.go#L159
Added line #L159 was not covered by tests
| if _, emptied := res.emptiedAccount[coloredAccountName]; emptied && !account.IsWorld() { | ||
| res.pushDiagnostic(account.Range, EmptiedAccount{Name: account.String()}) | ||
| } | ||
|
|
||
| res.emptiedAccount[account.String()] = struct{}{} | ||
| res.emptiedAccount[coloredAccountName] = struct{}{} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent error message for emptied colored accounts.
While the code correctly tracks emptied accounts using the colored account name, the error message still only shows the base account name without the color information.
Consider updating the error message to include color information:
if _, emptied := res.emptiedAccount[coloredAccountName]; emptied && !account.IsWorld() {
- res.pushDiagnostic(account.Range, EmptiedAccount{Name: account.String()})
+ res.pushDiagnostic(account.Range, EmptiedAccount{Name: coloredAccountName})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _, emptied := res.emptiedAccount[coloredAccountName]; emptied && !account.IsWorld() { | |
| res.pushDiagnostic(account.Range, EmptiedAccount{Name: account.String()}) | |
| } | |
| res.emptiedAccount[account.String()] = struct{}{} | |
| res.emptiedAccount[coloredAccountName] = struct{}{} | |
| if _, emptied := res.emptiedAccount[coloredAccountName]; emptied && !account.IsWorld() { | |
| - res.pushDiagnostic(account.Range, EmptiedAccount{Name: account.String()}) | |
| + res.pushDiagnostic(account.Range, EmptiedAccount{Name: coloredAccountName}) | |
| } | |
| res.emptiedAccount[coloredAccountName] = struct{}{} |
| func TestColorRestrictBalanceWhenMissingFunds(t *testing.T) { | ||
| script := ` | ||
| send [COIN 20] ( | ||
| source = @acc \ "red" | ||
| destination = @dest | ||
| ) | ||
| ` | ||
|
|
||
| tc := NewTestCase() | ||
| tc.setBalance("acc", "COIN", 100) | ||
| tc.setBalance("acc", "COIN*red", 1) | ||
| tc.compile(t, script) | ||
|
|
||
| tc.expected = CaseResult{ | ||
| Postings: []Posting{}, | ||
| Error: machine.MissingFundsErr{ | ||
| Needed: *big.NewInt(20), | ||
| Available: *big.NewInt(1), | ||
| Asset: "COIN", | ||
| }, | ||
| } | ||
| testWithFeatureFlag(t, tc, flags.ExperimentalAssetColors) | ||
| } |
There was a problem hiding this comment.
Inconsistency in error asset field
In the expected error, the Asset field is set to "COIN" but it should be "COIN*red" to accurately reflect that we're missing funds of the colored asset.
tc.expected = CaseResult{
Postings: []Posting{},
Error: machine.MissingFundsErr{
Needed: *big.NewInt(20),
Available: *big.NewInt(1),
- Asset: "COIN",
+ Asset: "COIN*red",
},
}There was a problem hiding this comment.
neh, we'll keep that this way. The statements deals with COIN, not COIN*red.
There was a problem hiding this comment.
@ascandone I understand - the error should reference "COIN" as specified in the statement rather than the colored version. Thanks for the clarification.
✏️ Learnings added
Learnt from: ascandone
PR: formancehq/numscript#55
File: internal/interpreter/interpreter_test.go:4273-4295
Timestamp: 2025-04-23T16:27:16.317Z
Learning: When a numscript operation fails due to insufficient funds on a colored asset (e.g., "COIN*red"), the error references the uncolored asset (e.g., "COIN") as specified in the script, not the colored version used internally.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| localctx = NewColorConstraintContext(p, p.GetParserRuleContext(), p.GetState()) | ||
| p.EnterRule(localctx, 22, NumscriptParserRULE_colorConstraint) | ||
| p.EnterOuterAlt(localctx, 1) | ||
| { | ||
| p.SetState(136) | ||
| p.Match(NumscriptParserRESTRICT) | ||
| if p.HasError() { | ||
| // Recognition error - abort rule | ||
| goto errorExit | ||
| } | ||
| } | ||
| { | ||
| p.SetState(137) | ||
| p.valueExpr(0) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
colorConstraint grammar is overly permissive – restrict it to string literals for now
Today any valueExpr is accepted after \, so a script can compile with:
send 10 COIN from vault \ 1 + 2
which makes little semantic sense and will probably blow up during execution.
If the intent is “only string (or identifier) color names”, consider tightening the rule in the grammar rather than post‑validating later:
colorConstraint
: RESTRICT (STRING | IDENTIFIER) # ColorString
;After editing Numscript.g4, regenerate the Go sources; do not hand‑edit this generated file, otherwise the change will be lost on the next antlr run.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2960-2961: internal/parser/antlrParser/numscript_parser.go#L2960-L2961
Added lines #L2960 - L2961 were not covered by tests
There was a problem hiding this comment.
nice catch; although we don't want to encode such constraints in the grammar. This'll be allowed by the grammar (in the same way it's sintactically valid to write true*100 in golang) and will result for the static analysis errors or else a runtime error
There was a problem hiding this comment.
@ascandone Thanks for clarifying your approach! I understand your design philosophy - keeping the grammar permissive while enforcing semantic constraints during static analysis or runtime is indeed a common pattern in language design. This separation of concerns gives you more flexibility for error handling and future language extensions without having to modify the grammar. Similar to how Go allows syntactically valid but semantically invalid expressions like true*100, your approach will likely lead to better error messages for users.
✏️ Learnings added
Learnt from: ascandone
PR: formancehq/numscript#55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.818Z
Learning: In numscript, type constraints for color expressions are not enforced at the grammar level but are instead handled during static analysis or at runtime. The grammar is intentionally kept permissive (accepting any valueExpr after the RESTRICT token) to maintain separation of concerns.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/interpreter/interpreter_error.go (1)
232-239: LGTM: New InvalidColor error type is well-structured.The new error type follows the existing pattern in the codebase by embedding
parser.Rangefor error location tracking and providing a clear error message. It aligns with the PR objectives of implementing asset coloring functionality.Consider making the error message slightly more flexible to accommodate potential future changes in color name requirements:
- return fmt.Sprintf("Invalid color name: '%s'. Only uppercase letters are allowed.", e.Color) + return fmt.Sprintf("Invalid color name: '%s'. Color names must consist of uppercase letters only.", e.Color)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/interpreter/evaluate_expr.go(2 hunks)internal/interpreter/interpreter.go(9 hunks)internal/interpreter/interpreter_error.go(1 hunks)internal/interpreter/interpreter_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/interpreter/evaluate_expr.go
- internal/interpreter/interpreter_test.go
- internal/interpreter/interpreter.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/interpreter/interpreter_error.go (1)
internal/parser/range.go (1)
Range(13-16)
Note for reviewers: the PR can be read commit by commit
Specs
This PR implements the core part of an "asset coloring" functionality, by introducing a "restriction" operator (ONLY in source position) that specifies the sub-asset where to pull the asset from.
For example:
will produce the following postings:
@user@destCOIN*stripewhere
COIN*stripeis an ordinary name for an asset in the ledger (except it wouldn't be accepted if we tried to send that asset directly (both via numscript or via the ledger postings API)This will allow to use all the numscript expressivity for more complex statements, e.g.
may produce the following postings:
@user@destCOIN*stripe@user@destCOIN*adyen@user@destCOIN*paypalFuture development and missing functionality
syntax: we'll probably change the "ASSET*color" representation, and we might consider using a different syntax
missing functionality: this feature doesn't yet allow to express something like "send a currency of any color". We think we'll archive similar functionality not by this syntax, but via other extensions (like looping constructs)