Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces virtual accounts—a new account type that enables intermediate fund pooling without permanent ledger entries. It adds a feature flag, a new Account type system with virtual account support, VirtualAccount with credit/debit settlement logic, and updates the interpreter to handle virtual account creation, sending, and receiving. Additionally, it includes 21 test scripts exercising virtual account scenarios. Changes
Sequence DiagramsequenceDiagram
participant User as User/World
participant Interp as Interpreter
participant VAcct as VirtualAccount
participant RAcct as Real Account
User->>Interp: send [USD 10] to `@world`
Interp->>RAcct: deposit USD 10
User->>Interp: account $v = virtual()
Interp->>VAcct: create VirtualAccount
User->>Interp: send [USD 10] from `@world` to $v
Interp->>RAcct: withdraw USD 10 (from `@world`)
Interp->>VAcct: Receive(USD 10, sender=@world)
VAcct->>VAcct: credits[USD].Push(sender=@world, amt=10)
User->>Interp: send [USD 5] from $v to `@dest`
Interp->>VAcct: Pull(USD 5, receiver=@dest)
VAcct->>VAcct: repay debits if any
VAcct->>VAcct: credits[USD].PullAnything(5)
Interp->>RAcct: deposit USD 5 (to `@dest`)
User->>Interp: send [USD 100] from $v to `@dest` (overdraft: unbounded)
Interp->>VAcct: Pull(USD 100, receiver=@dest, overdraft=true)
VAcct->>VAcct: credits[USD].PullAnything(100)
Note over VAcct: Only USD 5 available
VAcct->>VAcct: debits[USD].Push(sender=@dest, amt=95)
Interp->>RAcct: deposit USD 5 (to `@dest`)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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 #69 +/- ##
==========================================
+ Coverage 68.55% 68.94% +0.38%
==========================================
Files 46 47 +1
Lines 4621 4833 +212
==========================================
+ Hits 3168 3332 +164
- Misses 1280 1323 +43
- Partials 173 178 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ascandone
force-pushed
the
feat/virtual-account
branch
3 times, most recently
from
dca1aab to
72e72f1
Compare
ascandone
force-pushed
the
feat/virtual-account
branch
4 times, most recently
from
afaa35a to
55e8fda
Compare
ascandone
force-pushed
the
feat/virtual-account
branch
2 times, most recently
from
8287590 to
44f703c
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/interpreter/interpreter_error.go (1)
115-123: Drop the hardcoded '@' and rely on Account’s own representation in the error messageNow that Account is a wrapper over multiple representations (e.g., AccountAddress, VirtualAccount), hardcoding '@' is incorrect for non-address accounts. Also, formatting with %s assumes a string; prefer using %v (or explicitly .String()).
Apply:
- return fmt.Sprintf("Cannot fetch negative balance from account @%s", e.Account) + return fmt.Sprintf("Cannot fetch negative balance from account %v", e.Account)If Account doesn’t implement fmt.Stringer, add a String() string method that renders addresses with a leading '@' and virtual accounts with their own canonical form.
🧹 Nitpick comments (32)
internal/interpreter/function_statements.go (1)
35-35: Type erasure of meta to string.If downstream consumers rely on typed metadata (numbers, booleans), String() conversion loses type info. Consider storing the raw Value or a typed/JSON form instead.
internal/interpreter/testdata/script-tests/experimental/virtual-account/test-send-self-is-noop.num (1)
1-5: Covers self-send noop; consider adding guard cases.Add a counterpart that tries self-send across different assets (should fail) and one with an overdraft bound of zero to ensure no unintended postings occur.
internal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-left-negative.num (1)
1-11: Edge-case companion tests would strengthen coverage.
- Add a variant where the second transfer only partially offsets the first (to keep a negative residual).
- Add assertions in the harness (if supported) to validate final balances of $v and @DesT.
internal/interpreter/testdata/script-tests/experimental/virtual-account/bounded-overdraft-when-does-not-fail.num (1)
1-11: Nice: positive flow with large bound. Add failing counterpart.Add a “bounded-overdraft-when-fails” case (e.g., bound below 90 when sending 100 after receiving 10) to verify strict enforcement.
internal/interpreter/interpreter_error.go (1)
39-47: Optional: align other account-bearing error types to the Account wrapper for consistencyMetadataNotFound.Account still uses string, while NegativeBalanceError.Account is Account. Consider migrating others for a uniform API and future-proof messaging with virtual accounts.
I can batch-prepare a follow-up diff if you want.
Also applies to: 115-123
internal/interpreter/testdata/script-tests/experimental/virtual-account/repay-self.num (1)
1-14: Verify portions syntax and intent aroundUSD/2 *vsUSD/2 10Both constructs look intentional; just confirm the grammar here matches current parser rules and semantics (half of all vs half with floor/min). If the distinction isn’t required, consider standardizing to one form in tests.
internal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending-send-all.num (1)
8-14: Clarify intent: duplicate$ventries are intentional to test dedup.Consider adding a brief comment so future maintainers don’t “fix” the duplication.
internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-merchant-pays-fees-if-needed.num (1)
5-11: Exercise the “merchant pays if needed” path with a concrete low-$amt case.As written, this relies on external input for $amt. Add a companion script (or fixture) with $amt = 30 so 10% -> 3 EUR and the second send must draw 2 EUR from @merchant, plus one with $amt = 50 (exact 5) to cover rounding edges.
internal/interpreter/args_parser_test.go (2)
50-51: Make the expected Account literal explicit and keep expected-first order.Use the field name to avoid relying on struct field order and keep Equal(expected, actual) consistent.
- require.Equal(t, *big.NewInt(42), *a1) - require.Equal(t, Account{AccountAddress("user:001")}, *a2) + require.Equal(t, *big.NewInt(42), *a1) + require.Equal(t, Account{Repr: AccountAddress("user:001")}, *a2)Outside this hunk: prefer require.NoError(t, err) over require.Nil(t, err) at Lines 16 and 45 for clarity.
59-59: Style: keep construction consistent across the file.You construct the same value as Account{Repr: AccountAddress("user:001")} above; keep that form everywhere in this test for uniformity.
internal/interpreter/testdata/script-tests/experimental/virtual-account/account-kept.num (2)
8-14: Clarify “remaining kept” expectation with an assertion/golden.Intended outcome appears: 5 USD to @DesT, 5 USD left in $v. Please add/verify a golden so the next send moves exactly 5 USD to @other, confirming kept semantics.
16-19: Final drain check.Consider another variant where the second step is max [USD 15] to ensure the third send is zero, exercising the empty-balance path.
internal/interpreter/testdata/script-tests/experimental/virtual-account/creditors-stack.num (1)
4-15: Covers unbounded overdraft well; add a bounded-failure sibling.Add a test using “allowing overdraft up to [USD 150]” (or equivalent) to validate failure behavior and messages when limits are exceeded.
internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-no-commissions-with-low-amount.num (1)
13-19: Oneof branch semantics: verify low-amount path is exercised.For “no commissions with low amount,” make sure $amt is low enough that the “max [EUR 5] to @DesT” branch is actually taken in CI to cover this scenario.
If helpful, add a brief comment stating the intended ranges for $amt to clarify the test’s purpose.
internal/interpreter/testdata/script-tests/experimental/virtual-account/send-credit-around.num (1)
11-16: Fix minor grammar in comment.- // here's we're sending the credit we have from $v1 + // here we're sending the credit we have from $v1internal/interpreter/function_exprs.go (2)
22-27: Runtime expects a real account address; static typing still allows any account.Switching to expectAccountAddress is correct at runtime, but analysis still types balance/meta/overdraft params as generic account. Users could pass virtual() and get a runtime error instead of a compile-time diagnostic.
Consider adding an analysis-time check that rejects virtual() where a real address is required, or introduce a distinct AccountAddress type in the analyzer to catch this earlier.
56-61: Meta on virtual accounts.Same concern as above: meta() now requires a real address at runtime. Ensure the error clearly states “virtual accounts not allowed in meta()”.
If not already, specialize the expectAccountAddress error for virtual inputs.
internal/interpreter/batch_balances_query.go (2)
35-37: Skip batching for virtual accounts—good.Type-asserting to AccountAddress avoids querying balances for virtual accounts. Consider a small helper (e.g., asAddress(acc Account) (string, bool)) to reduce repetition across sites.
102-105: Same assertion pattern—consider helper to DRY.internal/analysis/check.go (1)
118-129: Builtin registration and gating look correct; mind analyzer vs runtime mismatch.Zero-arity, returns account, gated at >= 0.0.20 + ExperimentalVirtualAccount—good. However, analyzer still allows virtual() anywhere TypeAccount is accepted, while runtime now rejects it for balance/meta/overdraft.
Add an analyzer diagnostic that forbids passing virtual() into those builtins, or model “account-address” as a distinct type for those parameters to fail fast at analysis time.
Also confirm tests set version >= 0.0.20 and enable the flag.
internal/interpreter/interpreter_test.go (3)
401-405: Use keyed struct literal for stability when constructing AccountPositional struct literals are brittle across refactors. Prefer a keyed literal to avoid breakage if fields are reordered.
- Error: machine.NegativeBalanceError{ - Account: machine.Account{machine.AccountAddress("a")}, - Amount: *big.NewInt(-100), - }, + Error: machine.NegativeBalanceError{ + Account: machine.Account{Repr: machine.AccountAddress("a")}, + Amount: *big.NewInt(-100), + },
470-475: Same: prefer keyed Account literal in TypeError.ValueKeeps tests resilient to internal struct changes.
- Error: machine.TypeError{ - Expected: "monetary", - Value: machine.Account{machine.AccountAddress("bad:type")}, - }, + Error: machine.TypeError{ + Expected: "monetary", + Value: machine.Account{Repr: machine.AccountAddress("bad:type")}, + },
611-615: Same: prefer keyed Account literal in TypeError.Value- Error: machine.TypeError{ - Expected: "string", - Value: machine.Account{machine.AccountAddress("key_wrong_type")}, - }, + Error: machine.TypeError{ + Expected: "string", + Value: machine.Account{Repr: machine.AccountAddress("key_wrong_type")}, + },internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-fail-if-not-enough.num (1)
1-16: Comment/code mismatch: script currently sends full 10% to @fees, not “up to 5”
- The comment says “10%$amt (up to 5) to @fees”, but step 3 empties $fees_hold to @fees, resulting in total fees = 10%$amt when $amt > 50.
- If the intent is “min(10%*$amt, 5) goes to @fees and the rest to @DesT”, send the remainder of $fees_hold to $dest_hold (or @DesT), not @fees.
-// say that we need to send 10%*$amt (up to 5) to @fees; the rest to @dest +// send min(10% * $amt, 5) to @fees; send the rest to @dest @@ // now we empty the rest send [EUR *] ( source = $fees_hold - destination = @fees + destination = $dest_hold )If the goal was only “fail if $amt < 5”, updating the leading comment to reflect that would also resolve the mismatch.
Also applies to: 27-35
internal/interpreter/virtual_account_test.go (2)
26-33: Consider using a test helper type instead of anonymous struct.The
Postingtype is defined as an anonymous struct. Consider creating a type alias or importing the actual Posting type from the interpreter package to improve maintainability.Add a type alias at the top of the file:
type Posting = interpreter.Posting
381-409: Consider extracting the permutation helper to a test utilities package.The
permutefunction is a generic utility that could be useful for other test files. While the GPT-generated comment is honest, consider moving this to a shared test utilities package for reuse. The implementation itself is correct.Would you like me to create a test utilities package with this and other potentially reusable test helpers?
internal/interpreter/virtual_account.go (1)
173-174: TODO comment needs clarification or removal.The TODO comment on line 173 questions the overdraft implementation, but the code on line 174 appears to handle overdrafts correctly by checking if
remainingAmtis positive. Either clarify the concern or remove the TODO if it's resolved.Would you like me to help investigate the overdraft implementation to determine if this TODO can be resolved?
internal/interpreter/interpreter.go (3)
332-356: The function modifiestotalSentin place which could be confusing.The function takes
totalSent *big.Intand modifies it in place, then returns it. This dual approach (both modifying the parameter and returning it) could lead to confusion. The function namepushSenderalso doesn't indicate that it calculates and returns a total.Consider either:
- Only modifying the parameter in place (void function)
- Only returning the new total without modifying the parameter
- Renaming the function to better indicate its dual purpose (e.g.,
pushSenderAndAccumulate)
671-678: Inefficient retrieval and processing of credits.The code first gets credits, then pulls from them. This two-step process could be optimized.
Consider combining the operations if the API allows:
- fs := account.getCredits(s.CurrentAsset) - pulledSenders := fs.PullColored(amount, *color) + // Pull colored credits directly from the virtual account + pulledSenders := account.PullColoredCredits(s.CurrentAsset, amount, *color)
689-694: Complex logic for calculating additional send amount needs clarification.The calculation of
addionalSent(note the typo: should beadditionalSent) involves multiple conditions and could benefit from clearer variable naming and comments.- var addionalSent *big.Int + var additionalSent *big.Int if overdraft == nil { - addionalSent = new(big.Int).Set(missingAmt) + // Unlimited overdraft: send the full missing amount + additionalSent = new(big.Int).Set(missingAmt) } else { - // TODO check this is the correct number to eventually send - // TODO test overdraft - addionalSent = utils.MinBigInt(overdraft, missingAmt) + // Limited overdraft: send up to the overdraft limit + additionalSent = utils.MinBigInt(overdraft, missingAmt) } - s.pushSender(Sender{account, addionalSent, *color}, totalSent) + s.pushSender(Sender{account, additionalSent, *color}, totalSent)internal/interpreter/value.go (2)
12-20: Interface method names could be more descriptive.The
account()method name doesn't clearly indicate its purpose as a marker method. Consider using a more descriptive name likeisAccountValue()or following Go's convention with a private method likeaccountValue().type AccountValue interface { - account() + isAccountValue() // Marker method for AccountValue implementations String() string } type AccountAddress string -func (AccountAddress) account() {} -func (VirtualAccount) account() {} +func (AccountAddress) isAccountValue() {} +func (VirtualAccount) isAccountValue() {}
166-178: Function naming inconsistency and unclear separation of concerns.The function
expectAccountAddresssuggests it expects anAccountAddressvalue directly, but it actually expects anAccountand then extracts the address. This creates confusion about the function's purpose.Consider renaming for clarity:
-func expectAccountAddress(v Value, r parser.Range) (*string, InterpreterError) { +func expectAccountWithAddress(v Value, r parser.Range) (*string, InterpreterError) { acc, err := expectAccount(v, r) if err != nil { return nil, err } switch acc := acc.Repr.(type) { case AccountAddress: s := string(acc) return &s, nil default: - return nil, TypeError{Expected: analysis.TypeAccount, Value: v, Range: r} + // More specific error message + return nil, TypeError{Expected: "account with address", Value: v, Range: r} } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (18)
internal/interpreter/testdata/script-tests/experimental/virtual-account/account-create.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/account-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/bounded-overdraft-when-does-not-fail.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/creditors-stack.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/half-using-virtual.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-fail-if-not-enough.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-merchant-pays-fees-if-needed.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-no-commissions-with-low-amount.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-left-negative.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-when-fails.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/repay-self.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/send-credit-around.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/test-send-self-is-noop.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/transitive.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/wrong-currency.num.specs.jsonis excluded by!**/*.json
📒 Files selected for processing (33)
internal/analysis/check.go(2 hunks)internal/flags/flags.go(2 hunks)internal/interpreter/args_parser_test.go(2 hunks)internal/interpreter/batch_balances_query.go(3 hunks)internal/interpreter/evaluate_expr.go(1 hunks)internal/interpreter/function_exprs.go(5 hunks)internal/interpreter/function_statements.go(1 hunks)internal/interpreter/funds_stack.go(3 hunks)internal/interpreter/funds_stack_test.go(3 hunks)internal/interpreter/interpreter.go(11 hunks)internal/interpreter/interpreter_error.go(1 hunks)internal/interpreter/interpreter_test.go(3 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/account-create.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/account-kept.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/bounded-overdraft-when-does-not-fail.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/creditors-stack.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/half-using-virtual.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-fail-if-not-enough.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-merchant-pays-fees-if-needed.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-no-commissions-with-low-amount.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-left-negative.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-when-fails.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending-send-all.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/repay-self.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/send-credit-around.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/test-send-self-is-noop.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/transitive.num(1 hunks)internal/interpreter/testdata/script-tests/experimental/virtual-account/wrong-currency.num(1 hunks)internal/interpreter/value.go(4 hunks)internal/interpreter/virtual_account.go(1 hunks)internal/interpreter/virtual_account_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-16T14:51:51.052Z
Learnt from: ascandone
PR: formancehq/numscript#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/function_statements.go
📚 Learning: 2024-12-05T11:42:58.472Z
Learnt from: ascandone
PR: formancehq/numscript#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/args_parser_test.gointernal/interpreter/interpreter_test.go
🧬 Code graph analysis (13)
internal/interpreter/args_parser_test.go (3)
internal/interpreter/value.go (6)
Account(30-30)Account(38-38)AccountAddress(17-17)AccountAddress(19-19)Value(22-25)NewMonetaryInt(253-256)internal/interpreter/args_parser.go (1)
NewArgsParser(11-15)numscript.go (1)
Value(72-72)
internal/interpreter/funds_stack.go (1)
internal/interpreter/value.go (3)
Account(30-30)Account(38-38)AccountValue(12-15)
internal/analysis/check.go (3)
internal/analysis/version_check.go (1)
VersionClause(54-57)internal/parser/version.go (2)
Version(9-12)NewVersionInterpreter(28-34)internal/flags/flags.go (2)
FeatureFlag(3-3)ExperimentalVirtualAccount(13-13)
internal/interpreter/interpreter_error.go (1)
internal/interpreter/value.go (2)
Account(30-30)Account(38-38)
internal/interpreter/virtual_account_test.go (4)
internal/interpreter/virtual_account.go (2)
NewVirtualAccount(31-36)VirtualAccount(10-14)internal/interpreter/funds_stack.go (1)
Sender(7-11)internal/interpreter/value.go (7)
Account(30-30)Account(38-38)AccountAddress(17-17)AccountAddress(19-19)Asset(28-28)Asset(42-42)VirtualAccount(20-20)internal/interpreter/interpreter.go (1)
Posting(80-85)
internal/interpreter/virtual_account.go (4)
internal/interpreter/value.go (10)
VirtualAccount(20-20)String(27-27)String(37-37)AccountValue(12-15)AccountAddress(17-17)AccountAddress(19-19)Asset(28-28)Asset(42-42)Account(30-30)Account(38-38)internal/utils/utils.go (1)
MapGetOrPutDefault(50-58)internal/interpreter/funds_stack.go (1)
Sender(7-11)internal/interpreter/interpreter.go (1)
Posting(80-85)
internal/interpreter/funds_stack_test.go (2)
internal/interpreter/value.go (4)
Account(30-30)Account(38-38)AccountAddress(17-17)AccountAddress(19-19)internal/interpreter/funds_stack.go (1)
Sender(7-11)
internal/interpreter/batch_balances_query.go (1)
internal/interpreter/value.go (2)
AccountAddress(17-17)AccountAddress(19-19)
internal/interpreter/interpreter_test.go (2)
internal/interpreter/value.go (5)
Account(30-30)Account(38-38)AccountAddress(17-17)AccountAddress(19-19)Value(22-25)numscript.go (1)
Value(72-72)
internal/interpreter/function_exprs.go (4)
internal/interpreter/value.go (5)
Account(30-30)Account(38-38)AccountAddress(17-17)AccountAddress(19-19)Value(22-25)internal/interpreter/interpreter.go (1)
InterpreterError(73-76)internal/flags/flags.go (1)
ExperimentalVirtualAccount(13-13)internal/interpreter/virtual_account.go (1)
NewVirtualAccount(31-36)
internal/interpreter/interpreter.go (7)
internal/analysis/check.go (1)
FnVarOriginVirtual(64-64)internal/interpreter/value.go (9)
Account(30-30)Account(38-38)VirtualAccount(20-20)AccountAddress(17-17)AccountAddress(19-19)Asset(28-28)Asset(42-42)String(27-27)String(37-37)internal/interpreter/virtual_account.go (1)
VirtualAccount(10-14)internal/interpreter/funds_stack.go (1)
Sender(7-11)internal/parser/ast.go (4)
Destination(189-192)DestinationAccount(194-194)DestinationAccount(200-202)ValueExpr(8-11)internal/interpreter/interpreter_error.go (1)
InvalidUnboundedInSendAll(151-154)internal/utils/utils.go (2)
NonExhaustiveMatchPanic(8-10)MinBigInt(12-22)
internal/interpreter/value.go (5)
internal/interpreter/virtual_account.go (1)
VirtualAccount(10-14)internal/interpreter/interpreter.go (1)
InterpreterError(73-76)internal/interpreter/interpreter_error.go (2)
InvalidAccountName(215-218)TypeError(49-53)internal/parser/range.go (1)
Range(13-16)internal/analysis/check.go (1)
TypeAccount(14-14)
internal/interpreter/evaluate_expr.go (1)
internal/interpreter/value.go (2)
Account(30-30)Account(38-38)
🔇 Additional comments (36)
internal/interpreter/function_statements.go (2)
23-23: Good call: require a concrete account address for metadata.Using expectAccountAddress ensures metadata can’t be set on virtual accounts or non-concrete values. This aligns with the virtual-account model.
31-36: illationinternal/interpreter/evaluate_expr.go (2)
39-41: Ensure NewAccountAddress returns the Account wrapper.Given castToString now matches Account, returning Account from NewAccountAddress avoids subtle mismatches in downstream type switches.
218-221: Verify castToString handles raw AccountAddress
Confirm that in evaluate_expr.go the AST’s account‐literal branch always wraps addresses via NewAccountAddress (returning Account) and that castToString never sees a bare AccountAddress value. If direct AccountAddress values can still flow into castToString, add:case Account: return v.String(), nil + case AccountAddress: + return v.String(), nilinternal/interpreter/testdata/script-tests/experimental/virtual-account/transitive.num (1)
1-16: LGTM: clear transitive flow across two virtual accountsSequential sends world→v1→v2→dest exercise the transitive path well. Good coverage for balance propagation.
internal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending.num (1)
1-14: LGTM: good negative test for double-spending from the same virtual sourceListing $v twice in the source set should be rejected. Ensure the harness asserts the specific error surfaced by the interpreter for clarity.
internal/interpreter/testdata/script-tests/experimental/virtual-account/account-create.num (1)
1-11: LGTM: basic fund-and-spend with a virtual accountCovers the happy path world→$v then $v→dest. Nice baseline.
internal/interpreter/testdata/script-tests/experimental/virtual-account/half-using-virtual.num (1)
6-6: No changes needed: grammar supportsallowing unbounded overdraftinside composite sources. Tests acrossinternal/interpreter/testdata/script-testsalready use this modifier within braces, so the existing line is valid.internal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft.num (2)
5-8: LGTM: unbounded overdraft path covered.This exercises negative balance then later top-up; matches the described intent.
4-4: Comment syntax validated.//comments are consistently used in .num test files and supported by Numscript.internal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-when-fails.num (2)
8-11: Good negative case for bounded overdraft.Covers “credit 10 then attempt 100 with limit 1”; this should fail.
8-11: Annotate expected overdraft failure in the test script.
Ininternal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-when-fails.num, add a test-harness directive (e.g. a comment like# expect-error: NegativeBalance: overdraft exceeded) so the framework records and asserts the expected failure.internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-merchant-pays-fees-if-needed.num (1)
12-22: Confirm source stacking order ($fees_hold drained before @merchant).The intent seems to be “use $fees_hold, then fallback to @merchant.” Please verify interpreter behavior preserves this order and add a golden asserting which postings are produced when $fees_hold < 5.
internal/interpreter/args_parser_test.go (1)
39-40: Good: tests now pass Account wrapper, not bare AccountAddress.internal/interpreter/testdata/script-tests/experimental/virtual-account/creditors-stack.num (1)
16-19: Verify creditor ordering on later inflow.Please add/confirm goldens showing how the incoming 150 is applied against prior debits (ordering rules), so regressions are caught.
internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-no-commissions-with-low-amount.num (2)
1-5: Virtual accounts declared correctly; ensure flag/version are enabled in tests.This script depends on ExperimentalVirtualAccount and version >= 0.0.20. Please confirm the test harness sets the interpreter version and enables the flag, otherwise the run will fail at parse/exec time.
20-27: LGTM: final drains from holds mirror the intended flow.Transfers from $fees_hold to @fees and from $dest_hold to @DesT look consistent with the multi-hop design.
internal/interpreter/testdata/script-tests/experimental/virtual-account/send-credit-around.num (1)
18-22: Verify “no postings” assertion is covered.Please confirm the expected-results fixture asserts that this third send yields zero postings.
internal/interpreter/function_exprs.go (3)
89-96: Balance on virtual accounts.Identical static/runtime mismatch note as for overdraft/meta. Otherwise, change is correct.
Please verify existing tests cover passing virtual() to balance/meta/overdraft and produce the intended error.
103-107: NegativeBalanceError.Account wrapping.Using Account{AccountAddress(*account)} matches the new Account value model. Confirm all consumers of NegativeBalanceError now expect Account (not raw address).
159-167: virtual() builtin gated correctly.Feature-flag check and return shape look good. No further issues.
internal/interpreter/batch_balances_query.go (1)
123-126: Overdraft source batching respects virtual accounts.LGTM. Skips when overdraft is unbounded and when source is virtual.
internal/analysis/check.go (1)
64-65: New builtin symbol name is clear and consistent.FnVarOriginVirtual constant naming aligns with existing conventions.
internal/interpreter/funds_stack.go (3)
8-10: Move to AccountValue looks good; confirm equality semantics across implementationsStoring Account as AccountValue is aligned with the new abstraction. Please confirm all current AccountValue implementations are intended to be compared by value (addresses) or identity (virtual accounts) consistently across the stack logic.
83-91: Merge result construction is correct; amount addition is safeThe aggregated Sender keeps Account and Color and sums Amounts as expected.
155-168: Split/return path is correct; leftover is pushed back with preserved Account/ColorLogic for “more than enough” and exact split looks good and preserves invariants.
internal/interpreter/funds_stack_test.go (1)
12-13: LGTM! Clean migration to AccountAddress type.The refactoring from
NametoAccount: AccountAddress(...)improves type safety and aligns with the new account abstraction model.Also applies to: 17-18
internal/interpreter/virtual_account_test.go (2)
1-10: Test file structure looks good.Using a separate test package (
interpreter_test) promotes black-box testing and ensures the API is tested from a consumer perspective.
314-379: Excellent test coverage for commutativity.The commutative order test exhaustively validates that the order of operations doesn't affect the final outcome, which is crucial for a financial system. The test properly verifies all 24 permutations of the 4 operations.
internal/interpreter/virtual_account.go (3)
38-50: Good use of helper functions for lazy initialization.The
getCreditsandgetDebitshelper methods properly encapsulate the lazy initialization pattern using the utility function. This ensures consistent behavior across the codebase.
52-69: Clear and well-documented public API.The
Receivemethod has excellent documentation explaining the FIFO policy and debt repayment mechanism. The implementation correctly handles the debt-first repayment logic.
114-140: Good separation of concerns in repayWithSender.The helper function properly encapsulates the debt repayment logic and correctly manages the remaining amount calculation.
internal/interpreter/interpreter.go (4)
197-198: Remove redundant bounds‐check suggestion –virtual(s)already validates the ExperimentalVirtualAccount feature flag and returns an error on failure, so no additional validation is needed.Likely an incorrect or invalid review comment.
224-230: SettingDbgfield directly mutates the struct - consider immutability.The code creates a
VirtualAccountand then mutates itsDbgfield. This breaks immutability and could lead to unexpected behavior if the sameVirtualAccountis used elsewhere.Consider creating a new
VirtualAccountwith the debug name instead of mutating:- if acc, ok := value.(Account); ok { - if vacc, ok := acc.Repr.(VirtualAccount); ok { - vacc.Dbg = varsDecl.Name.Name - value = Account{vacc} - } - } + if acc, ok := value.(Account); ok { + if vacc, ok := acc.Repr.(VirtualAccount); ok { + // Create a new VirtualAccount with the debug name + newVacc := VirtualAccount{ + Dbg: varsDecl.Name.Name, + credits: vacc.credits, + debits: vacc.debits, + } + value = Account{newVacc} + } + }Likely an incorrect or invalid review comment.
543-557: Missing nil check foraccount.PullCreditsreturn value.The code calls
account.PullCredits(s.CurrentAsset)and iterates over the result, but doesn't check if it returns nil or handle potential errors.Add nil check for safety:
case VirtualAccount: totalSent := big.NewInt(0) senders := account.PullCredits(s.CurrentAsset) + if senders == nil { + return totalSent, nil + } for _, sender := range senders { s.pushSender(sender, totalSent) }Likely an incorrect or invalid review comment.
440-444: No extra error handling needed for account address conversion.evaluateExprAs(..., expectAccountAddress)already enforces and returns an error on invalid types.Likely an incorrect or invalid review comment.
| ExperimentalVirtualAccount FeatureFlag = "experimental-virtual-account" | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Flag added and wired into AllFlags — LGTM. Also verify usage coverage.
- OK to add ExperimentalVirtualAccount and include it in AllFlags.
- Optional: align naming with peers (e.g., ExperimentalVirtualAccountFeatureFlag) or type AllFlags as []FeatureFlag for readability (alias to string anyway).
Run to confirm the flag is actually gating features and tests:
Also applies to: 24-25
🏁 Script executed:
#!/bin/bash
rg -nC2 '\bExperimentalVirtualAccount\b'Length of output: 1295
Add unit tests for ExperimentalVirtualAccount gating
- Usage confirmed in internal/interpreter/function_exprs.go (checkFeatureFlag at line 162) and internal/analysis/check.go (FeatureFlag at line 125).
- No
_test.gofiles referenceExperimentalVirtualAccount; add tests covering both enabled and disabled scenarios to ensure the flag actually gates behavior.
🤖 Prompt for AI Agents
In internal/flags/flags.go around lines 13-14, there are no unit tests for the
ExperimentalVirtualAccount flag; add tests that verify behavior when the flag is
enabled and disabled. Create a new _test.go in the same package that (1) sets
ExperimentalVirtualAccount on, exercises the code paths that depend on it (e.g.,
call the feature-checking helper used by internal/interpreter/function_exprs.go
and any API/analysis functions referenced in internal/analysis/check.go) and
asserts the gated behavior is allowed, and (2) sets the flag off and asserts the
gated behavior is denied; ensure tests clean up/reset global flag state (use
defer to restore original value) and include clear assertions for both
scenarios.
| if first.Account != second.Account || first.Color != second.Color { | ||
| return | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid interface equality that can panic for non-comparable AccountValue types; make equality explicit
Comparing interface values with “!=” panics if the dynamic type is not comparable. Also, equality semantics may differ by implementation (string address vs virtual account identity). Use an explicit comparer.
- if first.Account != second.Account || first.Color != second.Color {
+ if !accountsEqual(first.Account, second.Account) || first.Color != second.Color {
return
}Add helper (outside the shown range) and import reflect:
import "reflect"
// accountsEqual defines equality semantics for AccountValue.
// - AccountAddress: value equality
// - pointer-backed implementations (e.g., virtual accounts): pointer identity
// - fallback: DeepEqual (safe for non-comparable structs)
func accountsEqual(a, b AccountValue) bool {
if a == nil || b == nil {
return a == b
}
ta, tb := reflect.TypeOf(a), reflect.TypeOf(b)
if ta != tb {
return false
}
va, vb := reflect.ValueOf(a), reflect.ValueOf(b)
switch ta.Kind() {
case reflect.String, reflect.Int, reflect.Int64, reflect.Uint, reflect.Uint64, reflect.Bool:
return a == b
case reflect.Ptr, reflect.UnsafePointer:
return va.Pointer() == vb.Pointer()
default:
return reflect.DeepEqual(a, b)
}
}🏁 Script executed:
#!/bin/bash
set -eu
# Locate the definition of AccountValue
rg -nP 'type\s+AccountValue' -C3 -g '*.go'
# Preview imports in internal/interpreter/funds_stack.go
sed -n '1,50p' internal/interpreter/funds_stack.go
# Show context around lines 79-81
sed -n '60,100p' internal/interpreter/funds_stack.go
# Check whether reflect is already imported
rg -n 'reflect' internal/interpreter/funds_stack.goLength of output: 2974
🏁 Script executed:
#!/bin/bash
set -eu
# Show AccountValue definition and its implementers in value.go
sed -n '1,200p' internal/interpreter/value.go
# Locate all types implementing the account() method
rg -nP '\.account\(' -C3 -g '*.go'Length of output: 4506
🏁 Script executed:
#!/bin/bash
set -eu
# Locate the definition of VirtualAccount
rg -nP 'type\s+VirtualAccount\b' -C3 -g '*.go'
# If found, show its full definition (context +/-10 lines)
FILE_AND_LINE=$(rg -nP 'type\s+VirtualAccount\b' -n -g '*.go' | head -n1)
if [ -n "$FILE_AND_LINE" ]; then
FILE=$(echo "$FILE_AND_LINE" | cut -d: -f1)
LINE=$(echo "$FILE_AND_LINE" | cut -d: -f2)
start=$(( LINE > 10 ? LINE - 10 : 1 ))
sed -n "${start},$((LINE+10))p" "$FILE"
fiLength of output: 1415
Replace interface equality on AccountValue with explicit helper
Dynamic types like VirtualAccount contain non-comparable maps, so first.Account != second.Account will panic at runtime. Change the check to:
if !accountsEqual(first.Account, second.Account) || first.Color != second.Color {
return
}Import "reflect" and add an accountsEqual(a, b AccountValue) bool helper that handles nil, enforces same type, compares pointers for pointer-backed types, and falls back to reflect.DeepEqual.
🤖 Prompt for AI Agents
internal/interpreter/funds_stack.go around lines 79 to 81: the current equality
check uses interface equality (first.Account != second.Account) which can panic
for dynamic types containing non-comparable fields; replace that condition with
a call to a new helper accountsEqual(first.Account, second.Account) and keep the
Color check; add import "reflect" and implement accountsEqual(a, b AccountValue)
bool that returns true for both nil, ensures types match, for pointer-backed
concrete types compares pointer addresses when possible, and otherwise falls
back to reflect.DeepEqual to determine equality.
| @@ -0,0 +1,9 @@ | |||
| vars { account $v = virtual() } | |||
|
|
|||
| send [USD/2 10] ( | |||
There was a problem hiding this comment.
Fix asset syntax: [USD/2 10] should be [USD 10].
The fraction is already specified in the source block; putting /2 in the asset token will fail to parse as a valid asset.
Apply this diff:
-send [USD/2 10] (
+send [USD 10] (📝 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.
| send [USD/2 10] ( | |
| send [USD 10] ( |
🤖 Prompt for AI Agents
In
internal/interpreter/testdata/script-tests/experimental/virtual-account/half-using-virtual.num
around line 3, the asset token `[USD/2 10]` is invalid because the fraction is
defined elsewhere; change the asset syntax to `[USD 10]` by removing the `/2`
from the asset token so the line reads the correct asset form.
| func NewAccountAddress(src string) (Account, InterpreterError) { | ||
| if !validateAddress(src) { | ||
| return AccountAddress(""), InvalidAccountName{Name: src} | ||
| return Account{AccountAddress("")}, InvalidAccountName{Name: src} | ||
| } | ||
| return AccountAddress(src), nil | ||
| return Account{AccountAddress(src)}, nil | ||
| } |
There was a problem hiding this comment.
Error handling returns invalid state.
When validation fails, the function returns Account{AccountAddress("")} with an error. This creates an invalid Account object that could cause issues if the error is ignored.
Return a zero value or nil-like representation instead:
func NewAccountAddress(src string) (Account, InterpreterError) {
if !validateAddress(src) {
- return Account{AccountAddress("")}, InvalidAccountName{Name: src}
+ return Account{}, InvalidAccountName{Name: src}
}
return Account{AccountAddress(src)}, nil
}📝 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.
| func NewAccountAddress(src string) (Account, InterpreterError) { | |
| if !validateAddress(src) { | |
| return AccountAddress(""), InvalidAccountName{Name: src} | |
| return Account{AccountAddress("")}, InvalidAccountName{Name: src} | |
| } | |
| return AccountAddress(src), nil | |
| return Account{AccountAddress(src)}, nil | |
| } | |
| func NewAccountAddress(src string) (Account, InterpreterError) { | |
| if !validateAddress(src) { | |
| return Account{}, InvalidAccountName{Name: src} | |
| } | |
| return Account{AccountAddress(src)}, nil | |
| } |
🤖 Prompt for AI Agents
internal/interpreter/value.go around lines 44-49: the function currently returns
a partially populated Account{AccountAddress("")} when validation fails which
creates an invalid object; change the failure return to the zero-value Account{}
(i.e., return Account{}, InvalidAccountName{Name: src}) so callers can't
accidentally use an invalid Account when they ignore the error.
| v0.Dbg = "v0" | ||
|
|
||
| v1 := interpreter.NewVirtualAccount() | ||
| v0.Dbg = "v1" |
There was a problem hiding this comment.
Fix incorrect debug field assignment.
Line 139 incorrectly assigns to v0.Dbg instead of v1.Dbg, which defeats the purpose of labeling the virtual accounts for debugging.
Apply this diff to fix the assignment:
- v0.Dbg = "v1"
+ v1.Dbg = "v1"📝 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.
| v0.Dbg = "v1" | |
| v1.Dbg = "v1" |
🤖 Prompt for AI Agents
In internal/interpreter/virtual_account_test.go around line 139, the test
incorrectly sets the debug label on v0 (v0.Dbg = "v1") instead of v1; change the
assignment to set v1.Dbg = "v1" so each virtual account has its intended debug
label (verify v0.Dbg remains "v0" and update only the single line to assign the
correct variable).
| func (v VirtualAccount) WithDbg(dbg string) VirtualAccount { | ||
| v.Dbg = dbg | ||
| return v | ||
| } |
There was a problem hiding this comment.
Consider using pointer receiver for WithDbg method.
The WithDbg method uses a value receiver and returns a copy of the VirtualAccount. Since VirtualAccount contains maps (reference types), this creates a shallow copy where the maps are shared between instances. This could lead to unexpected behavior.
Apply this diff to use a pointer receiver:
-func (v VirtualAccount) WithDbg(dbg string) VirtualAccount {
- v.Dbg = dbg
- return v
+func (v *VirtualAccount) WithDbg(dbg string) *VirtualAccount {
+ v.Dbg = dbg
+ return v
}📝 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.
| func (v VirtualAccount) WithDbg(dbg string) VirtualAccount { | |
| v.Dbg = dbg | |
| return v | |
| } | |
| func (v *VirtualAccount) WithDbg(dbg string) *VirtualAccount { | |
| v.Dbg = dbg | |
| return v | |
| } |
🤖 Prompt for AI Agents
internal/interpreter/virtual_account.go around lines 16 to 19: change WithDbg to
use a pointer receiver so it mutates the existing VirtualAccount instead of
returning a shallow-copied value; update the signature to func (v
*VirtualAccount) WithDbg(dbg string) *VirtualAccount { v.Dbg = dbg; return v }
and then update any call sites that expect a value return to accept the pointer
(or explicitly dereference) to preserve chaining while avoiding shared-map copy
issues.
| }} | ||
| case VirtualAccount: | ||
| // should be unreachable | ||
| panic("Unhandled: send from addr to virtual account") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning errors instead of panicking.
The send function panics in two unreachable cases. While these cases should theoretically never occur, panicking in production code can crash the entire application. Consider returning an error instead.
Apply this diff to handle errors gracefully:
func send(
source AccountValue,
destination AccountValue,
amount *big.Int,
asset string,
color string,
-) []Posting {
+) ([]Posting, error) {
switch source := source.(type) {
case AccountAddress:
switch destination := destination.(type) {
case AccountAddress:
- return []Posting{{
+ return []Posting{{
Source: string(source),
Destination: string(destination),
Amount: amount,
Asset: coloredAsset(asset, &color),
- }}
+ }}, nil
case VirtualAccount:
- panic("Unhandled: send from addr to virtual account")
+ return nil, fmt.Errorf("unhandled: send from addr to virtual account")
}
case VirtualAccount:
switch dest := destination.(type) {
case AccountAddress:
- return source.Pull(asset, Sender{
+ postings := source.Pull(asset, Sender{
Account: dest,
Amount: amount,
Color: color,
})
+ return postings, nil
case VirtualAccount:
- panic("Unhandled: send from virtual account to virtual account")
+ return nil, fmt.Errorf("unhandled: send from virtual account to virtual account")
}
}
- panic("non exhaustive match")
+ return nil, fmt.Errorf("non exhaustive match in send function")
}Note: This change would require updating callers to handle the error.
Also applies to: 106-106
🤖 Prompt for AI Agents
In internal/interpreter/virtual_account.go at lines 91 and 106, replace the
panics ("Unhandled: send from addr to virtual account") with returned errors:
change the send function signature to return an error (or add an error return if
it already returns values), create and return a descriptive error (e.g.,
fmt.Errorf or a package-level sentinel error) at those two branches, and update
all callers to handle and propagate or log the error appropriately; ensure tests
and call sites are adjusted to handle the new error return.
| pulled := credits.PullColored(receiver.Amount, receiver.Color) | ||
|
|
||
| remainingAmt := new(big.Int).Set(receiver.Amount) | ||
|
|
There was a problem hiding this comment.
Potential bug: remainingAmt is never decremented in Pull method.
The remainingAmt variable is initialized to receiver.Amount but is never decremented when postings are created. This means the overdraft logic will always trigger if any amount is requested, even if sufficient credits are available.
Apply this diff to fix the bug:
for _, pulledSender := range pulled {
newPostings := send(
pulledSender.Account,
receiver.Account,
pulledSender.Amount,
asset,
receiver.Color,
)
postings = append(postings, newPostings...)
+ remainingAmt.Sub(remainingAmt, pulledSender.Amount)
}Also applies to: 173-183
🤖 Prompt for AI Agents
In internal/interpreter/virtual_account.go around line 159 (and similarly for
the block at 173-183), remainingAmt is initialized to receiver.Amount but never
decremented while creating postings, so overdraft logic always triggers; fix by
decrementing remainingAmt each time you allocate funds to a posting: inside the
loop over available credits compute allocated = min(credit.Amount,
remainingAmt), create a posting for allocated, subtract allocated from
remainingAmt, and stop iterating when remainingAmt == 0; after the loop, if
remainingAmt > 0 then trigger the overdraft behavior (or return an error) as
originally intended.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/interpreter/interpreter.go (1)
524-553: Honor requested asset color in VirtualAccount send-allIn the VirtualAccount branch of sendAll (internal/interpreter/interpreter.go around line 547), we currently call
senders := account.PullCredits(s.CurrentAsset)which unconditionally drains all colors. Instead, when
coloris non-empty we must filter by that color—e.g.:senders := s.getCredits(s.CurrentAsset).PullColored(*color)and only fall back to
PullAll()ifcolor == "".
♻️ Duplicate comments (1)
internal/interpreter/interpreter.go (1)
395-403: Validate Pull or pre-check availability (repeat from prior review).Pull on a VirtualAccount is unchecked. Validate its result or pre-check available credits/debt policy; propagate an error on failure instead of proceeding silently.
🧹 Nitpick comments (3)
internal/interpreter/function_exprs.go (1)
22-24: Overdraft should not accept virtual accounts (clarify TODO or implement).You switched to expectAccountAddress, which effectively excludes virtual accounts. If that’s the intended semantics, drop the TODO. If overdraft must support virtual accounts later, ensure getBalance(Account) handles VirtualAccount correctly (see my getBalance fix in interpreter.go).
internal/interpreter/interpreter.go (2)
225-231: Prefer named field init for clarity; copy semantics are fine.Setting vacc.Dbg then re-wrapping avoids pointer aliasing. Minor clarity nit: initialize Account with a named field.
- value = Account{vacc} + value = Account{Repr: vacc}
259-261: Remove unused virtualAccountsCredits and virtualAccountsDebts
ThevirtualAccountsCreditsandvirtualAccountsDebtsfields (declared at lines 327–328) and their initializations (lines 259–260) aren’t referenced anywhere else. Drop them to eliminate dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/interpreter/function_exprs.go(3 hunks)internal/interpreter/interpreter.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/interpreter/interpreter.go (6)
internal/analysis/check.go (1)
FnVarOriginVirtual(64-64)internal/interpreter/value.go (5)
Account(30-30)Account(38-38)VirtualAccount(20-20)AccountAddress(17-17)AccountAddress(19-19)internal/interpreter/virtual_account.go (1)
VirtualAccount(10-14)internal/interpreter/funds_stack.go (1)
Sender(7-11)internal/interpreter/interpreter_error.go (2)
InvalidUnboundedInSendAll(151-154)QueryBalanceError(179-182)internal/utils/utils.go (2)
NonExhaustiveMatchPanic(8-10)MinBigInt(12-22)
internal/interpreter/function_exprs.go (4)
internal/interpreter/value.go (5)
Account(30-30)Account(38-38)AccountAddress(17-17)AccountAddress(19-19)Value(22-25)internal/interpreter/interpreter.go (1)
InterpreterError(73-76)internal/flags/flags.go (1)
ExperimentalVirtualAccount(13-13)internal/interpreter/virtual_account.go (1)
NewVirtualAccount(31-36)
⏰ 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 (10)
internal/interpreter/function_exprs.go (3)
29-31: OK: use Account wrapper for balance lookup.Wrapping AccountAddress into Account for getBalance aligns with the new API.
56-61: OK: meta() restricted to concrete account addresses.Switching to expectAccountAddress avoids ambiguous virtual/meta lookups.
159-167: virtual() builtin correctly gated and returns Account.Feature-flag check is correct; returning Account{Repr: NewVirtualAccount()} matches the new value model.
internal/interpreter/interpreter.go (7)
197-199: Wiring virtual() into handleFnCall looks good.The new case analysis.FnVarOriginVirtual delegates to virtual(s) as expected.
331-357: pushSender: zero-amount fast-path and balance mutation LGTM.Early return on zero and per-color balance subtraction for real senders are appropriate.
359-369: pushReceiver: correct dispatcher for AccountAddress vs VirtualAccount.Delegation to pushVirtualReceiver / pushReceiverAddress is clear and cohesive.
371-374: Virtual receive doesn’t sync CachedBalances; confirm invariants.vacc.Receive appends postings but doesn’t update CachedBalances. Ensure invariants don’t rely on CachedBalances reflecting virtual settlements mid-statement; if they do, update balances accordingly.
441-447: runSaveStatement: restricting to AccountAddress is correct.expectAccountAddress matches the storage model for saves.
811-813: Early return on zero receive is good.Avoids unnecessary stack ops and postings for no-ops.
919-921: Kept destination wrapping is correct.Wrapping KEPT_ADDR as Account{AccountAddress(…)} unifies downstream handling.
| switch account := account.Repr.(type) { | ||
| case AccountAddress: | ||
| var actuallySentAmt *big.Int | ||
| if overdraft == nil { | ||
| // unbounded overdraft: we send the required amount | ||
| actuallySentAmt = new(big.Int).Set(amount) | ||
| } else { | ||
| balance := s.CachedBalances.fetchBalance(string(account), s.CurrentAsset, *color) | ||
|
|
||
| // that's the amount we are allowed to send (balance + overdraft) | ||
| actuallySentAmt = CalculateSafeWithdraw(balance, overdraft, amount) | ||
| // that's the amount we are allowed to send (balance + overdraft) | ||
| actuallySentAmt = CalculateSafeWithdraw(balance, overdraft, amount) | ||
| } | ||
| return s.pushSender(Sender{account, actuallySentAmt, *color}, nil), nil | ||
|
|
||
| case VirtualAccount: | ||
| totalSent := big.NewInt(0) | ||
|
|
||
| fs := account.getCredits(s.CurrentAsset) | ||
| pulledSenders := fs.PullColored(amount, *color) | ||
|
|
||
| for _, sender := range pulledSenders { | ||
| s.pushSender(sender, totalSent) | ||
| } | ||
|
|
||
| // if we didn't pull enough | ||
| if totalSent.Cmp(amount) == -1 { | ||
|
|
||
| // invariant: missingAmt > 0 | ||
| // (we never pull more than required) | ||
| missingAmt := new(big.Int).Sub(amount, totalSent) | ||
|
|
||
| var addionalSent *big.Int | ||
| if overdraft == nil { | ||
| addionalSent = new(big.Int).Set(missingAmt) | ||
| } else { | ||
| // TODO check this is the correct number to eventually send | ||
| // TODO test overdraft | ||
| addionalSent = utils.MinBigInt(overdraft, missingAmt) | ||
| } | ||
|
|
||
| s.pushSender(Sender{account, addionalSent, *color}, totalSent) | ||
| } | ||
|
|
||
| return totalSent, nil | ||
|
|
||
| default: | ||
| utils.NonExhaustiveMatchPanic[any](account) | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fallback to PullAnything when no color is specified.
If evaluateColor returns "", PullColored likely narrows to uncolored only. Use PullAnything in that case to honor “any color”.
Apply this localized change:
- fs := account.getCredits(s.CurrentAsset)
- pulledSenders := fs.PullColored(amount, *color)
+ fs := account.getCredits(s.CurrentAsset)
+ var pulledSenders []Sender
+ if *color == "" {
+ pulledSenders = fs.PullAnything(amount)
+ } else {
+ pulledSenders = fs.PullColored(amount, *color)
+ }📝 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.
| switch account := account.Repr.(type) { | |
| case AccountAddress: | |
| var actuallySentAmt *big.Int | |
| if overdraft == nil { | |
| // unbounded overdraft: we send the required amount | |
| actuallySentAmt = new(big.Int).Set(amount) | |
| } else { | |
| balance := s.CachedBalances.fetchBalance(string(account), s.CurrentAsset, *color) | |
| // that's the amount we are allowed to send (balance + overdraft) | |
| actuallySentAmt = CalculateSafeWithdraw(balance, overdraft, amount) | |
| // that's the amount we are allowed to send (balance + overdraft) | |
| actuallySentAmt = CalculateSafeWithdraw(balance, overdraft, amount) | |
| } | |
| return s.pushSender(Sender{account, actuallySentAmt, *color}, nil), nil | |
| case VirtualAccount: | |
| totalSent := big.NewInt(0) | |
| fs := account.getCredits(s.CurrentAsset) | |
| pulledSenders := fs.PullColored(amount, *color) | |
| for _, sender := range pulledSenders { | |
| s.pushSender(sender, totalSent) | |
| } | |
| // if we didn't pull enough | |
| if totalSent.Cmp(amount) == -1 { | |
| // invariant: missingAmt > 0 | |
| // (we never pull more than required) | |
| missingAmt := new(big.Int).Sub(amount, totalSent) | |
| var addionalSent *big.Int | |
| if overdraft == nil { | |
| addionalSent = new(big.Int).Set(missingAmt) | |
| } else { | |
| // TODO check this is the correct number to eventually send | |
| // TODO test overdraft | |
| addionalSent = utils.MinBigInt(overdraft, missingAmt) | |
| } | |
| s.pushSender(Sender{account, addionalSent, *color}, totalSent) | |
| } | |
| return totalSent, nil | |
| default: | |
| utils.NonExhaustiveMatchPanic[any](account) | |
| return nil, nil | |
| } | |
| case VirtualAccount: | |
| totalSent := big.NewInt(0) | |
| fs := account.getCredits(s.CurrentAsset) | |
| var pulledSenders []Sender | |
| if *color == "" { | |
| pulledSenders = fs.PullAnything(amount) | |
| } else { | |
| pulledSenders = fs.PullColored(amount, *color) | |
| } | |
| for _, sender := range pulledSenders { | |
| s.pushSender(sender, totalSent) | |
| } | |
| // if we didn't pull enough | |
| if totalSent.Cmp(amount) == -1 { | |
| // invariant: missingAmt > 0 | |
| // (we never pull more than required) | |
| missingAmt := new(big.Int).Sub(amount, totalSent) | |
| var addionalSent *big.Int | |
| if overdraft == nil { | |
| addionalSent = new(big.Int).Set(missingAmt) | |
| } else { | |
| // TODO check this is the correct number to eventually send | |
| // TODO test overdraft | |
| addionalSent = utils.MinBigInt(overdraft, missingAmt) | |
| } | |
| s.pushSender(Sender{account, addionalSent, *color}, totalSent) | |
| } | |
| return totalSent, nil |
🤖 Prompt for AI Agents
internal/interpreter/interpreter.go around lines 656 to 704: in the
VirtualAccount branch we currently call fs.PullColored(amount, *color) which
narrows pulls to a specific color and will ignore the “any color” semantics when
evaluateColor returned an empty string; change the logic to check if *color ==
"" and call fs.PullAnything(amount) in that case, otherwise call
fs.PullColored(amount, *color); wire the resulting pulledSenders through the
same loop and keep the existing missingAmt/overdraft fallback behavior so
semantics are preserved.
| switch accountRepr := account.Repr.(type) { | ||
| case AccountAddress: | ||
| account := string(accountRepr) | ||
| s.batchQuery(account, asset, nil) | ||
| fetchBalanceErr := s.runBalancesQuery() | ||
| if fetchBalanceErr != nil { | ||
| return nil, QueryBalanceError{WrappedError: fetchBalanceErr} | ||
| } | ||
| balance := s.CachedBalances.fetchBalance(account, asset, "") | ||
| return balance, nil | ||
|
|
||
| case VirtualAccount: | ||
| fs := accountRepr.credits[s.CurrentAsset] | ||
| if fs == nil { | ||
| return big.NewInt(0), nil | ||
| } | ||
|
|
||
| lst := fs.senders | ||
| sum := big.NewInt(0) | ||
|
|
||
| for lst != nil { | ||
| if lst.Head.Color == "" { | ||
| sum.Add(sum, lst.Head.Amount) | ||
| } | ||
| lst = lst.Tail | ||
| } | ||
| return sum, nil | ||
|
|
||
| default: | ||
| utils.NonExhaustiveMatchPanic[any](account.Repr) | ||
| return nil, nil | ||
|
|
There was a problem hiding this comment.
getBalance uses CurrentAsset for VirtualAccount — wrong asset.
VirtualAccount balance computation ignores the asset param and reads s.CurrentAsset, which can be unset or different. Use the function’s asset argument. Also, keep summing only uncolored credits to match address-path behavior.
Apply this fix:
func getBalance(
s *programState,
- account Account,
- asset string,
+ account Account,
+ asset string,
) (*big.Int, InterpreterError) {
switch accountRepr := account.Repr.(type) {
case AccountAddress:
account := string(accountRepr)
s.batchQuery(account, asset, nil)
fetchBalanceErr := s.runBalancesQuery()
if fetchBalanceErr != nil {
return nil, QueryBalanceError{WrappedError: fetchBalanceErr}
}
balance := s.CachedBalances.fetchBalance(account, asset, "")
return balance, nil
case VirtualAccount:
- fs := accountRepr.credits[s.CurrentAsset]
+ fs := accountRepr.credits[asset]
if fs == nil {
return big.NewInt(0), nil
}
lst := fs.senders
sum := big.NewInt(0)
for lst != nil {
if lst.Head.Color == "" {
sum.Add(sum, lst.Head.Amount)
}
lst = lst.Tail
}
return sum, nil
default:
utils.NonExhaustiveMatchPanic[any](account.Repr)
return nil, nil
}
}📝 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.
| switch accountRepr := account.Repr.(type) { | |
| case AccountAddress: | |
| account := string(accountRepr) | |
| s.batchQuery(account, asset, nil) | |
| fetchBalanceErr := s.runBalancesQuery() | |
| if fetchBalanceErr != nil { | |
| return nil, QueryBalanceError{WrappedError: fetchBalanceErr} | |
| } | |
| balance := s.CachedBalances.fetchBalance(account, asset, "") | |
| return balance, nil | |
| case VirtualAccount: | |
| fs := accountRepr.credits[s.CurrentAsset] | |
| if fs == nil { | |
| return big.NewInt(0), nil | |
| } | |
| lst := fs.senders | |
| sum := big.NewInt(0) | |
| for lst != nil { | |
| if lst.Head.Color == "" { | |
| sum.Add(sum, lst.Head.Amount) | |
| } | |
| lst = lst.Tail | |
| } | |
| return sum, nil | |
| default: | |
| utils.NonExhaustiveMatchPanic[any](account.Repr) | |
| return nil, nil | |
| func getBalance( | |
| s *programState, | |
| account Account, | |
| asset string, | |
| ) (*big.Int, InterpreterError) { | |
| switch accountRepr := account.Repr.(type) { | |
| case AccountAddress: | |
| account := string(accountRepr) | |
| s.batchQuery(account, asset, nil) | |
| fetchBalanceErr := s.runBalancesQuery() | |
| if fetchBalanceErr != nil { | |
| return nil, QueryBalanceError{WrappedError: fetchBalanceErr} | |
| } | |
| balance := s.CachedBalances.fetchBalance(account, asset, "") | |
| return balance, nil | |
| case VirtualAccount: | |
| // Use the function’s asset argument instead of s.CurrentAsset | |
| fs := accountRepr.credits[asset] | |
| if fs == nil { | |
| return big.NewInt(0), nil | |
| } | |
| lst := fs.senders | |
| sum := big.NewInt(0) | |
| for lst != nil { | |
| if lst.Head.Color == "" { | |
| sum.Add(sum, lst.Head.Amount) | |
| } | |
| lst = lst.Tail | |
| } | |
| return sum, nil | |
| default: | |
| utils.NonExhaustiveMatchPanic[any](account.Repr) | |
| return nil, nil | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/interpreter/interpreter.go around lines 996 to 1027, the
VirtualAccount branch is using s.CurrentAsset instead of the function's asset
parameter and thus may compute balances for the wrong asset; change fs :=
accountRepr.credits[s.CurrentAsset] to fs := accountRepr.credits[asset] so the
lookup uses the passed-in asset, and keep the existing Color == "" check so only
uncolored credits are summed (preserving address-path behavior); ensure nil
checks remain and return the computed sum as before.
e69fbe3 to
114e6f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/interpreter/interpreter_error.go (1)
115-123:⚠️ Potential issue | 🟡 MinorError message assumes address-style account with
@prefix.Line 122 formats the account as
@%s, which is meaningful forAccountAddressbut potentially misleading for aVirtualAccount. Since the field is now typed asAccount(which can hold either variant), the@prefix may produce a confusing message for virtual accounts.Consider using
%salone and lettingAccount.String()handle the prefix, or branching the format based on the underlyingReprtype.#!/bin/bash # Check what Account.String() returns for both variants ast-grep --pattern $'func (a Account) String() string { $$$ }'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/interpreter_error.go` around lines 115 - 123, The Error() message on NegativeBalanceError currently hardcodes an '@' prefix when printing e.Account; update it to avoid assuming an address-style account by either printing e.Account with "%s" (relying on Account.String()) or by checking the underlying Account.Repr/type and formatting accordingly; change the NegativeBalanceError.Error method to remove the '@%s' literal and use the Account's String() representation or a small branch so VirtualAccount messages are not misleading (refer to NegativeBalanceError.Error, Account.String, and the Account/Repr variants).
♻️ Duplicate comments (7)
internal/flags/flags.go (1)
13-13: Naming inconsistency with peer flags.
ExperimentalVirtualAccountomits theFeatureFlagsuffix used by some peers (e.g.,ExperimentalOverdraftFunctionFeatureFlag). That said, other recent flags (ExperimentalAssetColors,ExperimentalMidScriptFunctionCall) also omit it, so the convention is already mixed. A minor nit — consider a follow-up to standardize all flag names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/flags/flags.go` at line 13, The flag name ExperimentalVirtualAccount is inconsistent with peers that include the FeatureFlag suffix (e.g., ExperimentalOverdraftFunctionFeatureFlag); pick the convention to standardize (recommend adding the suffix) and rename ExperimentalVirtualAccount to ExperimentalVirtualAccountFeatureFlag in internal/flags/flags.go, then update all usages/imports to the new symbol name (also consider aligning ExperimentalAssetColors and ExperimentalMidScriptFunctionCall to the chosen convention in a follow-up to keep names consistent across the codebase).internal/interpreter/funds_stack_test.go (1)
126-126: Positional struct literal forSender— already flagged.Lines 126, 166-167, 179, and 187 still use positional initialization (e.g.,
{AccountAddress("src"), big.NewInt(10), "X"}) while the rest of the file uses named fields. This was noted in a previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/funds_stack_test.go` at line 126, Replace the positional struct literal for Sender with a keyed (named) field literal wherever it appears (e.g., change occurrences like {AccountAddress("src"), big.NewInt(10), "X"} to a Sender{FieldNameA: AccountAddress("src"), FieldNameB: big.NewInt(10), FieldNameC: "X"} using the actual field names from the Sender type); update all instances flagged (the literal at the shown diff plus the ones noted later) so the file consistently uses named fields instead of positional initialization.internal/interpreter/value.go (1)
44-49:⚠️ Potential issue | 🟡 MinorReturn zero-value
Accounton validation failure.On Line [46], returning
Account{AccountAddress("")}leaks an invalid account object when callers ignore the error.✅ Proposed fix
func NewAccountAddress(src string) (Account, InterpreterError) { if !validateAddress(src) { - return Account{AccountAddress("")}, InvalidAccountName{Name: src} + return Account{}, InvalidAccountName{Name: src} } return Account{AccountAddress(src)}, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/value.go` around lines 44 - 49, NewAccountAddress currently returns a non-zero Account (Account{AccountAddress("")}) on validation failure which can leak an invalid object; change the failure return to the zero-value Account{} while keeping the same InterpreterError return (i.e., in NewAccountAddress return Account{}, InvalidAccountName{Name: src}) so callers can't silently receive a malformed Account; update the return in NewAccountAddress and run tests to ensure behavior remains consistent.internal/interpreter/interpreter.go (2)
1007-1009:⚠️ Potential issue | 🟠 MajorUse the function
assetargument for virtual-account balance lookup.On Line [1008],
getBalancereadss.CurrentAssetinstead ofasset, which can return balances for the wrong asset.🩹 Proposed fix
case VirtualAccount: - fs := accountRepr.credits[s.CurrentAsset] + fs := accountRepr.credits[asset] if fs == nil { return big.NewInt(0), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/interpreter.go` around lines 1007 - 1009, In the getBalance implementation inside the switch case for VirtualAccount, the code uses s.CurrentAsset when indexing accountRepr.credits (fs := accountRepr.credits[s.CurrentAsset]) which ignores the function parameter asset; change that lookup to use the asset parameter (fs := accountRepr.credits[asset]) so the balance returned corresponds to the requested asset, keeping the existing nil-check and subsequent logic intact.
395-403:⚠️ Potential issue | 🟠 MajorHandle
KEPT_ADDRbefore pulling from virtual accounts.On Line [398], the VirtualAccount branch still pulls when destination is kept. This diverges from the AccountAddress path and mutates virtual-account state for funds that should be kept.
🧭 Proposed fix
case VirtualAccount: // Here we have a debt from a virtual acc. // we don't want to emit that as a posting (but TODO check how does it interact with kept) + if name == KEPT_ADDR { + return + } senderAccountAddress.Pull(st.CurrentAsset, Sender{ AccountAddress(name), sender.Amount, sender.Color, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/interpreter.go` around lines 395 - 403, The VirtualAccount branch in the switch still calls senderAccountAddress.Pull(...) even when the destination is KEPT_ADDR, which mutates virtual-account state incorrectly; update the VirtualAccount case in interpreter.go to check for KEPT_ADDR (same condition used in the AccountAddress path) before calling senderAccountAddress.Pull, and skip the Pull when the destination is kept so virtual account state is not changed for kept funds, leaving the rest of the branch logic unchanged.internal/interpreter/virtual_account.go (1)
162-183:⚠️ Potential issue | 🔴 Critical
remainingAmtis never reduced duringPull, so debt is over-recorded.Line [158] initializes
remainingAmt, but the loop at Line [162]-[171] never subtracts pulled amounts. The check at Line [174] then pushes debt even when credits were already consumed.🔧 Proposed fix
for _, pulledSender := range pulled { newPostings := send( pulledSender.Account, receiver.Account, pulledSender.Amount, asset, receiver.Color, ) postings = append(postings, newPostings...) + remainingAmt.Sub(remainingAmt, pulledSender.Amount) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/virtual_account.go` around lines 162 - 183, The loop that processes pulled senders never reduces remainingAmt, causing debt to be recorded even when pulls satisfied the amount; inside the for loop that iterates over pulled (the block that calls send(pulledSender.Account,...)), subtract pulledSender.Amount from remainingAmt (e.g., remainingAmt.Sub(remainingAmt, pulledSender.Amount)) and break early if remainingAmt.Cmp(big.NewInt(0)) <= 0 so the subsequent overdraft block (vacc.getDebits and Push of Sender) only runs when a positive remainingAmt truly remains.internal/interpreter/funds_stack.go (1)
79-81:⚠️ Potential issue | 🔴 CriticalReplace interface equality on
AccountValueto avoid runtime panic.On Line [79],
first.Account != second.Accountcan panic when the dynamic type isVirtualAccount(it contains map fields and is non-comparable).💥 Proposed fix
import ( "math/big" + "reflect" ) @@ - if first.Account != second.Account || first.Color != second.Color { + if !accountsEqual(first.Account, second.Account) || first.Color != second.Color { return } @@ } + +func accountsEqual(a, b AccountValue) bool { + if a == nil || b == nil { + return a == b + } + if reflect.TypeOf(a) != reflect.TypeOf(b) { + return false + } + return reflect.DeepEqual(a, b) +}Use this read-only check to confirm the panic condition exists:
#!/bin/bash set -euo pipefail # 1) Find interface equality usage in compaction logic rg -nP 'first\.Account\s*!=\s*second\.Account' -C3 internal/interpreter/funds_stack.go # 2) Confirm VirtualAccount carries non-comparable fields (maps) rg -nP 'type\s+VirtualAccount\s+struct' -A8 internal/interpreter/virtual_account.go rg -nP 'credits\s+map\[string\]\*fundsStack|debits\s+map\[string\]\*fundsStack' internal/interpreter/virtual_account.goExpected result: equality check is present in
funds_stack.go, andVirtualAccountmap fields are present invirtual_account.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/funds_stack.go` around lines 79 - 81, The current equality check uses direct interface comparison (first.Account != second.Account) which can panic for non-comparable dynamic types like VirtualAccount; replace that comparison with a safe deep-equality check (e.g., use reflect.DeepEqual(first.Account, second.Account)) and invert the condition accordingly so the early return still happens when accounts differ, ensuring you add the reflect import and keep the Color comparison (first.Color != second.Color) unchanged; update the compaction logic where first.Account and second.Account are compared in funds_stack.go to use reflect.DeepEqual to avoid runtime panics.
🧹 Nitpick comments (1)
internal/interpreter/args_parser_test.go (1)
51-51: Minor inconsistency: positional vs. named field initialization.Line 51 uses
Account{AccountAddress("user:001")}(positional), while line 39 usesAccount{Repr: AccountAddress("user:001")}(named). SinceAccounthas a single field this is safe, but using named fields consistently would be clearer.Suggested fix
- require.Equal(t, Account{AccountAddress("user:001")}, *a2) + require.Equal(t, Account{Repr: AccountAddress("user:001")}, *a2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/args_parser_test.go` at line 51, Replace the positional struct literal in the test expectation with a named-field literal for consistency: change the assertion comparing *a2 to use Account{Repr: AccountAddress("user:001")} instead of Account{AccountAddress("user:001")}, keeping the rest of the require.Equal call the same so it matches the style used earlier in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/interpreter/virtual_account.go`:
- Around line 65-67: The Receive logic currently pushes a sender into credits
even when remainingAmount == 0 causing zero-amount credits to accumulate; update
the Receive handling so that after setting sender.Amount = remainingAmount you
only call credits.Push(sender) when remainingAmount > 0 (or sender.Amount > 0)
so zero-amount entries are not added; ensure the same check is applied wherever
credits.Push is used in this repayment path so no no-op flows are produced.
---
Outside diff comments:
In `@internal/interpreter/interpreter_error.go`:
- Around line 115-123: The Error() message on NegativeBalanceError currently
hardcodes an '@' prefix when printing e.Account; update it to avoid assuming an
address-style account by either printing e.Account with "%s" (relying on
Account.String()) or by checking the underlying Account.Repr/type and formatting
accordingly; change the NegativeBalanceError.Error method to remove the '@%s'
literal and use the Account's String() representation or a small branch so
VirtualAccount messages are not misleading (refer to NegativeBalanceError.Error,
Account.String, and the Account/Repr variants).
---
Duplicate comments:
In `@internal/flags/flags.go`:
- Line 13: The flag name ExperimentalVirtualAccount is inconsistent with peers
that include the FeatureFlag suffix (e.g.,
ExperimentalOverdraftFunctionFeatureFlag); pick the convention to standardize
(recommend adding the suffix) and rename ExperimentalVirtualAccount to
ExperimentalVirtualAccountFeatureFlag in internal/flags/flags.go, then update
all usages/imports to the new symbol name (also consider aligning
ExperimentalAssetColors and ExperimentalMidScriptFunctionCall to the chosen
convention in a follow-up to keep names consistent across the codebase).
In `@internal/interpreter/funds_stack_test.go`:
- Line 126: Replace the positional struct literal for Sender with a keyed
(named) field literal wherever it appears (e.g., change occurrences like
{AccountAddress("src"), big.NewInt(10), "X"} to a Sender{FieldNameA:
AccountAddress("src"), FieldNameB: big.NewInt(10), FieldNameC: "X"} using the
actual field names from the Sender type); update all instances flagged (the
literal at the shown diff plus the ones noted later) so the file consistently
uses named fields instead of positional initialization.
In `@internal/interpreter/funds_stack.go`:
- Around line 79-81: The current equality check uses direct interface comparison
(first.Account != second.Account) which can panic for non-comparable dynamic
types like VirtualAccount; replace that comparison with a safe deep-equality
check (e.g., use reflect.DeepEqual(first.Account, second.Account)) and invert
the condition accordingly so the early return still happens when accounts
differ, ensuring you add the reflect import and keep the Color comparison
(first.Color != second.Color) unchanged; update the compaction logic where
first.Account and second.Account are compared in funds_stack.go to use
reflect.DeepEqual to avoid runtime panics.
In `@internal/interpreter/interpreter.go`:
- Around line 1007-1009: In the getBalance implementation inside the switch case
for VirtualAccount, the code uses s.CurrentAsset when indexing
accountRepr.credits (fs := accountRepr.credits[s.CurrentAsset]) which ignores
the function parameter asset; change that lookup to use the asset parameter (fs
:= accountRepr.credits[asset]) so the balance returned corresponds to the
requested asset, keeping the existing nil-check and subsequent logic intact.
- Around line 395-403: The VirtualAccount branch in the switch still calls
senderAccountAddress.Pull(...) even when the destination is KEPT_ADDR, which
mutates virtual-account state incorrectly; update the VirtualAccount case in
interpreter.go to check for KEPT_ADDR (same condition used in the AccountAddress
path) before calling senderAccountAddress.Pull, and skip the Pull when the
destination is kept so virtual account state is not changed for kept funds,
leaving the rest of the branch logic unchanged.
In `@internal/interpreter/value.go`:
- Around line 44-49: NewAccountAddress currently returns a non-zero Account
(Account{AccountAddress("")}) on validation failure which can leak an invalid
object; change the failure return to the zero-value Account{} while keeping the
same InterpreterError return (i.e., in NewAccountAddress return Account{},
InvalidAccountName{Name: src}) so callers can't silently receive a malformed
Account; update the return in NewAccountAddress and run tests to ensure behavior
remains consistent.
In `@internal/interpreter/virtual_account.go`:
- Around line 162-183: The loop that processes pulled senders never reduces
remainingAmt, causing debt to be recorded even when pulls satisfied the amount;
inside the for loop that iterates over pulled (the block that calls
send(pulledSender.Account,...)), subtract pulledSender.Amount from remainingAmt
(e.g., remainingAmt.Sub(remainingAmt, pulledSender.Amount)) and break early if
remainingAmt.Cmp(big.NewInt(0)) <= 0 so the subsequent overdraft block
(vacc.getDebits and Push of Sender) only runs when a positive remainingAmt truly
remains.
---
Nitpick comments:
In `@internal/interpreter/args_parser_test.go`:
- Line 51: Replace the positional struct literal in the test expectation with a
named-field literal for consistency: change the assertion comparing *a2 to use
Account{Repr: AccountAddress("user:001")} instead of
Account{AccountAddress("user:001")}, keeping the rest of the require.Equal call
the same so it matches the style used earlier in the test.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (18)
internal/interpreter/testdata/script-tests/experimental/virtual-account/account-create.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/account-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/bounded-overdraft-when-does-not-fail.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/creditors-stack.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/half-using-virtual.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-fail-if-not-enough.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-merchant-pays-fees-if-needed.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-no-commissions-with-low-amount.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-left-negative.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-when-fails.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/repay-self.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/send-credit-around.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/test-send-self-is-noop.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/transitive.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/virtual-account/wrong-currency.num.specs.jsonis excluded by!**/*.json
📒 Files selected for processing (33)
internal/analysis/check.gointernal/flags/flags.gointernal/interpreter/args_parser_test.gointernal/interpreter/batch_balances_query.gointernal/interpreter/evaluate_expr.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/funds_stack.gointernal/interpreter/funds_stack_test.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_error.gointernal/interpreter/interpreter_test.gointernal/interpreter/testdata/script-tests/experimental/virtual-account/account-create.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/account-kept.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/bounded-overdraft-when-does-not-fail.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/creditors-stack.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/half-using-virtual.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-fail-if-not-enough.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-merchant-pays-fees-if-needed.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-no-commissions-with-low-amount.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-left-negative.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-when-fails.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending-send-all.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/repay-self.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/send-credit-around.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/test-send-self-is-noop.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/transitive.numinternal/interpreter/testdata/script-tests/experimental/virtual-account/wrong-currency.numinternal/interpreter/value.gointernal/interpreter/virtual_account.gointernal/interpreter/virtual_account_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-left-negative.num
🚧 Files skipped from review as they are similar to previous changes (19)
- internal/interpreter/testdata/script-tests/experimental/virtual-account/account-create.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/test-send-self-is-noop.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-no-commissions-with-low-amount.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/prevent-double-spending-send-all.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-merchant-pays-fees-if-needed.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/transitive.num
- internal/interpreter/interpreter_test.go
- internal/interpreter/virtual_account_test.go
- internal/interpreter/batch_balances_query.go
- internal/interpreter/testdata/script-tests/experimental/virtual-account/half-using-virtual.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/account-kept.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/repay-self.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft-when-fails.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/overdraft.num
- internal/interpreter/evaluate_expr.go
- internal/analysis/check.go
- internal/interpreter/testdata/script-tests/experimental/virtual-account/wrong-currency.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/bounded-overdraft-when-does-not-fail.num
- internal/interpreter/testdata/script-tests/experimental/virtual-account/min-constraint-fail-if-not-enough.num
| sender.Amount = remainingAmount | ||
| credits.Push(sender) | ||
|
|
There was a problem hiding this comment.
Avoid storing zero-amount credits after debt repayment.
On Line [65]-[67], Receive pushes a sender even when remainingAmount == 0. This can accumulate no-op entries and later produce zero-amount flows.
🧹 Proposed fix
credits := vacc.getCredits(asset)
sender.Amount = remainingAmount
- credits.Push(sender)
+ if sender.Amount.Cmp(big.NewInt(0)) == 1 {
+ credits.Push(sender)
+ }
return postings🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/interpreter/virtual_account.go` around lines 65 - 67, The Receive
logic currently pushes a sender into credits even when remainingAmount == 0
causing zero-amount credits to accumulate; update the Receive handling so that
after setting sender.Amount = remainingAmount you only call credits.Push(sender)
when remainingAmount > 0 (or sender.Amount > 0) so zero-amount entries are not
added; ensure the same check is applied wherever credits.Push is used in this
repayment path so no no-op flows are produced.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
internal/interpreter/virtual_account.go (2)
65-66:⚠️ Potential issue | 🟡 MinorAvoid storing zero-amount credits.
When
remainingAmount == 0after debt repayment, pushing a zero-amount sender accumulates no-op entries that may produce zero-amount flows later.sender.Amount = remainingAmount - credits.Push(sender) + if sender.Amount.Cmp(big.NewInt(0)) == 1 { + credits.Push(sender) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/virtual_account.go` around lines 65 - 66, Avoid pushing zero-amount credits: after computing remainingAmount, only assign sender.Amount and call credits.Push(sender) when remainingAmount > 0; otherwise skip pushing (do not create a sender with Amount 0). This change ensures you check remainingAmount before mutating sender and invoking credits.Push (referencing sender, remainingAmount, and credits.Push) to prevent accumulating no-op zero-amount entries.
158-183:⚠️ Potential issue | 🔴 CriticalBug:
remainingAmtis never decremented, causing incorrect overdraft behavior.
remainingAmtis initialized toreceiver.Amountbut never decremented when postings are created. The condition at line 174 (remainingAmt.Cmp(big.NewInt(0)) == 1) will always be true if any amount was requested, even when sufficient credits were pulled.for _, pulledSender := range pulled { newPostings := send( pulledSender.Account, receiver.Account, pulledSender.Amount, asset, receiver.Color, ) postings = append(postings, newPostings...) + remainingAmt.Sub(remainingAmt, pulledSender.Amount) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/virtual_account.go` around lines 158 - 183, remainingAmt is never reduced when you create postings for each pulledSender so the overdraft branch always triggers; inside the loop that iterates pulled (where you call send(pulledSender.Account, receiver.Account, pulledSender.Amount, asset, receiver.Color)), subtract pulledSender.Amount from remainingAmt using big.Int arithmetic (e.g. remainingAmt.Sub(remainingAmt, pulledSender.Amount)) after creating the postings, and then change the overdraft check to test remainingAmt > 0 (or remainingAmt.Sign() > 0); this will ensure vacc.getDebits(...) is only used when there is an actual shortfall.internal/interpreter/interpreter.go (1)
1007-1008:⚠️ Potential issue | 🟠 MajorBug: Uses
s.CurrentAssetinstead of function parameterasset.The
getBalancefunction receives anassetparameter but theVirtualAccountbranch reads froms.CurrentAsset, which may be unset or different. This can return incorrect balances.case VirtualAccount: - fs := accountRepr.credits[s.CurrentAsset] + fs := accountRepr.credits[asset]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/interpreter.go` around lines 1007 - 1008, In getBalance, the VirtualAccount branch wrongly indexes credits using s.CurrentAsset instead of the function parameter asset; update the VirtualAccount case so it uses the provided asset parameter (e.g., accountRepr.credits[asset]) and preserve any nil/zero checks and return behavior consistent with the other branches (refer to getBalance, VirtualAccount, accountRepr, s.CurrentAsset, and asset to locate and change the indexing).internal/interpreter/funds_queue.go (1)
79-81:⚠️ Potential issue | 🟠 MajorInterface equality comparison can panic for non-comparable types.
Comparing interface values with
!=panics at runtime if the dynamic type contains non-comparable fields.VirtualAccountcontains maps (credits,debits), which are not comparable in Go.Use an explicit equality helper or type-switch to compare accounts safely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/funds_queue.go` around lines 79 - 81, The current comparison if first.Account != second.Account || first.Color != second.Color can panic because first.Account and second.Account are interface values whose dynamic type VirtualAccount contains non-comparable fields (maps); replace this direct interface equality with a safe equality check: implement and call a helper (e.g., accountsEqual(a, b) or a method on the concrete type) that type-asserts the interfaces to the expected concrete type(s) (VirtualAccount) and compares only comparable identifiers or fields (for example an AccountID or other scalar keys) or uses reflect.DeepEqual if appropriate; ensure you still short-circuit on Color inequality (compare first.Color != second.Color) but use the new accountsEqual(first.Account, second.Account) to avoid panics.
🧹 Nitpick comments (1)
internal/interpreter/funds_queue_test.go (1)
126-127: Inconsistent struct initialization style.Lines 126, 166-167, 179, and 187 use positional struct literals while the rest of the file uses explicit field names. This inconsistency reduces maintainability.
- {AccountAddress("src"), big.NewInt(10), "X"}, + {Account: AccountAddress("src"), Amount: big.NewInt(10), Color: "X"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interpreter/funds_queue_test.go` around lines 126 - 127, Replace the positional struct literals in internal/interpreter/funds_queue_test.go (e.g., the entries using AccountAddress("src"), big.NewInt(10), "X") with keyed (field-name) struct initializers to match the rest of the file; update all occurrences referenced in the comment (the literal at lines around 126 and the ones near 166-167, 179, 187) so they use explicit field names from the test struct definition (use the exact field names declared on that struct) instead of positional ordering to maintain consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/interpreter/interpreter.go`:
- Around line 259-260: Remove the unused virtualAccountsCredits and
virtualAccountsDebts fields: delete their declarations from the Interpreter
struct (the fields named virtualAccountsCredits and virtualAccountsDebts typed
as map[string]map[string]*fundsQueue) and remove their initialization (the
make(map[string]map[string]*fundsQueue) entries) in the interpreter
constructor/initializer where those maps are created; ensure no other references
to virtualAccountsCredits/virtualAccountsDebts remain and run a build to catch
any remaining references to fundsQueue or those fields.
- Around line 395-402: The VirtualAccount branch currently calls
senderAccountAddress.Pull(...) and discards the returned postings and also omits
the KEPT_ADDR handling present in the AccountAddress branch; update the
VirtualAccount case to mirror the AccountAddress logic: call
senderAccountAddress.Pull(st.CurrentAsset, Sender{AccountAddress(name),
sender.Amount, sender.Color}), capture the returned postings, and then apply the
same KEPT_ADDR check/handling used in AccountAddress (i.e., if name == KEPT_ADDR
treat postings as kept/handle appropriately, otherwise emit them or skip based
on the intended "virtual account" semantics). Also update the inline comment to
reflect whether postings should be emitted or suppressed so the intent is
explicit.
---
Duplicate comments:
In `@internal/interpreter/funds_queue.go`:
- Around line 79-81: The current comparison if first.Account != second.Account
|| first.Color != second.Color can panic because first.Account and
second.Account are interface values whose dynamic type VirtualAccount contains
non-comparable fields (maps); replace this direct interface equality with a safe
equality check: implement and call a helper (e.g., accountsEqual(a, b) or a
method on the concrete type) that type-asserts the interfaces to the expected
concrete type(s) (VirtualAccount) and compares only comparable identifiers or
fields (for example an AccountID or other scalar keys) or uses reflect.DeepEqual
if appropriate; ensure you still short-circuit on Color inequality (compare
first.Color != second.Color) but use the new accountsEqual(first.Account,
second.Account) to avoid panics.
In `@internal/interpreter/interpreter.go`:
- Around line 1007-1008: In getBalance, the VirtualAccount branch wrongly
indexes credits using s.CurrentAsset instead of the function parameter asset;
update the VirtualAccount case so it uses the provided asset parameter (e.g.,
accountRepr.credits[asset]) and preserve any nil/zero checks and return behavior
consistent with the other branches (refer to getBalance, VirtualAccount,
accountRepr, s.CurrentAsset, and asset to locate and change the indexing).
In `@internal/interpreter/virtual_account.go`:
- Around line 65-66: Avoid pushing zero-amount credits: after computing
remainingAmount, only assign sender.Amount and call credits.Push(sender) when
remainingAmount > 0; otherwise skip pushing (do not create a sender with Amount
0). This change ensures you check remainingAmount before mutating sender and
invoking credits.Push (referencing sender, remainingAmount, and credits.Push) to
prevent accumulating no-op zero-amount entries.
- Around line 158-183: remainingAmt is never reduced when you create postings
for each pulledSender so the overdraft branch always triggers; inside the loop
that iterates pulled (where you call send(pulledSender.Account,
receiver.Account, pulledSender.Amount, asset, receiver.Color)), subtract
pulledSender.Amount from remainingAmt using big.Int arithmetic (e.g.
remainingAmt.Sub(remainingAmt, pulledSender.Amount)) after creating the
postings, and then change the overdraft check to test remainingAmt > 0 (or
remainingAmt.Sign() > 0); this will ensure vacc.getDebits(...) is only used when
there is an actual shortfall.
---
Nitpick comments:
In `@internal/interpreter/funds_queue_test.go`:
- Around line 126-127: Replace the positional struct literals in
internal/interpreter/funds_queue_test.go (e.g., the entries using
AccountAddress("src"), big.NewInt(10), "X") with keyed (field-name) struct
initializers to match the rest of the file; update all occurrences referenced in
the comment (the literal at lines around 126 and the ones near 166-167, 179,
187) so they use explicit field names from the test struct definition (use the
exact field names declared on that struct) instead of positional ordering to
maintain consistency.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
internal/interpreter/funds_queue.gointernal/interpreter/funds_queue_test.gointernal/interpreter/interpreter.gointernal/interpreter/virtual_account.go
| virtualAccountsCredits: make(map[string]map[string]*fundsQueue), | ||
| virtualAccountsDebts: make(map[string]map[string]*fundsQueue), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of virtualAccountsCredits and virtualAccountsDebts
rg -n 'virtualAccountsCredits|virtualAccountsDebts' --type goRepository: formancehq/numscript
Length of output: 473
Remove unused virtualAccountsCredits and virtualAccountsDebts fields.
These maps are initialized at lines 259-260 but are never referenced anywhere in the codebase. They should be removed along with their struct field declarations at lines 327-328.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/interpreter/interpreter.go` around lines 259 - 260, Remove the
unused virtualAccountsCredits and virtualAccountsDebts fields: delete their
declarations from the Interpreter struct (the fields named
virtualAccountsCredits and virtualAccountsDebts typed as
map[string]map[string]*fundsQueue) and remove their initialization (the
make(map[string]map[string]*fundsQueue) entries) in the interpreter
constructor/initializer where those maps are created; ensure no other references
to virtualAccountsCredits/virtualAccountsDebts remain and run a build to catch
any remaining references to fundsQueue or those fields.
| case VirtualAccount: | ||
| // Here we have a debt from a virtual acc. | ||
| // we don't want to emit that as a posting (but TODO check how does it interact with kept) | ||
| senderAccountAddress.Pull(st.CurrentAsset, Sender{ | ||
| AccountAddress(name), | ||
| sender.Amount, | ||
| sender.Color, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and the specific lines mentioned
fd -t f interpreter.go | head -20Repository: formancehq/numscript
Length of output: 99
🏁 Script executed:
# Read the specific lines in question along with surrounding context
sed -n '380,420p' internal/interpreter/interpreter.goRepository: formancehq/numscript
Length of output: 1451
🏁 Script executed:
# Let's also check if there's a VirtualAccount struct/type definition
rg -n "type VirtualAccount" --type goRepository: formancehq/numscript
Length of output: 135
🏁 Script executed:
# Look for the Pull method definition
rg -n "func.*Pull\(" internal/interpreter/ --type go -A 5Repository: formancehq/numscript
Length of output: 1512
🏁 Script executed:
# Check the AccountAddress branch mentioned in the review for comparison
sed -n '350,450p' internal/interpreter/interpreter.goRepository: formancehq/numscript
Length of output: 2981
🏁 Script executed:
# Read the full Pull method implementation
sed -n '154,200p' internal/interpreter/virtual_account.goRepository: formancehq/numscript
Length of output: 959
🏁 Script executed:
# Check how Pull is used elsewhere in the codebase
rg -n "\.Pull\(" --type go -B 2 -A 3Repository: formancehq/numscript
Length of output: 8014
🏁 Script executed:
# Check the VirtualAccount Receive method for comparison
sed -n '140,160p' internal/interpreter/virtual_account.goRepository: formancehq/numscript
Length of output: 958
Virtual account debt handling discards postings and lacks KEPT_ADDR check.
When pulling from a VirtualAccount, the returned postings are discarded (line 398), which contradicts the comment stating funds shouldn't be emitted as postings. If postings should be ignored, the semantic intent should be clarified. Additionally, the VirtualAccount case lacks KEPT_ADDR handling present in the AccountAddress branch (compare lines 367-375 with 395-402), and the inline TODO acknowledges this gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/interpreter/interpreter.go` around lines 395 - 402, The
VirtualAccount branch currently calls senderAccountAddress.Pull(...) and
discards the returned postings and also omits the KEPT_ADDR handling present in
the AccountAddress branch; update the VirtualAccount case to mirror the
AccountAddress logic: call senderAccountAddress.Pull(st.CurrentAsset,
Sender{AccountAddress(name), sender.Amount, sender.Color}), capture the returned
postings, and then apply the same KEPT_ADDR check/handling used in
AccountAddress (i.e., if name == KEPT_ADDR treat postings as kept/handle
appropriately, otherwise emit them or skip based on the intended "virtual
account" semantics). Also update the inline comment to reflect whether postings
should be emitted or suppressed so the intent is explicit.
6a9808f to
0b28db4
Compare
updated queue terminology refactor: change interpreter naming (#117)
ascandone
force-pushed
the
feat/virtual-account
branch
from
0b28db4 to
74dd969
Compare
1 participant
No description provided.