refactor(interpreter): Simplify fundsQueue & improve naming #100
Conversation
Azorlogh
commented
Oct 13, 2025
Azorlogh
commented
- renamed fundsStack to fundsQueue to mirror how it's actually used
- change fundsQueue to use a slice
- renamed send-related interpreter functions to mirror the naming logic: "take from a source" and "send to a destination"
- moved CurrentAsset into the fundsQueue
WalkthroughReplaces a stack-based funds implementation with a queue-based one (introduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Program
participant Interpreter
participant fundsQueue
participant BalanceQuery
Program->>Interpreter: Execute script
Interpreter->>fundsQueue: Push(Sender{name, amount, color})
note right of fundsQueue #DDEBF7: merge adjacent same name/color\nignore zero amounts
Interpreter->>fundsQueue: Pull(maxAmount, color?)
alt color provided
fundsQueue-->>Interpreter: Selected senders (filtered, split if needed)
else no color
fundsQueue-->>Interpreter: Selected senders (FIFO, split if needed)
end
Interpreter->>BalanceQuery: Queue balance checks using fundsQueue.asset
BalanceQuery-->>Interpreter: Balances
Interpreter-->>Program: Return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
- Coverage 70.49% 70.07% -0.42%
==========================================
Files 43 43
Lines 4982 4956 -26
==========================================
- Hits 3512 3473 -39
- Misses 1313 1326 +13
Partials 157 157 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
internal/interpreter/batch_balances_query.go(3 hunks)internal/interpreter/funds_queue.go(1 hunks)internal/interpreter/funds_queue_test.go(3 hunks)internal/interpreter/funds_stack.go(0 hunks)internal/interpreter/interpreter.go(19 hunks)
💤 Files with no reviewable changes (1)
- internal/interpreter/funds_stack.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/interpreter/funds_queue_test.go (1)
internal/interpreter/funds_queue.go (1)
Sender(9-13)
internal/interpreter/interpreter.go (5)
internal/interpreter/funds_queue.go (1)
Sender(9-13)internal/parser/ast.go (20)
Source(122-125)Destination(189-192)SentValueLiteral(278-281)SentValueLiteral(287-287)ValueExpr(8-11)SourceAccount(130-130)SourceAccount(135-138)SourceOverdraft(132-132)SourceOverdraft(167-172)SourceInorder(127-127)SourceInorder(140-143)SourceCapped(131-131)SourceCapped(161-165)DestinationAccount(194-194)DestinationAccount(200-202)KeptOrDestination(234-236)DestinationKept(238-238)DestinationKept(242-244)DestinationTo(239-239)DestinationTo(246-248)internal/interpreter/value.go (4)
Asset(18-18)Asset(32-32)Monetary(22-25)Monetary(30-30)internal/interpreter/interpreter_error.go (2)
NegativeAmountErr(125-128)MissingFundsErr(10-15)internal/utils/utils.go (1)
NonNeg(36-38)
| func (s fundsQueue) Clone() fundsQueue { | ||
| return fundsQueue{ | ||
| senders: slices.Clone(s.senders), | ||
| asset: s.asset, | ||
| } |
There was a problem hiding this comment.
Deep-clone sender amounts in Clone()
Clone() only copies the slice, so every cloned Sender.Amount still points to the same *big.Int. When cloneState() (interpreter.go Line 604) restores fundsQueue, subsequent mutations done after the clone leak back into the “restored” queue, breaking backtracking (e.g. oneof sources will see already-depleted amounts). Please deep-copy each amount.
func (s fundsQueue) Clone() fundsQueue {
- return fundsQueue{
- senders: slices.Clone(s.senders),
- asset: s.asset,
- }
+ cloned := make([]Sender, len(s.senders))
+ for i, sender := range s.senders {
+ cloned[i] = Sender{
+ Name: sender.Name,
+ Color: sender.Color,
+ Amount: new(big.Int).Set(sender.Amount),
+ }
+ }
+ return fundsQueue{
+ senders: cloned,
+ asset: s.asset,
+ }
}📝 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 (s fundsQueue) Clone() fundsQueue { | |
| return fundsQueue{ | |
| senders: slices.Clone(s.senders), | |
| asset: s.asset, | |
| } | |
| func (s fundsQueue) Clone() fundsQueue { | |
| cloned := make([]Sender, len(s.senders)) | |
| for i, sender := range s.senders { | |
| cloned[i] = Sender{ | |
| Name: sender.Name, | |
| Color: sender.Color, | |
| Amount: new(big.Int).Set(sender.Amount), | |
| } | |
| } | |
| return fundsQueue{ | |
| senders: cloned, | |
| asset: s.asset, | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/interpreter/funds_queue.go around lines 114 to 118, Clone()
currently shallow-copies the senders slice so each Sender.Amount still points to
the same *big.Int; change Clone() to deep-copy each Sender and its Amount by
creating a new slice of senders, copying each Sender struct and, for each
non-nil Amount, allocating a new big.Int and calling Set(oldAmount) (or using
new(big.Int).Set) so the cloned fundsQueue has independent amount values and
subsequent mutations do not affect the original.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/interpreter/funds_queue.go (1)
122-126: Deep-clone sender amounts inClone()This issue was already flagged in previous reviews:
Clone()shallow-copies the senders slice, so each clonedSender.Amountstill points to the same*big.Int. When the interpreter backtracks (e.g.,oneofbranches), mutations to Amount values will leak between the cloned and original queues.Apply the suggested fix from the previous review to deep-copy each Sender and its Amount.
🧹 Nitpick comments (1)
internal/interpreter/funds_queue.go (1)
77-77: Consider addressing the TODO for performance.The TODO suggests preallocating the output queue for performance. This could be beneficial in hot paths where Pull is called frequently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/interpreter/funds_queue.go(1 hunks)
🔇 Additional comments (3)
internal/interpreter/funds_queue.go (3)
37-51: LGTM! Merge logic is correct.The merge logic correctly leverages pointer semantics: when
last.Amount.Add(last.Amount, sender.Amount)is called, it mutates the*big.Intthat both the locallastvariable and the slice element point to, making the change visible in the queue. The zero-amount early return is also a good optimization.
72-119: Pull logic is sound with proper offset handling.The offset-based iteration correctly handles:
- Skipping non-matching colors by incrementing offset
- Removing matched elements without adjusting offset (elements shift left naturally)
- Cloning maxAmount for safe mutation
- Splitting senders when partially satisfying the pull
The logic has been traced through multiple scenarios and appears correct.
20-27: Asset field initialization is handled by callers in interpreter.go (lines 424, 436) and batch_balances_query.go (42); no changes needed.
1 participant
