feat: asset scaling #102
feat: asset scaling #102
Conversation
WalkthroughAdds asset-scaling syntax and runtime support: three lexer tokens (WITH, SCALING, THROUGH), a SourceWithScaling AST/grammar branch, asset-scaling algorithms, interpreter handling (send/trySend variants), tests, feature flag, and related parser/listener codegen updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
User->>Parser: parse script (includes "with scaling through `@swap`")
Parser->>AST: produce SourceWithScaling node
User->>Interpreter: execute/send action
Interpreter->>BalanceStore: fetch balances for involved accounts (base asset / scaled variants)
Interpreter->>AssetScaling: call findSolution(requiredAmount, balances)
AssetScaling-->>Interpreter: solution (scale steps) or nil
alt solution found
Interpreter->>Ledger: emit postings for each scale step and final scaling posting
Interpreter->>BalanceStore: update cached balances
Interpreter->>Destination: deliver amount
else no solution
Interpreter-->>User: return error / fallback path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
+ Coverage 67.84% 68.35% +0.51%
==========================================
Files 45 46 +1
Lines 4422 4608 +186
==========================================
+ Hits 3000 3150 +150
- Misses 1260 1282 +22
- Partials 162 176 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ascandone
force-pushed
the
feat/asset-scaling
branch
2 times, most recently
from
593b157 to
2ba44fd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
internal/parser/parser_test.go (1)
505-512: LGTM - Parser test for scaling syntax.The test correctly validates parsing of the
with scalingsyntax and follows the established test patterns in this file.Consider adding additional test cases for edge scenarios:
- Scaling with color constraint:
@src \ "color" with scaling- Scaling with variable account:
$account with scalingThese would ensure comprehensive coverage of the new syntax in combination with existing features.
internal/interpreter/asset_scaling.go (1)
81-83: Integer overflow possible in sort comparison for extreme scale values.Casting
int64difference tointmay overflow on 32-bit systems or for large scale differences. Consider usingcmp.Compareor explicit comparison.🔎 Proposed fix
+import "cmp" + // Sort in ASC order (e.g. EUR, EUR/2, ..) slices.SortFunc(assets, func(p scalePair, other scalePair) int { - return int(p.scale - other.scale) + return cmp.Compare(p.scale, other.scale) })internal/interpreter/asset_scaling_test.go (3)
127-140: Typo in test function name.
TestUnboundedScalinHigherAssetshould beTestUnboundedScalingHigherAsset(missing 'g').🔎 Proposed fix
-func TestUnboundedScalinHigherAsset(t *testing.T) { +func TestUnboundedScalingHigherAsset(t *testing.T) {
142-153: Typo in test function name.
TestUnboundedScalinHigherAssetTrimRemaindershould beTestUnboundedScalingHigherAssetTrimRemainder.🔎 Proposed fix
-func TestUnboundedScalinHigherAssetTrimRemainder(t *testing.T) { +func TestUnboundedScalingHigherAssetTrimRemainder(t *testing.T) {
27-41: Consider verifyingtotalSentreturn value.The tests ignore the second return value (
totalSent) fromfindSolution. For comprehensive coverage, consider asserting on this value to ensure the total amount computation is correct.🔎 Example
- sol, _ := findSolution( + sol, totalSent := findSolution( // Need [EUR/2 200] big.NewInt(200), 2, // Have: {EUR/2: 201} map[int64]*big.Int{ }) require.Equal(t, []scalePair{ {2, big.NewInt(200)}, }, sol) + require.Equal(t, big.NewInt(200), totalSent)internal/interpreter/interpreter.go (1)
525-570: Consider extracting common scaling logic to reduce duplication.The scaling logic in
sendAll(lines 525-570) andtrySendingUpTo(lines 682-733) is nearly identical:
- Resolve account and asset scale
- Fetch cached balances
- Call
findSolution- Emit postings for each scaling step
- Emit final scaling posting
- Update balances
Extracting this into a helper method would improve maintainability.
Also applies to: 682-733
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-all-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling.num.specs.jsonis excluded by!**/*.jsoninternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (23)
Lexer.g4Numscript.g4internal/analysis/check.gointernal/interpreter/asset_scaling.gointernal/interpreter/asset_scaling_test.gointernal/interpreter/batch_balances_query.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_test.gointernal/interpreter/testdata/script-tests/experimental/scaling/scaling-all-allotment.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling-allotment.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling.numinternal/parser/antlrParser/Lexer.interpinternal/parser/antlrParser/Lexer.tokensinternal/parser/antlrParser/Numscript.interpinternal/parser/antlrParser/Numscript.tokensinternal/parser/antlrParser/lexer.gointernal/parser/antlrParser/numscript_base_listener.gointernal/parser/antlrParser/numscript_listener.gointernal/parser/antlrParser/numscript_parser.gointernal/parser/ast.gointernal/parser/parser.gointernal/parser/parser_test.go
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-12-05T11:42:58.472Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
Applied to files:
internal/interpreter/interpreter_test.gointernal/interpreter/asset_scaling_test.go
📚 Learning: 2025-07-16T14:51:51.052Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 70
File: internal/interpreter/accounts_metadata.go:13-24
Timestamp: 2025-07-16T14:51:51.052Z
Learning: The DeepClone method in internal/interpreter/accounts_metadata.go works correctly despite using MapGetOrPutDefault in what initially appears to be a non-assignment context. The fetchAccountMetadata method returns a reference to the map stored in the cloned structure, and MapGetOrPutDefault modifies it in-place, so the cloned values persist correctly.
Applied to files:
internal/interpreter/interpreter_test.gointernal/interpreter/interpreter.gointernal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-06T17:48:48.962Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-19T11:57:21.654Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 39
File: internal/parser/parser.go:0-0
Timestamp: 2025-02-19T11:57:21.654Z
Learning: Percentage literals in NumScript are inherently bounded as they represent percentages, making overflow checks unnecessary. The uint16 type for floating digits is more than sufficient as it can handle up to 65535 decimal places.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-06T17:48:48.961Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-01-20T23:01:48.857Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/interpreter/interpreter.go:479-488
Timestamp: 2025-01-20T23:01:48.857Z
Learning: In the `sendAll` method of the Numscript interpreter, the `oneof` operator only uses the first source because none of the branches can fail in a "send all" context. This differs from `trySendingUpTo` where backtracking is needed as sources can fail when trying to send a specific amount.
Applied to files:
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.num
📚 Learning: 2025-04-22T16:45:07.842Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
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.
Applied to files:
Numscript.g4
🧬 Code graph analysis (9)
internal/parser/parser_test.go (3)
internal/parser/parser.go (1)
Parse(50-81)numscript.go (2)
Parse(40-42)Value(72-72)internal/interpreter/value.go (1)
Value(12-15)
internal/interpreter/batch_balances_query.go (1)
internal/parser/ast.go (2)
SourceWithScaling(133-133)SourceWithScaling(175-179)
internal/parser/parser.go (3)
internal/parser/antlrParser/numscript_parser.go (1)
SrcAccountWithScalingContext(3317-3320)internal/parser/ast.go (2)
SourceWithScaling(133-133)SourceWithScaling(175-179)internal/parser/range.go (1)
Range(13-16)
internal/interpreter/interpreter_test.go (2)
internal/interpreter/interpreter.go (4)
StaticStore(35-38)Balances(25-25)AccountBalance(24-24)BalanceQuery(19-19)numscript.go (4)
StaticStore(70-70)Balances(58-58)AccountBalance(57-57)BalanceQuery(55-55)
internal/analysis/check.go (1)
internal/parser/ast.go (2)
SourceWithScaling(133-133)SourceWithScaling(175-179)
internal/interpreter/asset_scaling.go (2)
internal/interpreter/interpreter.go (1)
AccountBalance(24-24)internal/utils/utils.go (1)
MinBigInt(12-22)
internal/parser/antlrParser/numscript_listener.go (1)
internal/parser/antlrParser/numscript_parser.go (1)
SrcAccountWithScalingContext(3317-3320)
internal/parser/antlrParser/numscript_base_listener.go (1)
internal/parser/antlrParser/numscript_parser.go (1)
SrcAccountWithScalingContext(3317-3320)
internal/parser/ast.go (1)
internal/parser/range.go (1)
Range(13-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (24)
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-all-allotment.num (1)
1-7: LGTM! Well-structured test case for scaling with allotment.This test data effectively demonstrates a "send all" operation with scaling applied to one source in a multi-source allotment scenario. The syntax is clean and the test case covers an important edge case where only one of multiple sources uses scaling.
internal/interpreter/testdata/script-tests/experimental/scaling/scaling.num (1)
1-4: LGTM! Clean baseline test for scaling feature.This test provides a straightforward example of the scaling syntax with a fixed amount. The simplicity makes it an excellent foundational test case for the new feature.
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-allotment.num (1)
1-7: LGTM! Good test coverage for fixed-amount scaling with allotment.This test complements the other scaling tests by exercising a fixed-amount send operation with scaling on one of multiple sources. The test suite now covers both "send all" and fixed-amount scenarios.
internal/parser/antlrParser/numscript_base_listener.go (1)
177-181: LGTM! Correctly generated ANTLR base listener methods.The new
EnterSrcAccountWithScalingandExitSrcAccountWithScalingmethods follow the established pattern for ANTLR-generated base listener implementations. They're correctly positioned alongside other source-related production handlers.internal/parser/antlrParser/numscript_listener.go (2)
85-86: LGTM! Correctly generated listener interface method.The
EnterSrcAccountWithScalingmethod is properly added to theNumscriptListenerinterface, following ANTLR conventions for parser rule context handling.
220-221: LGTM! Correctly generated listener interface method.The
ExitSrcAccountWithScalingmethod complements the Enter method and is correctly positioned in the Exit methods section of the interface.internal/parser/antlrParser/Lexer.interp (1)
35-36: LGTM - Auto-generated lexer interpretation file updated correctly.The WITH and SCALING tokens are properly added to the literal names, symbolic names, rule names, and ATN data sections. This file appears to be correctly regenerated from the updated Lexer.g4 grammar.
Also applies to: 82-83, 128-129, 151-151
Numscript.g4 (1)
46-54: LGTM - Grammar updated for asset scaling support.The source rule now includes
srcAccountWithScalingas a new alternative with correct token ordering (WITH SCALING). The grammar structure properly integrates with existing source patterns.internal/parser/antlrParser/Lexer.tokens (1)
33-44: LGTM - Token definitions correctly updated.The WITH and SCALING tokens are properly added with sequential IDs (33, 34), and all subsequent tokens are correctly reindexed. The literal mappings for the new tokens and shifted '@' and ':' are consistent.
Also applies to: 73-76
internal/interpreter/batch_balances_query.go (1)
101-113: LGTM - Balance query batching for SourceWithScaling.The implementation correctly follows the pattern established by
SourceAccount, with the key difference being the use ofassetToScaledAsset(st.CurrentAsset)for the batch query. This appropriately transforms the asset identifier to its scaled namespace format (appending/*to the namespace portion) for scaled balance lookups.internal/analysis/check.go (1)
596-599: Clarify whether @world and emptiedAccount validation should apply to SourceWithScaling.The type checking correctly validates
AddressasTypeAccountandColorasTypeString. However, unlikeSourceAccountandSourceOverdraft, this case lacks:
- Validation for
@worldaccount (triggeringInvalidUnboundedAccountin send-all context)emptiedAccounttracking to detect duplicate account usageThe grammar defines
SourceWithScalingasaddress = valueExpr colorConstraint? WITH SCALING, and it uses the sameAddressfield as other source types. WhileSourceAllotmenthas documented exemptions from send-all contexts, no such documentation exists forSourceWithScaling.Verify whether these omissions are intentional due to scaling-specific semantics, or if the validation checks should be added for consistency.
Lexer.g4 (1)
35-36: LGTM!The new
WITHandSCALINGtokens are correctly defined following the established pattern for keyword tokens in the lexer grammar.internal/parser/parser.go (1)
240-246: LGTM!The new parsing branch for
SrcAccountWithScalingContextcorrectly constructs theSourceWithScalingAST node, following the established pattern used by other source types likeSourceOverdraft.internal/parser/ast.go (2)
127-133: LGTM!The
source()method declarations are properly aligned, andSourceWithScalingcorrectly implements theSourceinterface.
174-179: LGTM!The
SourceWithScalingstruct is well-designed, mirroringSourceOverdraftwith the appropriate fields (Range,Color,Address). The embeddedRangeprovides correctRangedinterface compliance viaGetRange().internal/interpreter/interpreter_test.go (2)
157-189: LGTM!The "request currencies" subtest correctly validates
GetBalancesbehavior with specific currency queries. The test setup and assertions are well-structured.
191-214: LGTM!The "assets catchall" subtest correctly validates the wildcard asset matching feature (
USD/*) that returns all USD variants. This aligns with the asset scaling feature's need to query scaled asset variants.internal/parser/antlrParser/Numscript.interp (1)
35-36: LGTM!This generated file correctly reflects the new
WITHandSCALINGtokens added to the grammar.Also applies to: 82-83
internal/interpreter/asset_scaling_test.go (2)
10-25: Skipped test indicates potentially incomplete functionality.
TestScalingZeroNeededis skipped, which may indicate the zero-amount edge case is not yet handled. Consider adding a TODO or tracking issue if this is intentional.
84-97: Skipped test for mixed scale failure case.
TestMixedFailis skipped. This suggests the mixed-scale insufficient funds scenario may not be fully implemented yet.internal/interpreter/interpreter.go (1)
55-73: Catch-all asset pattern implementation looks correct.The logic correctly handles the
/*suffix pattern by iterating over account balances and copying those that match either the base asset exactly or have the base asset as a prefix. The use ofnew(big.Int).Set(v)ensures proper value copying.internal/parser/antlrParser/numscript_parser.go (1)
1-1: Generated parser code - no manual review required.This file is ANTLR-generated from
Numscript.g4. The additions forSrcAccountWithScalingContext,WITH/SCALINGtokens, and source rule parsing are consistent with the grammar extension.internal/parser/antlrParser/Numscript.tokens (1)
33-44: Generated token definitions - consistent with grammar.The new
WITHandSCALINGtokens are correctly added at positions 33 and 34, with subsequent tokens renumbered appropriately.internal/parser/antlrParser/lexer.go (1)
1-1: Generated lexer code - no manual review required.This file is ANTLR-generated from
Lexer.g4. The additions forWITHandSCALINGtokens (constants 33 and 34) are consistent with the grammar and other generated files.
| func getAssets(balance AccountBalance, baseAsset string) map[int64]*big.Int { | ||
| result := make(map[int64]*big.Int) | ||
| for asset, amount := range balance { | ||
| if strings.HasPrefix(asset, baseAsset) { | ||
| _, scale := getAssetScale(asset) | ||
| result[scale] = amount | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Prefix matching may cause false positives.
strings.HasPrefix(asset, baseAsset) will incorrectly match assets like "USDC" or "USDC/2" when searching for "USD". Consider checking for an exact match or ensuring the prefix is followed by / or end-of-string.
🔎 Proposed fix
func getAssets(balance AccountBalance, baseAsset string) map[int64]*big.Int {
result := make(map[int64]*big.Int)
for asset, amount := range balance {
- if strings.HasPrefix(asset, baseAsset) {
+ base, _ := getAssetScale(asset)
+ if base == baseAsset {
_, scale := getAssetScale(asset)
result[scale] = amount
}
}
return result
}🤖 Prompt for AI Agents
internal/interpreter/asset_scaling.go around lines 41 to 50: the current
strings.HasPrefix(asset, baseAsset) check can produce false positives (e.g.
"USDC" matching "USD"); change the condition to accept either an exact match
(asset == baseAsset) or a prefix followed by a separator (e.g.
strings.HasPrefix(asset, baseAsset+"/")) so only the base asset or variants like
"USD/..." match; update the if to use this combined check and keep the rest of
the function unchanged.
| if leftAmt != nil && leftAmt.Cmp(big.NewInt(0)) != 1 { | ||
| break | ||
| } |
There was a problem hiding this comment.
Break condition may not trigger correctly.
leftAmt is computed before totalSent is updated, so after the update, leftAmt still holds the old value. The check leftAmt.Cmp(big.NewInt(0)) != 1 (i.e., leftAmt <= 0) should use the updated remaining amount. Currently, this may cause an extra iteration or incorrect early termination.
🔎 Proposed fix
Recompute the remaining amount after updating totalSent:
totalSent.Add(totalSent, taken)
intPart := new(big.Int).Mul(taken, scalingFactor.Denom())
intPart.Div(intPart, scalingFactor.Num())
if intPart.Cmp(big.NewInt(0)) == 0 {
continue
}
out = append(out, scalePair{
scale: p.scale,
amount: intPart,
})
- if leftAmt != nil && leftAmt.Cmp(big.NewInt(0)) != 1 {
+ if neededAmt != nil {
+ remaining := new(big.Int).Sub(neededAmt, totalSent)
+ if remaining.Cmp(big.NewInt(0)) <= 0 {
+ break
+ }
+ }
- break
- }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/interpreter/asset_scaling.go around lines 129 to 131, the break
condition uses leftAmt computed before totalSent is updated, so it may be stale;
after you update totalSent recompute the remaining amount (e.g., recalc leftAmt
= totalNeeded.Sub(totalNeeded, totalSent) or recompute from original values) and
then check leftAmt.Cmp(big.NewInt(0)) != 1 to decide breaking; replace the
existing break check with one that uses this recomputed remaining amount so the
loop terminates correctly.
There was a problem hiding this comment.
@coderabbitai give me tests that can reproduce the error. Or describe them if you can't reproduce the numscript exactly
There was a problem hiding this comment.
@ascandone, here's a test case that demonstrates the issue:
func TestScalingBreakConditionStale(t *testing.T) {
// Setup: Need exactly 100 at scale 0
// Available: scale 0 has 100, scale 1 has 1000
// Expected: Should only use scale 0 (since it satisfies the need exactly)
// Bug: The function continues to the next iteration unnecessarily
neededAmt := big.NewInt(100)
neededScale := int64(0)
scales := map[int64]*big.Int{
0: big.NewInt(100), // Exactly what we need
1: big.NewInt(1000), // Extra available at higher scale
}
result, totalSent := findSolution(neededAmt, neededScale, scales)
// After processing scale 0:
// - leftAmt = 100 - 0 = 100 (before update)
// - taken = 100
// - totalSent = 100
// - Check: leftAmt (100) <= 0? False → continues to next iteration
//
// Next iteration (scale 1):
// - leftAmt = 100 - 100 = 0
// - taken = 0
// - Gets skipped due to intPart == 0 check
//
// While functionally correct, the break condition should have
// terminated after the first iteration since remaining = 0
assert.Equal(t, big.NewInt(100), totalSent)
assert.Equal(t, 1, len(result)) // Should only have scale 0
assert.Equal(t, int64(0), result[0].scale)
assert.Equal(t, big.NewInt(100), result[0].amount)
}Description of the issue:
The problem occurs when an asset scale exactly satisfies the remaining need. In the example above:
- First iteration:
leftAmt = 100, we take all 100, updatetotalSent = 100 - The break check uses the old
leftAmt(100), not the remaining after the update (0) - Since
100 > 0, we don't break - Second iteration computes
leftAmt = 0, which correctly results intaken = 0and gets skipped
While this doesn't produce incorrect results (due to the intPart == 0 check), it causes an unnecessary iteration. The fix would ensure we break immediately when the requirement is fully satisfied, making the logic clearer and slightly more efficient.
There was a problem hiding this comment.
@coderabbitai I ran this test as-is and it's passing
There was a problem hiding this comment.
@ascandone, you're absolutely right — the test passes because the code is functionally correct. I need to clarify my original assessment:
What actually happens:
-
When an asset scale exactly satisfies the remaining need:
leftAmtis computed asneededAmt - totalSent(old value)- We take the needed amount and update
totalSent - The break condition checks the stale
leftAmt(which is still > 0) - Loop continues to next iteration
-
Next iteration:
- Computes new
leftAmt = neededAmt - totalSent(now 0) taken = min(allowed, 0) = 0intPartbecomes 0- The check on line 119 (
if intPart.Cmp(big.NewInt(0)) == 0) causes acontinue, skipping that entry
- Computes new
The verdict:
This is a minor inefficiency (one unnecessary loop iteration), not a correctness bug. The intPart == 0 check prevents any incorrect results. My original severity rating of "Major" was too high.
The suggested fix would make the code slightly cleaner and more efficient by breaking immediately when the need is satisfied, but it's not critical. If you prefer to keep the code as-is for simplicity, that's reasonable.
Thank you for verifying — this helped clarify the actual impact! 🙏
internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-send-all.num
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@internal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.num:
- Around line 1-6: Create a new specs file named
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.num.specs.json
that provides test expectations for the scaling-kept.num script: include a test
case that runs the script, asserts that the `source = @acc with scaling` scales
the input amounts as specified, and that the `destination = { remaining kept }`
preserves the remaining balance (i.e., remaining funds are kept and not sent);
include expected success/failure status, expected amounts sent to BTC/2 and
remaining balance values (or ranges) and any relevant account IDs so the test
harness can validate the behavior of the `with scaling` + `remaining kept`
combination.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.num.specs.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.num
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests
- GitHub Check: Dirty
internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-kept.num
Show resolved
Hide resolved
ascandone
force-pushed
the
feat/asset-scaling
branch
from
1fca1d8 to
437d975
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @internal/interpreter/interpreter.go:
- Around line 574-620: Replace the panic in the SourceWithScaling branch with
proper error return: when s.CachedBalances[*account] is missing, construct and
return an error (e.g., fmt.Errorf or the existing InternalError type) describing
"account balance not found in cache for account %s" (include the account),
rather than panicking; update the surrounding code path in the case for
*parser.SourceWithScaling (which calls evaluateExprAs, uses s.CachedBalances,
findSolution and later trySendingToAccount) to propagate that error to the
caller so execution fails gracefully instead of crashing.
🧹 Nitpick comments (1)
Numscript.g4 (1)
49-49: Minor typo in existing code:maxOvedraftshould bemaxOverdraft.This appears to be pre-existing, but worth noting for a future cleanup.
📝 Suggested fix
- | address = valueExpr colorConstraint? ALLOWING OVERDRAFT UP TO maxOvedraft = valueExpr #srcAccountBoundedOverdraft + | address = valueExpr colorConstraint? ALLOWING OVERDRAFT UP TO maxOverdraft = valueExpr #srcAccountBoundedOverdraft
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-all-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling.num.specs.jsonis excluded by!**/*.jsoninternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (24)
Lexer.g4Numscript.g4internal/analysis/check.gointernal/interpreter/asset_scaling.gointernal/interpreter/asset_scaling_test.gointernal/interpreter/batch_balances_query.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_test.gointernal/interpreter/testdata/script-tests/experimental/scaling/scaling-all-allotment.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling-allotment.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling.numinternal/parser/antlrParser/Lexer.interpinternal/parser/antlrParser/Lexer.tokensinternal/parser/antlrParser/Numscript.interpinternal/parser/antlrParser/Numscript.tokensinternal/parser/antlrParser/lexer.gointernal/parser/antlrParser/numscript_base_listener.gointernal/parser/antlrParser/numscript_listener.gointernal/parser/antlrParser/numscript_parser.gointernal/parser/ast.gointernal/parser/parser.gointernal/parser/parser_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.num
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/analysis/check.go
- internal/parser/antlrParser/numscript_base_listener.go
- internal/interpreter/testdata/script-tests/experimental/scaling/scaling-allotment.num
- internal/parser/parser.go
- internal/interpreter/testdata/script-tests/experimental/scaling/scaling-all-allotment.num
- internal/parser/parser_test.go
- internal/interpreter/interpreter_test.go
- internal/interpreter/asset_scaling_test.go
- internal/interpreter/asset_scaling.go
- internal/interpreter/testdata/script-tests/experimental/scaling/scaling.num
- internal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.num
- internal/parser/antlrParser/Lexer.tokens
- internal/parser/antlrParser/Numscript.tokens
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-04-22T16:45:07.842Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
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.
Applied to files:
Numscript.g4
📚 Learning: 2025-07-16T14:51:51.052Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 70
File: internal/interpreter/accounts_metadata.go:13-24
Timestamp: 2025-07-16T14:51:51.052Z
Learning: The DeepClone method in internal/interpreter/accounts_metadata.go works correctly despite using MapGetOrPutDefault in what initially appears to be a non-assignment context. The fetchAccountMetadata method returns a reference to the map stored in the cloned structure, and MapGetOrPutDefault modifies it in-place, so the cloned values persist correctly.
Applied to files:
internal/interpreter/interpreter.gointernal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-06T17:48:48.962Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-19T11:57:21.654Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 39
File: internal/parser/parser.go:0-0
Timestamp: 2025-02-19T11:57:21.654Z
Learning: Percentage literals in NumScript are inherently bounded as they represent percentages, making overflow checks unnecessary. The uint16 type for floating digits is more than sufficient as it can handle up to 65535 decimal places.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-06T17:48:48.961Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
🧬 Code graph analysis (5)
internal/parser/antlrParser/numscript_listener.go (1)
internal/parser/antlrParser/numscript_parser.go (1)
SrcAccountWithScalingContext(3393-3396)
internal/interpreter/batch_balances_query.go (1)
internal/parser/ast.go (2)
SourceWithScaling(146-146)SourceWithScaling(188-192)
internal/interpreter/interpreter.go (5)
internal/parser/ast.go (3)
SourceWithScaling(146-146)SourceWithScaling(188-192)Source(135-138)internal/interpreter/interpreter_test.go (1)
Posting(225-225)numscript.go (1)
Posting(52-52)internal/interpreter/value.go (2)
Asset(17-17)Asset(31-31)internal/utils/utils.go (1)
MapGetOrPutDefault(50-58)
internal/parser/ast.go (1)
internal/parser/range.go (1)
Range(13-16)
internal/parser/antlrParser/numscript_parser.go (2)
internal/parser/ast.go (5)
ValueExpr(8-11)Source(135-138)Destination(209-212)KeptOrDestination(254-256)SentValue(293-296)internal/parser/antlrParser/numscript_listener.go (1)
NumscriptListener(7-285)
🔇 Additional comments (11)
internal/interpreter/batch_balances_query.go (1)
101-113: LGTM!The new
SourceWithScalingcase correctly follows the established pattern for handling source types. The implementation properly:
- Evaluates the account from
source.Address- Evaluates the optional color
- Batches the query using
assetToScaledAsset(st.CurrentAsset)to query for the scaled asset variantThe error handling is consistent with other cases in this function.
Lexer.g4 (1)
35-36: LGTM!The new
WITHandSCALINGtokens are defined correctly and follow the existing lexer pattern for keyword tokens.internal/parser/antlrParser/numscript_listener.go (1)
88-89: Generated file - changes are consistent with grammar updates.The new
EnterSrcAccountWithScalingandExitSrcAccountWithScalinglistener methods are auto-generated from the Numscript.g4 grammar updates and follow the established ANTLR listener pattern.Also applies to: 226-227
internal/parser/ast.go (1)
188-192: LGTM!The new
SourceWithScalingtype is well-structured with:
Rangefor source location tracking (consistent with other source types)ColorandAddressfields matching the grammar requirementsThe interface implementation at line 146 correctly satisfies the
Sourceinterface.Numscript.g4 (1)
47-55: LGTM!The new
srcAccountWithScalingalternative is well-placed in the grammar:
- Positioned before the generic
srcAccountto ensure specific patterns match first- Follows the same structure as other source alternatives with optional
colorConstraint- The
WITH SCALINGkeyword sequence is clear and unambiguousinternal/parser/antlrParser/Lexer.interp (1)
35-36: Generated file - changes are consistent with Lexer.g4 updates.The new
WITHandSCALINGtokens are correctly reflected in the literal names, symbolic names, rule names, and ATN data.Also applies to: 82-83, 128-129, 151-151
internal/parser/antlrParser/Numscript.interp (1)
35-36: Generated file - changes are consistent with Numscript.g4 updates.The interpreter file correctly reflects:
- New
WITHandSCALINGtokens in literal and symbolic names- Updated ATN with token count increased from 42 to 44 and new parsing states for
srcAccountWithScalingAlso applies to: 82-83, 119-119
internal/interpreter/interpreter.go (1)
54-74: LGTM: Catch-all asset pattern support looks correct.The logic properly handles the
/*suffix for querying all assets under a base asset. The matching conditionk == baseAsset || strings.HasPrefix(k, baseAsset+"/")correctly identifies both the base asset itself and all sub-assets.internal/parser/antlrParser/lexer.go (1)
49-50: LGTM: Generated lexer code for new tokens.The additions of
WITHandSCALINGtokens are correctly integrated into the generated lexer. The token IDs, literal names, symbolic names, and rule names have been properly updated. Since this is ANTLR-generated code, the changes follow the expected patterns.Also applies to: 57-59, 66-68, 311-322
internal/parser/antlrParser/numscript_parser.go (2)
3393-3466: LGTM: New SrcAccountWithScalingContext properly defined.The new parsing context for
WITH SCALINGsyntax follows ANTLR's standard patterns. The struct includes:
- Proper inheritance from
SourceContext- Address field with getter/setter
- Token accessors for
WITHandSCALING- Listener hooks for parse tree traversal
ColorConstraintaccessor for optional color constraintsThe implementation is consistent with other source context types like
SrcAccountBoundedOverdraftContext.
3888-3927: LGTM: Scaling syntax parsing integrated correctly.The new case 3 in the
Source()method properly handles theaddress [colorConstraint] WITH SCALINGsyntax:
- Parses the account address expression
- Handles optional color constraint
- Matches the
WITHandSCALINGtokens- Creates appropriate parsing context
The integration maintains consistency with existing source patterns and properly updates the adaptive prediction logic.
Also applies to: 3768-4112
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/flags/flags.go (1)
13-13: Consider consistent naming for the feature flag constant.Other feature flag constants in this file use suffixes like
FeatureFlagorFlag(e.g.,ExperimentalOverdraftFunctionFeatureFlag,ExperimentalAccountInterpolationFlag), butAssetScalinglacks this suffix. Consider renaming toExperimentalAssetScalingFeatureFlagorAssetScalingFlagfor consistency.Suggested rename
- AssetScaling FeatureFlag = "experimental-asset-scaling" + ExperimentalAssetScalingFeatureFlag FeatureFlag = "experimental-asset-scaling"internal/interpreter/interpreter.go (1)
574-625: Consider extracting common scaling logic into a helper function.The
SourceWithScalinghandling in bothsendAll(lines 574-625) andtrySendingUpTo(lines 736-793) share significant logic: feature flag check, account evaluation, asset scale extraction, solution finding, posting emission, and balance updates. Extracting this into a shared helper would reduce duplication and make future changes easier to maintain.Potential helper signature
// emitScalingPostings handles the common scaling logic for both sendAll and trySendingUpTo. // If requestedAmount is nil, it sends all available scaled balances. func (s *programState) emitScalingPostings( source *parser.SourceWithScaling, requestedAmount *big.Int, ) (*big.Int, InterpreterError)Also applies to: 736-793
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-all-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling.num.specs.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
internal/flags/flags.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-05T11:42:58.472Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
Applied to files:
internal/interpreter/interpreter_test.go
📚 Learning: 2025-07-16T14:51:51.052Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 70
File: internal/interpreter/accounts_metadata.go:13-24
Timestamp: 2025-07-16T14:51:51.052Z
Learning: The DeepClone method in internal/interpreter/accounts_metadata.go works correctly despite using MapGetOrPutDefault in what initially appears to be a non-assignment context. The fetchAccountMetadata method returns a reference to the map stored in the cloned structure, and MapGetOrPutDefault modifies it in-place, so the cloned values persist correctly.
Applied to files:
internal/interpreter/interpreter.go
📚 Learning: 2025-07-16T14:50:12.931Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 70
File: internal/utils/pretty_csv.go:36-40
Timestamp: 2025-07-16T14:50:12.931Z
Learning: ascandone prefers using panics for violated function preconditions rather than explicit error handling. In the CsvPretty function in internal/utils/pretty_csv.go, panicking when row[fieldIndex] is out of bounds is intentional behavior for enforcing the precondition that headers must not be shorter than any rows.
Applied to files:
internal/interpreter/interpreter.go
🧬 Code graph analysis (1)
internal/interpreter/interpreter_test.go (4)
internal/interpreter/interpreter.go (4)
StaticStore(35-38)Balances(25-25)AccountBalance(24-24)BalanceQuery(19-19)numscript.go (4)
StaticStore(70-70)Balances(58-58)AccountBalance(57-57)BalanceQuery(55-55)internal/interpreter/interpreter_error.go (2)
InvalidUnboundedAddressInScalingAddress(259-261)InvalidUnboundedAddressInScalingAddress(263-265)internal/flags/flags.go (1)
AssetScaling(13-13)
🔇 Additional comments (7)
internal/interpreter/interpreter_test.go (3)
157-189: LGTM!The "request currencies" subtest is well-structured and properly tests the
GetBalancesfunctionality with explicit currency queries.
191-214: LGTM!The "assets catchall" subtest properly validates the new wildcard asset pattern matching (
USD/*), ensuring all matching balances are returned.
1144-1159: LGTM!The test correctly validates that using
@world(an unbounded address) with thewith scalingsyntax produces the expectedInvalidUnboundedAddressInScalingAddresserror when theAssetScalingfeature flag is enabled.internal/interpreter/interpreter.go (4)
55-74: LGTM!The catchall pattern implementation correctly handles the
/*suffix for wildcard asset matching. The logic properly matches both the exact base asset and any scaled variants (e.g.,USD/*matchesUSD,USD/2,USD/3), while correctly excluding unrelated assets (e.g.,USDC).
759-763: Good defensive fallback for insufficient funds.The fallback to the standard non-scaled path when the requested amount exceeds the total available ensures the user gets a meaningful error message rather than a confusing scaling-related failure.
5-5: LGTM!The
fmtimport is appropriately added to support thefmt.Sprintfcalls used for constructing the intermediate scaling account addresses.
597-611: No issue found. The code is safe.At line 610,
acc[asset]is guaranteed to be non-nil. ThegetAssetsfunction extracts scales only from existing assets in the balance map, andbuildScaledAssetcorrectly reconstructs the original asset names. For scale 0, it returns the base asset; for other scales, it returns"baseAsset/scale". These reconstructed names exactly match the original keys inacc, so the map lookup always succeeds.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @internal/interpreter/interpreter.go:
- Around line 592-594: The error returned uses
InvalidUnboundedAddressInScalingAddress but doesn't set its Range field;
construct and return the error with Range populated from the AST node that
represents the scaling address (e.g., use scalingAddr.Range() or addr.Range() or
the local node variable that holds the address) so the error includes the source
location: return nil, InvalidUnboundedAddressInScalingAddress{Range:
<scaling-address-node>.Range()} replacing <scaling-address-node> with the actual
variable in scope.
- Around line 757-760: The error returned when a cached balance for an address
is missing (the InvalidUnboundedAddressInScalingAddress constructed in the block
that checks s.CachedBalances[*account]) lacks the Range field; update the error
construction to include the appropriate range info (populate the Range field
with the unbounded address/range represented by account, e.g. set Range:
*account or the variable that holds the address range) so the error mirrors the
fix applied in the sendAll case.
🧹 Nitpick comments (1)
internal/parser/parser_test.go (1)
504-512: LGTM!The test follows the established pattern and validates basic parsing of the new scaling syntax. The snapshot testing approach is consistent with other tests in the file.
Consider adding additional parser tests for edge cases such as:
- Variable swap account:
$src with scaling through $swap- Color restriction combined with scaling:
@src \ "color" with scaling through @swap- Scaling with capped source:
max [EUR/2 50] from @src with scaling through @swapThese could help ensure the grammar handles all valid combinations correctly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-all-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scaling/scaling.num.specs.jsonis excluded by!**/*.jsoninternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (21)
Lexer.g4Numscript.g4internal/analysis/check.gointernal/interpreter/batch_balances_query.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_test.gointernal/interpreter/testdata/script-tests/experimental/scaling/scaling-all-allotment.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling-allotment.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.numinternal/interpreter/testdata/script-tests/experimental/scaling/scaling.numinternal/parser/antlrParser/Lexer.interpinternal/parser/antlrParser/Lexer.tokensinternal/parser/antlrParser/Numscript.interpinternal/parser/antlrParser/Numscript.tokensinternal/parser/antlrParser/lexer.gointernal/parser/antlrParser/numscript_parser.gointernal/parser/ast.gointernal/parser/parser.gointernal/parser/parser_test.gonumscript
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/analysis/check.go
- internal/interpreter/testdata/script-tests/experimental/scaling/scaling.num
- internal/interpreter/testdata/script-tests/experimental/scaling/scaling-kept.num
- internal/interpreter/testdata/script-tests/experimental/scaling/scaling-all-allotment.num
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2024-12-05T11:42:58.472Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
Applied to files:
internal/interpreter/interpreter_test.go
📚 Learning: 2025-01-20T23:01:48.857Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/interpreter/interpreter.go:479-488
Timestamp: 2025-01-20T23:01:48.857Z
Learning: In the `sendAll` method of the Numscript interpreter, the `oneof` operator only uses the first source because none of the branches can fail in a "send all" context. This differs from `trySendingUpTo` where backtracking is needed as sources can fail when trying to send a specific amount.
Applied to files:
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.num
📚 Learning: 2025-07-16T14:51:51.052Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 70
File: internal/interpreter/accounts_metadata.go:13-24
Timestamp: 2025-07-16T14:51:51.052Z
Learning: The DeepClone method in internal/interpreter/accounts_metadata.go works correctly despite using MapGetOrPutDefault in what initially appears to be a non-assignment context. The fetchAccountMetadata method returns a reference to the map stored in the cloned structure, and MapGetOrPutDefault modifies it in-place, so the cloned values persist correctly.
Applied to files:
internal/interpreter/interpreter.gointernal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-07-16T14:50:12.931Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 70
File: internal/utils/pretty_csv.go:36-40
Timestamp: 2025-07-16T14:50:12.931Z
Learning: ascandone prefers using panics for violated function preconditions rather than explicit error handling. In the CsvPretty function in internal/utils/pretty_csv.go, panicking when row[fieldIndex] is out of bounds is intentional behavior for enforcing the precondition that headers must not be shorter than any rows.
Applied to files:
internal/interpreter/interpreter.go
📚 Learning: 2025-04-22T16:45:07.842Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
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.
Applied to files:
Numscript.g4
📚 Learning: 2025-02-06T17:48:48.962Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-19T11:57:21.654Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 39
File: internal/parser/parser.go:0-0
Timestamp: 2025-02-19T11:57:21.654Z
Learning: Percentage literals in NumScript are inherently bounded as they represent percentages, making overflow checks unnecessary. The uint16 type for floating digits is more than sufficient as it can handle up to 65535 decimal places.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-06T17:48:48.961Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
🧬 Code graph analysis (6)
internal/parser/parser.go (3)
internal/parser/antlrParser/numscript_parser.go (1)
SrcAccountWithScalingContext(3395-3399)internal/parser/ast.go (2)
SourceWithScaling(146-146)SourceWithScaling(188-193)internal/parser/range.go (1)
Range(13-16)
internal/parser/parser_test.go (3)
internal/parser/parser.go (1)
Parse(50-81)numscript.go (2)
Parse(40-42)Value(72-72)internal/interpreter/value.go (1)
Value(11-14)
internal/interpreter/batch_balances_query.go (1)
internal/parser/ast.go (2)
SourceWithScaling(146-146)SourceWithScaling(188-193)
internal/interpreter/interpreter.go (4)
internal/parser/ast.go (3)
SourceWithScaling(146-146)SourceWithScaling(188-193)Source(135-138)internal/flags/flags.go (1)
AssetScaling(13-13)internal/interpreter/interpreter_error.go (2)
InvalidUnboundedAddressInScalingAddress(259-261)InvalidUnboundedAddressInScalingAddress(263-265)internal/utils/utils.go (1)
MapGetOrPutDefault(50-58)
internal/parser/ast.go (1)
internal/parser/range.go (1)
Range(13-16)
internal/parser/antlrParser/numscript_parser.go (2)
internal/parser/ast.go (5)
ValueExpr(8-11)Source(135-138)Destination(210-213)KeptOrDestination(255-257)SentValue(294-297)internal/parser/antlrParser/numscript_listener.go (1)
NumscriptListener(7-285)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (20)
internal/interpreter/testdata/script-tests/experimental/scaling/scaling-send-all.num (1)
1-4: LGTM!The test script correctly exercises the new
with scaling throughsyntax combined with the "send all" (*) operator. This provides coverage for the scaling feature in the unbounded send context.internal/interpreter/testdata/script-tests/experimental/scaling/scaling-allotment.num (1)
1-7: LGTM!Good test case that exercises a mixed inorder source block containing both a plain account (
@acc1) and a scaling source (@acc2 with scaling through @swap). This validates the interpreter's handling of heterogeneous source types.Lexer.g4 (1)
35-37: LGTM!The new lexer tokens
WITH,SCALING, andTHROUGHfollow the existing naming convention and enable the new scaling syntax. The lowercase literals are consistent with other keywords in the grammar.internal/interpreter/batch_balances_query.go (1)
101-114: Swap account balance handling is correct.The
source.Throughfield (swap account) is intentionally not evaluated in the batch query. This is proper design—the batch query validates the source account balance for the scaled asset, while the actual send operations ininterpreter.go(lines 574 and 741) properly evaluate and validate the swap account when needed.internal/parser/parser.go (1)
240-246: LGTM! The newSrcAccountWithScalingContextcase is well-integrated.The implementation correctly follows the established pattern for source parsing, consistent with adjacent cases like
SrcAccountUnboundedOverdraft. The field mapping (GetSwap()→Through) aligns with the grammar definition inNumscript.g4whereswap=valueExpris used.internal/parser/ast.go (2)
140-146: LGTM! Interface implementation is correct.The
source()marker method properly registersSourceWithScalingas a validSourcetype. The formatting cleanup improves consistency across all source type declarations.
187-193: LGTM! TheSourceWithScalingstruct is well-designed.The struct correctly:
- Embeds
Rangeto satisfy theGetRange()requirement of theSourceinterface- Uses
ValueExprforColor,Address, andThroughfields, consistent with similar types likeSourceOverdraft- Follows the established naming and field conventions in this file
internal/parser/antlrParser/Lexer.interp (1)
35-37: LGTM! Generated lexer tokens are correctly added.The new
WITH,SCALING, andTHROUGHtokens are properly defined across literal names, symbolic names, and rule names sections. This is a generated ANTLR artifact that correctly reflects the grammar changes.Also applies to: 83-85, 130-132
internal/interpreter/interpreter_test.go (1)
156-216: LGTM! Good refactoring ofTestStaticStoreinto subtests.The restructuring improves test organization:
- "request currencies" validates targeted balance queries
- "assets catchall" tests the
USD/*wildcard pattern correctly returning all USD-prefixed assetsBoth subtests properly exercise
GetBalanceswith distinct scenarios.Numscript.g4 (1)
47-55: LGTM! The grammar extension for scaling is well-structured.The
srcAccountWithScalingalternative is correctly positioned:
- After the more specific overdraft rules (which have
ALLOWINGkeyword)- Before the generic
srcAccountrule (which would otherwise match first)This ordering ensures the parser correctly distinguishes
@addr with scaling through @swapfrom a plain@addrsource. The optionalcolorConstraint?maintains consistency with other source alternatives.internal/parser/antlrParser/Numscript.interp (1)
1-121: Auto-generated file changes look consistent.This file is generated from the Numscript.g4 grammar. The new tokens WITH, SCALING, and THROUGH are correctly added and the ATN data is updated accordingly.
internal/interpreter/interpreter.go (3)
54-74: LGTM: Catch-all asset pattern matching logic.The implementation correctly identifies assets matching the base asset or its scaled variants (baseAsset/N pattern). The use of
strings.CutSuffixandstrings.HasPrefixis appropriate.
768-772: Good fallback for better error messages.The fallback to the non-scaled path when
amount > totalis a thoughtful UX decision - it provides a more informative error message about insufficient funds rather than a generic scaling error.
608-616: No issues found. The nil pointer dereference concern is invalid. The contract betweengetAssetsandfindSolutionguarantees that reconstructed assets will exist inacc:
getAssetsonly extracts scales from assets already present inaccthat match thebaseAssetprefixfindSolutionreturns only scales from the input map keys (no new scales are generated)buildScaledAssetdeterministically reconstructs asset names, soacc[asset]will always reference a valid entryThe
.Sub()call on line 615 is safe.internal/parser/antlrParser/Numscript.tokens (1)
33-78: Auto-generated token file changes are consistent.The new tokens (WITH, SCALING, THROUGH) are correctly added and existing tokens are properly renumbered. The literal mappings are consistent with the token definitions.
internal/parser/antlrParser/lexer.go (1)
1-328: Auto-generated lexer file.This file is generated from Lexer.g4 by ANTLR 4.13.2. The changes correctly add the new WITH, SCALING, and THROUGH tokens and update all related data structures (LiteralNames, SymbolicNames, RuleNames, serializedATN, and token constants).
internal/parser/antlrParser/Lexer.tokens (1)
33-78: Auto-generated token file is consistent.Token definitions match the other generated files and the grammar changes.
internal/parser/antlrParser/numscript_parser.go (3)
3395-3502: Auto-generated parser context for scaling syntax.The new
SrcAccountWithScalingContextfollows the established ANTLR pattern with proper fields, accessors, and listener hooks. This is generated code consistent with the grammar definition.
3924-3978: Auto-generated parsing logic for scaling syntax.The new case 3 in the Source() parser correctly handles the
address [color] with scaling through swapsyntax pattern. The generated code properly createsSrcAccountWithScalingContextand binds the address and swap expressions.
252-264: Token constants correctly updated.The new tokens (WITH, SCALING, THROUGH) are properly defined with their numeric values, and subsequent tokens are correctly renumbered.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/analysis/hover_test.go:
- Around line 545-552: The test is missing a nil check on hover before the type
assertion and has reversed require.Equal arguments; first assert hover != nil
(e.g., fail with a clear message consistent with other tests) before doing the
type assertion to avoid a panic, then perform the type assertion to
*analysis.VariableHover and use require.Equal(t, "x", varHover.Node.Name) so the
expected and actual are in the correct order; reference the hover variable, the
VariableHover type, varHover.Node.Name, and the require.Equal call when making
the changes.
🧹 Nitpick comments (1)
internal/analysis/hover_test.go (1)
527-554: Consider a more descriptive test name and additional assertions.The test name
TestHoveris quite generic compared to other tests (e.g.,TestHoverOnSource,TestHoverOnBoundedCap). A name likeTestHoverOnSourceWithScalingwould better document what's being tested.Additionally, for consistency with other hover tests, consider verifying:
- The hover range matches
rng- The variable resolves correctly via
CheckProgram(...).ResolveVar(...)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/analysis/hover_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/analysis/hover_test.go (3)
internal/parser/range.go (1)
RangeOfIndexed(155-164)internal/parser/parser.go (1)
Parse(50-81)internal/analysis/hover.go (3)
HoverOn(32-70)VariableHover(10-13)VariableHover(15-15)
| hover := analysis.HoverOn(program, rng.Start) | ||
|
|
||
| varHover, ok := hover.(*analysis.VariableHover) | ||
| if !ok { | ||
| t.Fatalf("Expected a VariableHover") | ||
| } | ||
|
|
||
| require.Equal(t, varHover.Node.Name, "x") |
There was a problem hiding this comment.
Missing nil check and incorrect require.Equal argument order.
- A nil check on
hoverbefore the type assertion would prevent a potential panic and provide a clearer failure message, consistent with all other tests in this file. - The
require.Equalarguments are reversed—testify convention is(t, expected, actual).
Proposed fix
rng := parser.RangeOfIndexed(input, "$x", 1)
program := parser.Parse(input).Value
hover := analysis.HoverOn(program, rng.Start)
+ require.NotNil(t, hover)
varHover, ok := hover.(*analysis.VariableHover)
if !ok {
t.Fatalf("Expected a VariableHover")
}
- require.Equal(t, varHover.Node.Name, "x")
+ require.Equal(t, "x", varHover.Node.Name)📝 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.
| hover := analysis.HoverOn(program, rng.Start) | |
| varHover, ok := hover.(*analysis.VariableHover) | |
| if !ok { | |
| t.Fatalf("Expected a VariableHover") | |
| } | |
| require.Equal(t, varHover.Node.Name, "x") | |
| hover := analysis.HoverOn(program, rng.Start) | |
| require.NotNil(t, hover) | |
| varHover, ok := hover.(*analysis.VariableHover) | |
| if !ok { | |
| t.Fatalf("Expected a VariableHover") | |
| } | |
| require.Equal(t, "x", varHover.Node.Name) |
🤖 Prompt for AI Agents
In @internal/analysis/hover_test.go around lines 545 - 552, The test is missing
a nil check on hover before the type assertion and has reversed require.Equal
arguments; first assert hover != nil (e.g., fail with a clear message consistent
with other tests) before doing the type assertion to avoid a panic, then perform
the type assertion to *analysis.VariableHover and use require.Equal(t, "x",
varHover.Node.Name) so the expected and actual are in the correct order;
reference the hover variable, the VariableHover type, varHover.Node.Name, and
the require.Equal call when making the changes.
e382860 to
2c069d4
Compare
ascandone
force-pushed
the
feat/asset-scaling
branch
from
2c069d4 to
f1e631e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/interpreter/interpreter.go:
- Line 615: getAssets has overly-broad filtering using strings.HasPrefix which
can match assets like "EUROS" when baseAsset is "EUR" and lead to a nil
dereference when buildScaledAsset reconstructs "EUR"; update getAssets to only
accept exact matches (asset == baseAsset) or scaled forms that start with
baseAsset + "/" and then call getAssetScale(asset) to extract the scale and
store amounts into result[scale]; reference getAssets, getAssetScale and
buildScaledAsset when making the change so the acc/result map keys align with
how buildScaledAsset reconstructs asset names.
🧹 Nitpick comments (2)
internal/parser/parser_test.go (1)
505-512: LGTM!Good addition of a parser test for the new scaling syntax. The test follows the established pattern using snapshot testing.
Consider additional test coverage (optional):
- Scaling with a variable source:
$src with scaling through @swap- Scaling with a variable swap account:
@src with scaling through $swap- Scaling within nested structures (inorder, oneof) at the parser level
internal/interpreter/interpreter.go (1)
574-630: Consider extracting common scaling logic to reduce duplication.The scaling implementations in
sendAll(lines 574-630) andtrySendingUpTo(lines 741-802) share significant logic:
- Feature flag check
- Account evaluation
- Balance lookup and validation
- Posting generation for scaling steps
- Balance updates
A helper function could consolidate the common parts, improving maintainability.
♻️ Suggested refactor direction
Consider extracting a helper like:
func (s *programState) executeScalingPostings( account string, scalingAccount string, solution []scalingPair, baseAsset string, acc AccountBalance, ) { for _, pair := range solution { asset := buildScaledAsset(baseAsset, pair.scale) s.Postings = append(s.Postings, Posting{ Source: account, Destination: fmt.Sprintf("%s:%s", account, scalingAccount), Amount: new(big.Int).Set(pair.amount), Asset: asset, }) acc[asset].Sub(acc[asset], pair.amount) } }This would reduce duplication while keeping the different flow control in
sendAllvstrySendingUpTo.Also applies to: 741-802
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (7)
internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-all-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-with-oneof.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling.num.specs.jsonis excluded by!**/*.jsoninternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (23)
Lexer.g4Numscript.g4internal/analysis/check.gointernal/analysis/hover.gointernal/analysis/hover_test.gointernal/interpreter/batch_balances_query.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_test.gointernal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-all-allotment.numinternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-allotment.numinternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-kept.numinternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-send-all.numinternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-with-oneof.numinternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling.numinternal/parser/antlrParser/Lexer.interpinternal/parser/antlrParser/Lexer.tokensinternal/parser/antlrParser/Numscript.interpinternal/parser/antlrParser/Numscript.tokensinternal/parser/antlrParser/lexer.gointernal/parser/antlrParser/numscript_parser.gointernal/parser/ast.gointernal/parser/parser.gointernal/parser/parser_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-allotment.num
- internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-kept.num
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/interpreter/batch_balances_query.go
- internal/analysis/hover.go
- internal/parser/parser.go
- internal/analysis/check.go
- internal/analysis/hover_test.go
- Lexer.g4
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2024-12-05T11:42:58.472Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 27
File: internal/interpreter/interpreter.go:667-668
Timestamp: 2024-12-05T11:42:58.472Z
Learning: In Go test files within the `internal/interpreter` package (e.g., `reconciler_test.go` and `interpreter_test.go`), it's acceptable to use hardcoded `"<kept>"` strings in test data and comments, and they do not need to be replaced with the `KEPT_ADDR` constant.
Applied to files:
internal/interpreter/interpreter_test.go
📚 Learning: 2025-02-06T17:48:48.961Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.961Z
Learning: Edge cases for oneof destination parsing in numscript are covered by the existing inorder parsing fault tolerance tests, as both constructs share similar parsing logic.
Applied to files:
internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-with-oneof.numinternal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-01-20T23:01:48.857Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/interpreter/interpreter.go:479-488
Timestamp: 2025-01-20T23:01:48.857Z
Learning: In the `sendAll` method of the Numscript interpreter, the `oneof` operator only uses the first source because none of the branches can fail in a "send all" context. This differs from `trySendingUpTo` where backtracking is needed as sources can fail when trying to send a specific amount.
Applied to files:
internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-with-oneof.num
📚 Learning: 2025-07-16T14:51:51.052Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 70
File: internal/interpreter/accounts_metadata.go:13-24
Timestamp: 2025-07-16T14:51:51.052Z
Learning: The DeepClone method in internal/interpreter/accounts_metadata.go works correctly despite using MapGetOrPutDefault in what initially appears to be a non-assignment context. The fetchAccountMetadata method returns a reference to the map stored in the cloned structure, and MapGetOrPutDefault modifies it in-place, so the cloned values persist correctly.
Applied to files:
internal/interpreter/interpreter.go
📚 Learning: 2025-07-16T14:50:12.931Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 70
File: internal/utils/pretty_csv.go:36-40
Timestamp: 2025-07-16T14:50:12.931Z
Learning: ascandone prefers using panics for violated function preconditions rather than explicit error handling. In the CsvPretty function in internal/utils/pretty_csv.go, panicking when row[fieldIndex] is out of bounds is intentional behavior for enforcing the precondition that headers must not be shorter than any rows.
Applied to files:
internal/interpreter/interpreter.go
📚 Learning: 2025-04-22T16:45:07.842Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 55
File: internal/parser/antlrParser/numscript_parser.go:2953-2967
Timestamp: 2025-04-22T16:45:07.842Z
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.
Applied to files:
Numscript.g4
📚 Learning: 2025-02-06T17:48:48.962Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 31
File: internal/parser/parser_test.go:115-126
Timestamp: 2025-02-06T17:48:48.962Z
Learning: The numscript parser's fault tolerance is built into the infrastructure and handles errors gracefully for all clause types. Tests in parser_fault_tolerance_test.go cover both inorder and oneof constructs as they share the same parsing infrastructure.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
📚 Learning: 2025-02-19T11:57:21.654Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 39
File: internal/parser/parser.go:0-0
Timestamp: 2025-02-19T11:57:21.654Z
Learning: Percentage literals in NumScript are inherently bounded as they represent percentages, making overflow checks unnecessary. The uint16 type for floating digits is more than sufficient as it can handle up to 65535 decimal places.
Applied to files:
internal/parser/antlrParser/numscript_parser.go
🧬 Code graph analysis (5)
internal/parser/parser_test.go (3)
internal/parser/parser.go (1)
Parse(50-81)numscript.go (2)
Parse(40-42)Value(72-72)internal/interpreter/value.go (1)
Value(11-14)
internal/interpreter/interpreter_test.go (4)
internal/interpreter/interpreter_error.go (2)
InvalidUnboundedAddressInScalingAddress(259-261)InvalidUnboundedAddressInScalingAddress(263-265)internal/parser/range.go (1)
Range(13-16)internal/interpreter/interpreter.go (4)
StaticStore(35-38)Balances(25-25)AccountBalance(24-24)BalanceQuery(19-19)internal/flags/flags.go (1)
AssetScaling(13-13)
internal/interpreter/interpreter.go (6)
internal/parser/ast.go (4)
SourceWithScaling(146-146)SourceWithScaling(188-193)Source(135-138)Destination(210-213)internal/flags/flags.go (1)
AssetScaling(13-13)internal/interpreter/interpreter_error.go (2)
InvalidUnboundedAddressInScalingAddress(259-261)InvalidUnboundedAddressInScalingAddress(263-265)internal/parser/range.go (1)
Range(13-16)internal/interpreter/value.go (2)
Asset(17-17)Asset(31-31)internal/utils/utils.go (1)
MapGetOrPutDefault(50-58)
internal/parser/antlrParser/numscript_parser.go (1)
internal/parser/ast.go (5)
ValueExpr(8-11)Source(135-138)Destination(210-213)KeptOrDestination(255-257)SentValue(294-297)
internal/parser/ast.go (1)
internal/parser/range.go (1)
Range(13-16)
🔇 Additional comments (23)
internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling.num (1)
1-4: LGTM!This test file correctly exercises the new
with scaling throughsyntax for basic asset scaling scenarios.internal/parser/antlrParser/Numscript.interp (1)
35-37: LGTM!The new token definitions (
with,scaling,through/WITH,SCALING,THROUGH) are correctly added to the interpreter file, consistent with the grammar changes.Also applies to: 83-85
Numscript.g4 (1)
47-55: Verify:srcAccountWithScalinglackscolorConstraint?unlike other address alternatives.The other address-based source alternatives (
srcAccountUnboundedOverdraft,srcAccountBoundedOverdraft,srcAccount) all include an optionalcolorConstraint?, butsrcAccountWithScalingdoes not. Please confirm this is intentional (i.e., scaling and color restriction are mutually exclusive by design) rather than an oversight.If color constraints should be supported with scaling, consider:
- | address = valueExpr WITH SCALING THROUGH swap=valueExpr # srcAccountWithScaling + | address = valueExpr colorConstraint? WITH SCALING THROUGH swap=valueExpr # srcAccountWithScalinginternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-all-allotment.num (1)
1-7: LGTM!Good test coverage for scaling within an inorder source block, verifying that scaling can be applied selectively to individual accounts within composite source structures.
internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-with-oneof.num (1)
1-10: LGTM!Good test coverage for scaling within a
oneofsource structure. This correctly tests the backtracking behavior when a scaled source might not satisfy the required amount.internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-send-all.num (1)
1-4: LGTM!Good test coverage for the send-all (
*) scenario with asset scaling.internal/parser/antlrParser/Lexer.interp (1)
35-37: LGTM - Auto-generated lexer updates for new tokens.The new
WITH,SCALING, andTHROUGHtokens are correctly added to the token literal names, symbolic names, rule names, and ATN data. This file appears to be auto-generated fromLexer.g4.Also applies to: 83-85, 130-132, 154-154
internal/interpreter/interpreter_test.go (3)
64-66: LGTM - Error normalization extended for new error type.The new case for
InvalidUnboundedAddressInScalingAddressfollows the established pattern of resetting theRangefield for consistent test comparisons.
160-217: LGTM - Good refactoring to subtests.Splitting
TestStaticStoreinto focused subtests improves test isolation and readability. The "assets catchall" subtest properly validates the new/*wildcard asset pattern matching behavior.
1147-1162: LGTM - Test validates scaling context restrictions.The test correctly verifies that using
@worldin a scaling context produces the expectedInvalidUnboundedAddressInScalingAddresserror, and properly uses theAssetScalingfeature flag.internal/parser/ast.go (2)
140-146: LGTM - Source interface implementation extended.The
source()marker method is correctly added forSourceWithScaling, following the established interface implementation pattern for other source types.
187-193: LGTM - Well-structured AST node for scaling sources.The
SourceWithScalingstruct follows the established pattern (similar toSourceOverdraft) with embeddedRangefor interface compliance andValueExprfields for flexibility in accepting variables or literals.internal/parser/antlrParser/Numscript.tokens (1)
33-45: LGTM - Auto-generated token mappings updated correctly.The new
WITH,SCALING, andTHROUGHtokens are assigned sequential IDs (33-35), and existing tokens are correctly renumbered. The literal mappings are properly updated.Also applies to: 74-78
internal/interpreter/interpreter.go (4)
5-5: LGTM - Import added for fmt usage.The
fmtimport is needed forfmt.Sprintfin the scaling account destination formatting.
55-74: LGTM - Catchall asset pattern implementation.The
/*suffix pattern correctly matches both the base asset (e.g.,USD) and scaled variants (e.g.,USD/2,USD/3). The implementation properly creates newbig.Intcopies to avoid aliasing issues between the source and output maps.
741-802: LGTM - Scaling logic in trySendingUpTo with graceful fallback.The implementation correctly:
- Checks feature flag
- Computes scaling solution for the requested amount
- Falls back to standard error path (line 771) when insufficient funds
- Emits scaling postings and updates balances
The same observation about
acc[asset]access (line 786) applies here as noted above.
574-630: Verify thattrySendingToAccountdoesn't double-send the scaled amount.The implementation emits postings for the scaling operations (lines 609-616), updates the account's
CurrentAssetbalance (lines 624-627), and then callstrySendingToAccount(source.Address, totSent, big.NewInt(0), source.Color)(line 629).The concern is whether the balance update and the
trySendingToAccountcall might result in double accounting. Specifically:
- Are the postings emitted just for record-keeping, or do they directly deduct from the account balance?
- Does
trySendingToAccountread the freshly updated balance, or does it use theamountparameter independently?- Are there tests confirming that the final posting amount equals the sum of converted amounts?
This requires examining the
trySendingToAccountimplementation and the test coverage to ensure the scaling path does not double-deduct from the account.internal/parser/antlrParser/lexer.go (1)
315-327: LGTM - Generated lexer code with new tokens.This is ANTLR-generated code properly introducing the
WITH,SCALING, andTHROUGHtokens (values 33-35) with consistent renumbering of subsequent tokens. The generated structure follows the expected ANTLR lexer pattern.internal/parser/antlrParser/Lexer.tokens (2)
33-45: LGTM - Token definitions consistent with lexer.The new tokens
WITH,SCALING, andTHROUGHare correctly defined with values 33-35, and all subsequent tokens are properly renumbered. The token-to-value mappings are consistent with the generated lexer.
74-78: LGTM - Literal mappings correctly updated.The literal-to-token mappings for the new keywords and the shifted token positions for
@and:are accurate.internal/parser/antlrParser/numscript_parser.go (3)
250-262: LGTM - Parser token constants consistent with lexer.The new token constants
NumscriptParserWITH=33,NumscriptParserSCALING=34, andNumscriptParserTHROUGH=35match the lexer definitions, and subsequent tokens are properly renumbered.
3907-3946: LGTM - Source() rule correctly integrates new scaling alternative.The new case 3 in the
Source()method properly:
- Creates
SrcAccountWithScalingContext- Parses the
addressvalue expression- Matches the
WITH,SCALING,THROUGHtoken sequence- Parses the
swapvalue expressionThe parsing flow correctly handles the new
<address> with scaling through <swap>syntax.
3393-3484: LGTM - New SrcAccountWithScalingContext follows established patterns.The new parsing context is well-structured:
- Contains
addressandswapfields of typeIValueExprContext- Implements proper getter/setter methods
- Provides token accessors for
WITH(),SCALING(),THROUGH()- Implements
EnterRule/ExitRulefor listener integrationThe corresponding listener methods
EnterSrcAccountWithScalingandExitSrcAccountWithScalingare properly defined in theNumscriptListenerinterface and base listener implementation.
| Amount: new(big.Int).Set(convAmt), | ||
| Asset: asset, | ||
| }) | ||
| acc[asset].Sub(acc[asset], convAmt) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and get context around line 615
git ls-files | grep -E "interpreter\.go$"Repository: formancehq/numscript
Length of output: 99
🏁 Script executed:
# Get context around line 615
head -n 650 internal/interpreter/interpreter.go | tail -n 60Repository: formancehq/numscript
Length of output: 1737
🏁 Script executed:
# Find and examine getAssets function
rg -A 15 "func getAssets" internal/interpreter/Repository: formancehq/numscript
Length of output: 1011
🏁 Script executed:
# Find and examine findSolution function
rg -A 30 "func findSolution" internal/interpreter/Repository: formancehq/numscript
Length of output: 1960
🏁 Script executed:
# Find and examine buildScaledAsset function
rg -A 10 "func buildScaledAsset" internal/interpreter/Repository: formancehq/numscript
Length of output: 792
🏁 Script executed:
# Get full getAssetScale implementation to confirm the inverse
rg -A 15 "func getAssetScale" internal/interpreter/asset_scaling.goRepository: formancehq/numscript
Length of output: 498
🏁 Script executed:
# Find where baseAsset is defined in the sendAll function
rg -B 20 "getAssets\(acc, baseAsset\)" internal/interpreter/interpreter.go | head -40Repository: formancehq/numscript
Length of output: 1014
🏁 Script executed:
# Check if there are any test cases that might reveal if prefix collisions are possible
fd -e go -path "*/test*" -o -path "*_test.go" internal/interpreter/ | head -5Repository: formancehq/numscript
Length of output: 232
🏁 Script executed:
# Find test files related to asset scaling
find internal/interpreter -name "*test.go" | head -10Repository: formancehq/numscript
Length of output: 387
🏁 Script executed:
# Examine the asset scaling tests to understand expected behavior
cat internal/interpreter/asset_scaling_test.goRepository: formancehq/numscript
Length of output: 2462
Fix the asset filtering logic in getAssets to prevent nil dereference.
Line 615 can panic because getAssets uses strings.HasPrefix(asset, baseAsset) to filter assets, which is too broad. If the balance contains assets like "EUROS" while baseAsset is "EUR", the filter will match unintended assets. When getAssetScale("EUROS") is called, it returns ("EUROS", 0) since there's no "/" separator. Later, buildScaledAsset("EUR", 0) reconstructs "EUR", not "EUROS", leaving acc["EUR"] potentially nil and causing a panic.
The fix should be to validate that matched assets actually have the expected structure:
func getAssets(balance AccountBalance, baseAsset string) map[int64]*big.Int {
result := make(map[int64]*big.Int)
for asset, amount := range balance {
if asset == baseAsset {
result[0] = amount
} else if strings.HasPrefix(asset, baseAsset + "/") {
_, scale := getAssetScale(asset)
result[scale] = amount
}
}
return result
}
This ensures only exact base asset matches or assets with proper scale separators are included.
🤖 Prompt for AI Agents
In @internal/interpreter/interpreter.go at line 615, getAssets has overly-broad
filtering using strings.HasPrefix which can match assets like "EUROS" when
baseAsset is "EUR" and lead to a nil dereference when buildScaledAsset
reconstructs "EUR"; update getAssets to only accept exact matches (asset ==
baseAsset) or scaled forms that start with baseAsset + "/" and then call
getAssetScale(asset) to extract the scale and store amounts into result[scale];
reference getAssets, getAssetScale and buildScaledAsset when making the change
so the acc/result map keys align with how buildScaledAsset reconstructs asset
names.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/interpreter/interpreter.go:
- Around line 601-615: The loop that builds postings uses buildScaledAsset to
derive asset and then calls acc[asset].Sub(acc[asset], convAmt) which can
nil-panic if acc lacks that key; replace that direct map access with a defensive
retrieval (use MapGetOrPutDefault or a check like ok := acc[asset]; if !ok {
acc[asset] = NewZeroBalance(); } ) before calling Sub, or use the existing
MapGetOrPutDefault helper to obtain a non-nil balance for the derived asset;
update the code around the Posting append and subsequent Sub call to
fetch/ensure acc[asset] safely (referencing convAmt.amount/scale,
buildScaledAsset, s.Postings append, and the acc map).
🧹 Nitpick comments (3)
internal/interpreter/interpreter.go (3)
617-628: Complex flow - consider adding explanatory comments.The scaling flow is sophisticated:
- Move scaled assets from
accounttoscalingAccount- Credit
totSentback toaccountin the target scale- Send
totSentthrough normal flow viatrySendingToAccountThe logic appears correct, but the interplay between direct posting emission and the normal send flow could benefit from inline comments explaining the invariants being maintained.
767-771: Clever fallback for error messages, but add a comment.The fallback to non-scaled mode when
amount > totalis a smart way to reuse existing error handling (MissingFundsErr). However, this intent isn't immediately obvious. Consider adding a comment explaining that this delegation is purely for generating a user-friendly error message.📝 Suggested comment
if amount.Cmp(total) == 1 { - // we already know we are failing, but we're delegating to the "standard" (non-scaled) mode - // so that we get a somewhat helpful (although limited) error message + // Insufficient funds: delegate to standard (non-scaled) mode to generate + // a MissingFundsErr with the available vs. needed amounts. return s.trySendingToAccount(source.Address, amount, big.NewInt(0), source.Color) }
573-629: Consider extracting shared scaling logic.The
SourceWithScalinghandling in bothsendAll(lines 573-629) andtrySendingUpTo(lines 740-801) share substantial structural similarity:
- Feature flag check
- Account resolution
- CachedBalances lookup
- Posting emission loop
- Reverse posting emission
- Balance update via
MapGetOrPutDefault- Final
trySendingToAccountcallExtracting the common scaling posting logic into a helper method would reduce duplication and make future maintenance easier.
Also applies to: 740-801
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
internal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-all-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-with-oneof.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling.num.specs.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
internal/interpreter/interpreter.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-16T14:51:51.052Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 70
File: internal/interpreter/accounts_metadata.go:13-24
Timestamp: 2025-07-16T14:51:51.052Z
Learning: The DeepClone method in internal/interpreter/accounts_metadata.go works correctly despite using MapGetOrPutDefault in what initially appears to be a non-assignment context. The fetchAccountMetadata method returns a reference to the map stored in the cloned structure, and MapGetOrPutDefault modifies it in-place, so the cloned values persist correctly.
Applied to files:
internal/interpreter/interpreter.go
📚 Learning: 2025-07-16T14:50:12.931Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 70
File: internal/utils/pretty_csv.go:36-40
Timestamp: 2025-07-16T14:50:12.931Z
Learning: ascandone prefers using panics for violated function preconditions rather than explicit error handling. In the CsvPretty function in internal/utils/pretty_csv.go, panicking when row[fieldIndex] is out of bounds is intentional behavior for enforcing the precondition that headers must not be shorter than any rows.
Applied to files:
internal/interpreter/interpreter.go
🧬 Code graph analysis (1)
internal/interpreter/interpreter.go (6)
internal/parser/ast.go (3)
SourceWithScaling(146-146)SourceWithScaling(188-193)Source(135-138)internal/flags/flags.go (1)
AssetScaling(13-13)internal/interpreter/interpreter_error.go (2)
InvalidUnboundedAddressInScalingAddress(259-261)InvalidUnboundedAddressInScalingAddress(263-265)internal/parser/range.go (1)
Range(13-16)internal/interpreter/value.go (2)
Asset(17-17)Asset(31-31)internal/utils/utils.go (1)
MapGetOrPutDefault(50-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (1)
internal/interpreter/interpreter.go (1)
54-73: LGTM - Catch-all asset pattern handling.The logic correctly handles the
/*suffix pattern for asset scaling queries. The code properly:
- Distinguishes catch-all from regular currency queries
- Matches both the base asset exactly and scaled variants (
baseAsset/N)- Creates defensive copies of
big.Intvalues to avoid shared state
| for _, convAmt := range sol { | ||
| scale := convAmt.scale | ||
| convAmt := convAmt.amount | ||
|
|
||
| // here we manually emit postings based on the known solution, | ||
| // and update balances accordingly | ||
| asset := buildScaledAsset(baseAsset, scale) | ||
| s.Postings = append(s.Postings, Posting{ | ||
| Source: *account, | ||
| Destination: *scalingAccount, | ||
| Amount: new(big.Int).Set(convAmt), | ||
| Asset: asset, | ||
| }) | ||
| acc[asset].Sub(acc[asset], convAmt) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the full context around the flagged code
sed -n '573,629p' internal/interpreter/interpreter.goRepository: formancehq/numscript
Length of output: 1568
🏁 Script executed:
# Find the getAssets function
rg -A 20 'func getAssets' internal/Repository: formancehq/numscript
Length of output: 1288
🏁 Script executed:
# Find the findSolution function
rg -A 25 'func findSolution' internal/Repository: formancehq/numscript
Length of output: 1615
🏁 Script executed:
# Check the CachedBalances type and how acc is initialized
rg -B 5 -A 10 'type CachedBalances' internal/Repository: formancehq/numscript
Length of output: 46
🏁 Script executed:
# Find buildScaledAsset function
rg -A 5 'func buildScaledAsset' internal/Repository: formancehq/numscript
Length of output: 441
🏁 Script executed:
# Find AccountBalance type definition
rg 'type AccountBalance' internal/Repository: formancehq/numscript
Length of output: 141
🏁 Script executed:
# Check how getAssetScale works
rg -A 10 'func getAssetScale' internal/Repository: formancehq/numscript
Length of output: 761
🏁 Script executed:
# Check how CachedBalances is populated and if nil values are possible
rg -B 5 -A 10 'CachedBalances\[' internal/interpreter/interpreter.go | head -100Repository: formancehq/numscript
Length of output: 709
🏁 Script executed:
# Look for any place where nil big.Int values might be added to AccountBalance
rg 'CachedBalances.*=.*nil' internal/Repository: formancehq/numscript
Length of output: 46
🏁 Script executed:
# Check the entire flow of how values are added to the balance map
rg -A 3 'acc\[.*\]\s*=' internal/interpreter/interpreter.go | head -50Repository: formancehq/numscript
Length of output: 46
🏁 Script executed:
# Check if there are other similar patterns where acc[asset] is accessed
rg 'acc\[.*\]\.' internal/interpreter/interpreter.goRepository: formancehq/numscript
Length of output: 141
🏁 Script executed:
# Check if MapGetOrPutDefault is used elsewhere for similar safety
rg -B 2 -A 2 'MapGetOrPutDefault.*acc' internal/interpreter/interpreter.goRepository: formancehq/numscript
Length of output: 327
🏁 Script executed:
# Look at the complete findSolution function to understand what it returns
rg -A 50 'func findSolution\(' internal/interpreter/asset_scaling.goRepository: formancehq/numscript
Length of output: 1309
🏁 Script executed:
# Get more context on the full findSolution function to see what it returns
rg -A 100 'func findSolution\(' internal/interpreter/asset_scaling.go | head -120Repository: formancehq/numscript
Length of output: 1586
🏁 Script executed:
# Check if there's any code that adds nil values to CachedBalances
rg -B 3 -A 3 'CachedBalances\[.*\]\[.*\]\s*=' internal/interpreter/interpreter.go | head -60Repository: formancehq/numscript
Length of output: 46
🏁 Script executed:
# Verify the exact reconstruction logic by checking if buildScaledAsset output matches original asset names
cat -n internal/interpreter/asset_scaling.go | sed -n '1,150p'Repository: formancehq/numscript
Length of output: 3829
🏁 Script executed:
# Check how CachedBalances values are initialized and if nil values are possible
rg -B 5 -A 5 'CachedBalances\s*=' internal/interpreter/interpreter.go | head -80Repository: formancehq/numscript
Length of output: 337
🏁 Script executed:
# Look for how balance entries are created to see if nil is possible
rg -B 3 -A 3 'make\(map\[string\]\*big.Int' internal/Repository: formancehq/numscript
Length of output: 571
🏁 Script executed:
# Verify if nil values can exist in AccountBalance by checking initialization patterns
rg -B 5 -A 10 'DeepClone' internal/interpreter/interpreter.goRepository: formancehq/numscript
Length of output: 522
🏁 Script executed:
# Check if there are any patterns where nil values might be added to the balance map
rg '\[.*\]\s*=\s*nil' internal/interpreter/interpreter.goRepository: formancehq/numscript
Length of output: 46
🏁 Script executed:
# Look for how balances are initially created for accounts
rg -B 10 'CachedBalances\[.*\]\s*=' internal/interpreter/interpreter.go | head -80Repository: formancehq/numscript
Length of output: 46
🏁 Script executed:
# Get the complete context to understand the full flow
sed -n '41,50p' internal/interpreter/asset_scaling.goRepository: formancehq/numscript
Length of output: 344
🏁 Script executed:
# Verify what scales are guaranteed to exist
sed -n '67,135p' internal/interpreter/asset_scaling.goRepository: formancehq/numscript
Length of output: 1586
🏁 Script executed:
# Check one more time the exact reconstruction and access pattern
sed -n '601,615p' internal/interpreter/interpreter.goRepository: formancehq/numscript
Length of output: 524
🏁 Script executed:
# Check if there are any validations on asset names
rg -i 'asset.*valid' internal/interpreter/ | head -20Repository: formancehq/numscript
Length of output: 137
🏁 Script executed:
# Look for any comments or documentation about asset naming
rg -B 2 'asset' internal/interpreter/asset_scaling.go | grep -A 2 '//'Repository: formancehq/numscript
Length of output: 176
🏁 Script executed:
# Check if there are any asset name validation tests
fd -e go internal/interpreter/.*test.go -x rg -l 'asset.*scale\|scale.*asset' {} \;Repository: formancehq/numscript
Length of output: 46
🏁 Script executed:
# Look for test cases that might show expected asset name formats
rg 'EUR|USD|asset.*/' internal/interpreter/interpreter_test.go | head -20Repository: formancehq/numscript
Length of output: 666
🏁 Script executed:
# Check if MapGetOrPutDefault is available and used elsewhere for similar safety
rg -B 1 -A 1 'MapGetOrPutDefault' internal/interpreter/interpreter.go | head -30Repository: formancehq/numscript
Length of output: 414
🏁 Script executed:
# Examine getAssetScale behavior more carefully
sed -n '28,39p' internal/interpreter/asset_scaling.goRepository: formancehq/numscript
Length of output: 345
🏁 Script executed:
# Check if there's any validation when assets are added to CachedBalances
rg -B 5 'CachedBalances.*\[.*\]\[' internal/interpreter/interpreter.go | head -60Repository: formancehq/numscript
Length of output: 46
🏁 Script executed:
# Look for where assets are initially added to understand if they're validated
rg 'accountBalanceLookup\[' internal/interpreter/interpreter.goRepository: formancehq/numscript
Length of output: 112
🏁 Script executed:
# Final verification: trace what happens with a malformed asset name
python3 << 'EOF'
# Simulate the data flow with edge case
baseAsset = "EUR"
# Scenario: acc has "EUR/2extra" as a key (malformed)
acc = {"EUR/2extra": 100} # Note: no "EUR" key with scale 0
# getAssets processes this
assets_dict = {}
for asset in acc.keys():
if asset.startswith(baseAsset): # "EUR/2extra" starts with "EUR" ✓
# getAssetScale would return...
parts = asset.split("/")
if len(parts) == 2:
try:
scale = int(parts[1]) # int("2extra") fails
except:
# fallback: returns (asset, 0)
scale = 0
base = asset
else:
scale = 0
base = asset
print(f"Asset: {asset}, parsed_scale: {scale}, base: {base}")
assets_dict[scale] = 100
print(f"\nAssets dict from getAssets: {assets_dict}")
# Now buildScaledAsset reconstructs
for scale in assets_dict.keys():
if scale == 0:
reconstructed = baseAsset # "EUR"
else:
reconstructed = f"{baseAsset}/{scale}"
print(f"Scale {scale} reconstructs to: {reconstructed}")
print(f"Accessing acc['{reconstructed}']: {acc.get(reconstructed, 'WOULD BE NIL!')}")
EOFRepository: formancehq/numscript
Length of output: 150
Add a defensive check or use MapGetOrPutDefault for safe map access at line 614.
The reconstructed asset name via buildScaledAsset could differ from the original key if asset names are malformed, causing a nil pointer dereference when calling .Sub(). This is especially inconsistent with line 623, which uses MapGetOrPutDefault for defensive access to derived keys.
🤖 Prompt for AI Agents
In @internal/interpreter/interpreter.go around lines 601 - 615, The loop that
builds postings uses buildScaledAsset to derive asset and then calls
acc[asset].Sub(acc[asset], convAmt) which can nil-panic if acc lacks that key;
replace that direct map access with a defensive retrieval (use
MapGetOrPutDefault or a check like ok := acc[asset]; if !ok { acc[asset] =
NewZeroBalance(); } ) before calling Sub, or use the existing MapGetOrPutDefault
helper to obtain a non-nil balance for the derived asset; update the code around
the Posting append and subsequent Sub call to fetch/ensure acc[asset] safely
(referencing convAmt.amount/scale, buildScaledAsset, s.Postings append, and the
acc map).
No description provided.