feat: prevent invalid outputs #105
Conversation
WalkthroughThis change introduces validation for asset and account names with new error types. Monetary and asset parsing now validate names using regex patterns and return custom error types (InternalError, InvalidAsset) on failure. Invariant checks are performed on postings after program computation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
ascandone
force-pushed
the
feat/prevent-invalid-outputs
branch
from
d784f68 to
db5f5c4
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="internal/interpreter/interpreter.go">
<violation number="1" location="internal/interpreter/interpreter.go:237">
P2: Regex is compiled on every function call. Move `Regexp` to package level for better performance, consistent with existing patterns like `colorRe`, `percentRegex`, and `fractionRegex`.</violation>
<violation number="2" location="internal/interpreter/interpreter.go:244">
P2: Regex is compiled on every function call. Move `Regexp` to package level for better performance, consistent with existing patterns like `colorRe`, `percentRegex`, and `fractionRegex`.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| // https://github.com/formancehq/ledger/blob/main/pkg/assets/asset.go | ||
| func checkAssetName(v string) bool { | ||
| const Pattern = `[A-Z][A-Z0-9]{0,16}(_[A-Z]{1,16})?(\/\d{1,6})?` | ||
| var Regexp = regexp.MustCompile("^" + Pattern + "$") |
There was a problem hiding this comment.
P2: Regex is compiled on every function call. Move Regexp to package level for better performance, consistent with existing patterns like colorRe, percentRegex, and fractionRegex.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/interpreter/interpreter.go, line 244:
<comment>Regex is compiled on every function call. Move `Regexp` to package level for better performance, consistent with existing patterns like `colorRe`, `percentRegex`, and `fractionRegex`.</comment>
<file context>
@@ -225,6 +230,40 @@ func (s *programState) parseVars(varDeclrs []parser.VarDeclaration, rawVars map[
+// https://github.com/formancehq/ledger/blob/main/pkg/assets/asset.go
+func checkAssetName(v string) bool {
+ const Pattern = `[A-Z][A-Z0-9]{0,16}(_[A-Z]{1,16})?(\/\d{1,6})?`
+ var Regexp = regexp.MustCompile("^" + Pattern + "$")
+ return Regexp.Match([]byte(v))
+}
</file context>
| func checkAccountName(addr string) bool { | ||
| const SegmentRegex = "[a-zA-Z0-9_-]+" | ||
| const Pattern = "^" + SegmentRegex + "(:" + SegmentRegex + ")*$" | ||
| var Regexp = regexp.MustCompile(Pattern) |
There was a problem hiding this comment.
P2: Regex is compiled on every function call. Move Regexp to package level for better performance, consistent with existing patterns like colorRe, percentRegex, and fractionRegex.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/interpreter/interpreter.go, line 237:
<comment>Regex is compiled on every function call. Move `Regexp` to package level for better performance, consistent with existing patterns like `colorRe`, `percentRegex`, and `fractionRegex`.</comment>
<file context>
@@ -225,6 +230,40 @@ func (s *programState) parseVars(varDeclrs []parser.VarDeclaration, rawVars map[
+func checkAccountName(addr string) bool {
+ const SegmentRegex = "[a-zA-Z0-9_-]+"
+ const Pattern = "^" + SegmentRegex + "(:" + SegmentRegex + ")*$"
+ var Regexp = regexp.MustCompile(Pattern)
+ return Regexp.Match([]byte(addr))
+}
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/interpreter/interpreter.go (2)
233-247: Centralised name validation is good; consider hoisting regex compilationThe
checkAccountName/checkAssetNamehelpers correctly mirror the ledger regexes, but they recompile their regexes on every call. For modest performance and consistency withcolorRe,percentRegex, andfractionRegex, consider hoisting the compiled regexes to package scope and reusing them:-// https://github.com/formancehq/ledger/blob/main/pkg/accounts/accounts.go -func checkAccountName(addr string) bool { - const SegmentRegex = "[a-zA-Z0-9_-]+" - const Pattern = "^" + SegmentRegex + "(:" + SegmentRegex + ")*$" - var Regexp = regexp.MustCompile(Pattern) - return Regexp.Match([]byte(addr)) -} +const accountSegmentRegex = "[a-zA-Z0-9_-]+" +const accountPattern = "^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*$" +var accountNameRegexp = regexp.MustCompile(accountPattern) + +func checkAccountName(addr string) bool { + return accountNameRegexp.MatchString(addr) +} -// https://github.com/formancehq/ledger/blob/main/pkg/assets/asset.go -func checkAssetName(v string) bool { - const Pattern = `[A-Z][A-Z0-9]{0,16}(_[A-Z]{1,16})?(\/\d{1,6})?` - var Regexp = regexp.MustCompile("^" + Pattern + "$") - return Regexp.Match([]byte(v)) -} +const assetPattern = `[A-Z][A-Z0-9]{0,16}(_[A-Z]{1,16})?(\/\d{1,6})?` +var assetNameRegexp = regexp.MustCompile("^" + assetPattern + "$") + +func checkAssetName(v string) bool { + return assetNameRegexp.MatchString(v) +}
252-265: Posting invariants are correct; add nil-amount safety for robustnessThe
checkPostingInvariants+ final loop inRunProgramcorrectly enforce non‑negative amounts and valid account/asset names before returning postings, which is exactly what the PR is aiming for. One robustness tweak: if a future change ever appends a posting withAmount == nil,posting.Amount.Cmp(...)will panic instead of returning anInternalError. You could guard that case explicitly:func checkPostingInvariants(posting Posting) InterpreterError { - isAmtNegative := posting.Amount.Cmp(big.NewInt(0)) == -1 + if posting.Amount == nil { + return InternalError{Posting: posting} + } + isAmtNegative := posting.Amount.Cmp(big.NewInt(0)) == -1 // ... }This keeps invariant violations reporting through
InternalErrorinstead of panicking, while still treating them as “should never happen” conditions. Based on learnings, this also preserves the existing behaviour where insufficient‑funds errors on colored assets still report the base asset symbol.Also applies to: 318-323
internal/interpreter/value.go (1)
33-45: Account/asset constructors now enforce validation; clarify or align NewMonetarySwitching
NewAccountAddresstocheckAccountNameand introducingNewAssetgives a clear, centralized validation path and the rightInvalidAccountName/InvalidAsseterrors for untrusted strings. One small consistency gap isNewMonetary, which still accepts an arbitraryassetstring and wraps it asAssetwithout validation; if this helper is ever used with external input, it could bypass the new checks.Consider either:
- documenting that
NewMonetaryexpects an already‑validated asset string, or- adding a separate validated constructor (e.g.
NewValidatedMonetary(asset string, n int64) (Monetary, InterpreterError)) that usesNewAssetunder the hood.Also applies to: 224-229
internal/interpreter/interpreter_error.go (1)
10-18: New InternalError and InvalidAsset types fit the error model; consider test helper update
InternalErrorandInvalidAssetare well‑shaped for the new invariant and asset‑validation paths (they embedparser.Rangeand implementError()cleanly). To keep tests resilient if you later start populating theirRangefields, it may be worth extendingremoveRangeininternal/interpreter/interpreter_test.goto normalize these types as well, similar toMissingFundsErrandTypeError, so struct equality in tests stays focused on semantic fields rather than locations.Also applies to: 233-240
📜 Review details
Configuration used: CodeRabbit 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 (4)
internal/interpreter/interpreter.go(4 hunks)internal/interpreter/interpreter_error.go(2 hunks)internal/interpreter/interpreter_test.go(1 hunks)internal/interpreter/value.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-23T16:27:16.351Z
Learnt from: ascandone
Repo: formancehq/numscript PR: 55
File: internal/interpreter/interpreter_test.go:4273-4295
Timestamp: 2025-04-23T16:27:16.351Z
Learning: When a numscript operation fails due to insufficient funds on a colored asset (e.g., "COIN*red"), the error references the uncolored asset (e.g., "COIN") as specified in the script, not the colored version used internally.
Applied to files:
internal/interpreter/interpreter_error.go
🧬 Code graph analysis (3)
internal/interpreter/value.go (1)
internal/interpreter/interpreter_error.go (2)
InvalidAccountName(224-227)InvalidAsset(233-236)
internal/interpreter/interpreter_test.go (3)
internal/interpreter/interpreter.go (3)
AccountsMetadata(27-27)AccountMetadata(26-26)Posting(80-85)numscript.go (3)
AccountsMetadata(63-63)AccountMetadata(60-60)Posting(52-52)internal/interpreter/interpreter_error.go (1)
InvalidAsset(233-236)
internal/interpreter/interpreter_error.go (2)
internal/parser/range.go (1)
Range(13-16)internal/interpreter/interpreter.go (1)
Posting(80-85)
⏰ 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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (2)
internal/interpreter/interpreter_test.go (1)
225-249: TestBadAssetInMeta correctly asserts InvalidAsset from metadataThis test cleanly exercises the
meta()+assetvar origin path and confirms that an invalid asset coming from account metadata surfaces asInvalidAssetwith the offending name and no postings, matching the newNewAssetvalidation flow.internal/interpreter/interpreter.go (1)
95-118: Monetary and asset variables now flow through NewAsset validationWiring both
parseMonetaryand theanalysis.TypeAssetbranch ofparseVarthroughNewAssetensures asset strings coming from JSON ormeta()are validated and surface asInvalidAsseton failure, which aligns with the runtime validation goals of this PR.Also applies to: 120-148
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
==========================================
- Coverage 68.13% 68.13% -0.01%
==========================================
Files 45 45
Lines 4290 4321 +31
==========================================
+ Hits 2923 2944 +21
- Misses 1209 1216 +7
- Partials 158 161 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR prevents the interpreter to output invalid postings.
We check that, in every posting:
Point 1) should be a consequence of the implementation but it's very hard to prove that no such bugs exist. We can't simply disallow negative monetaries to be constructed because they are legit values, although we can't send them around. But they can be involved in expressions like
$x - $y + $z(maybe$x-$yis neg, but the whole expr is positive).The condition 2 and 3 are also checked on runtime, so that invalid assets/account can never exist, at no point of the execution. Still, it's better to double-check at the end of the script.
The regex are taken from the ledger. For the sake of simplicity, we are duplicating this domain data and avoiding depending on Formance common packages. This is justified by the fact that they very rarely change, and we want to control whether we relax the regex on this repo (we don't want to update by mistake to a version of the dependency that relaxes the regex)