feat: prevent invalid outputs #105
Conversation
WalkthroughAdds account and asset name validation, new error types (InternalError, InvalidAsset), uses NewAsset during parsing, enforces posting invariants after RunProgram, and adds a test for invalid asset metadata. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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
![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 67.83% 67.84%
=======================================
Files 45 45
Lines 4396 4422 +26
=======================================
+ Hits 2982 3000 +18
- Misses 1254 1260 +6
- Partials 160 162 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ascandone
force-pushed
the
feat/prevent-invalid-outputs
branch
from
f18da4a to
56fcc0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/interpreter/interpreter.go (1)
249-266: Consider including Range information in InternalError for better debugging.The
checkPostingInvariantsfunction correctly validates all three invariants, but the returnedInternalErrordoesn't populate theRangefield. While thePostingprovides the account and asset names, having location information would help users identify which statement in their script produced the invalid posting.If range information is available at the call site or can be tracked during posting creation, consider passing it to improve error messages.
📜 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/interpreter/interpreter.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/interpreter/interpreter.go (2)
internal/interpreter/value.go (5)
NewAsset(40-45)Monetary(21-24)Monetary(29-29)Asset(17-17)Asset(31-31)internal/interpreter/interpreter_error.go (1)
InternalError(10-13)
🔇 Additional comments (4)
internal/interpreter/interpreter.go (4)
108-117: LGTM! Early asset validation prevents invalid assets.The change to use
NewAssetensures asset names are validated during parsing, catching invalid assets before they can propagate through the system.
135-135: LGTM! Consistent asset validation.Using
NewAssetfor asset variable parsing ensures all asset values are validated regardless of how they enter the system.
319-324: LGTM! Final safeguard ensures no invalid postings are produced.This validation loop correctly implements the PR's objective to prevent invalid outputs. It runs after all program logic completes but before returning results, ensuring:
- No negative amounts in final postings (while allowing negative intermediates during computation)
- All account and asset names are valid
- The system fails fast on the first invalid posting
233-247: The regexes are correct and functional. Verification confirms that colored assets (using the_[A-Z]{1,16}pattern) are properly supported by theassetNameRegexp. Both regex patterns work as intended:
- Account regex correctly validates alphanumeric, underscore, and hyphen characters with colon-separated segments
- Asset regex correctly validates uppercase-starting assets with optional colored suffix and ledger notation (
/digits)No changes needed. The implementation is sound.
ascandone
force-pushed
the
feat/prevent-invalid-outputs
branch
from
56fcc0d to
768ab11
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/interpreter/interpreter_test.go (1)
59-76: Consider addingInvalidAssettoremoveRangefor consistency.The
removeRangehelper normalizes error ranges for test comparisons but doesn't handle the newInvalidAsseterror type. While the current test passes (both expected and actual have zero-valued ranges), adding it would ensure consistency with other error types and prevent future test fragility.♻️ Suggested addition
func removeRange(e machine.InterpreterError) machine.InterpreterError { switch e := e.(type) { case machine.MissingFundsErr: e.Range = parser.Range{} return e case machine.TypeError: e.Range = parser.Range{} return e case machine.InvalidTypeErr: e.Range = parser.Range{} return e case machine.NegativeAmountErr: e.Range = parser.Range{} return e + case machine.InvalidAsset: + e.Range = parser.Range{} + return e + case machine.InternalError: + e.Range = parser.Range{} + return e default: return e } }internal/interpreter/interpreter.go (1)
249-266: Consider providing more specific error details for debugging.The function correctly validates all posting invariants, but when a violation occurs,
InternalErrordoesn't indicate which invariant failed (negative amount, invalid asset, invalid source, or invalid destination). This could make debugging harder in production.♻️ Optional: Add specific failure reason
-// Check the following invariants: -// - no negative postings -// - no invalid account names -// - no invalid asset names -func checkPostingInvariants(posting Posting) InterpreterError { - isAmtNegative := posting.Amount.Cmp(big.NewInt(0)) == -1 - - isInvalidPosting := (isAmtNegative || - !checkAssetName(posting.Asset) || - !checkAccountName(posting.Source) || - !checkAccountName(posting.Destination)) - - if isInvalidPosting { - return InternalError{Posting: posting} - } - - return nil -} +// Check the following invariants: +// - no negative postings +// - no invalid account names +// - no invalid asset names +func checkPostingInvariants(posting Posting) InterpreterError { + if posting.Amount.Cmp(big.NewInt(0)) == -1 { + return InternalError{Posting: posting, Reason: "negative amount"} + } + if !checkAssetName(posting.Asset) { + return InternalError{Posting: posting, Reason: "invalid asset name"} + } + if !checkAccountName(posting.Source) { + return InternalError{Posting: posting, Reason: "invalid source account"} + } + if !checkAccountName(posting.Destination) { + return InternalError{Posting: posting, Reason: "invalid destination account"} + } + return nil +}This would require adding a
Reason stringfield toInternalError.
📜 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 (4)
internal/interpreter/interpreter.gointernal/interpreter/interpreter_error.gointernal/interpreter/interpreter_test.gointernal/interpreter/value.go
🧰 Additional context used
🧠 Learnings (2)
📚 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-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 (4)
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)
internal/interpreter/interpreter.go (3)
internal/interpreter/value.go (5)
NewAsset(40-45)Monetary(21-24)Monetary(29-29)Asset(17-17)Asset(31-31)numscript.go (2)
Posting(52-52)InterpreterError(74-74)internal/interpreter/interpreter_error.go (1)
InternalError(10-13)
internal/interpreter/value.go (3)
internal/interpreter/interpreter_error.go (2)
InvalidAccountName(224-227)InvalidAsset(233-236)internal/interpreter/interpreter.go (1)
InterpreterError(73-76)numscript.go (1)
InterpreterError(74-74)
🔇 Additional comments (8)
internal/interpreter/interpreter_test.go (1)
225-249: LGTM! Test correctly validates asset name validation via metadata.The test properly verifies that an invalid asset name ("Aa" - lowercase letters not allowed) resolved through account metadata triggers an
InvalidAsseterror.internal/interpreter/value.go (1)
33-45: LGTM! Clean implementation of validation constructors.Both
NewAccountAddressandNewAssetfollow a consistent pattern: validate input using the respective check function, return an appropriate error on failure, or return the typed value on success. This ensures invalid values cannot be constructed.internal/interpreter/interpreter.go (4)
108-117: LGTM! Asset validation added to monetary parsing.The change correctly validates the asset name before constructing the
Monetaryvalue, ensuring invalid assets cannot enter the system through monetary literals.
134-136: LGTM! Asset variable parsing now validates input.Asset type variables from JSON input are now validated through
NewAsset, consistent with how account variables are validated viaNewAccountAddress.
233-247: LGTM! Validation regex patterns are precompiled and well-documented.The regex patterns are correctly precompiled at package initialization for performance. The comments referencing the ledger source files provide good traceability for the duplicated patterns.
319-324: LGTM! Invariant checks correctly positioned as final validation.The post-execution validation loop serves as a safety net, ensuring no invalid postings escape even if intermediate expressions involve negative values. This aligns with the PR's design rationale of allowing intermediate negative monetaries while enforcing non-negativity only at output.
internal/interpreter/interpreter_error.go (2)
10-17: LGTM! InternalError type correctly captures invariant violations.The error type appropriately embeds
parser.Rangeto satisfy theInterpreterErrorinterface and includes the fullPostingfor debugging context.
233-240: LGTM! InvalidAsset error type follows established patterns.The structure mirrors
InvalidAccountNamewith the embeddedparser.RangeandNamefield, providing a consistent error interface and clear error message.
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)