feat: get involved accounts#131
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughRenames SaveStatement.Amount → SaveStatement.Account across parser, analysis, interpreter, and batch-prefetch. Adds GetInvolvedAccounts analysis (expression model, validators, and traversal) with comprehensive tests, exposes involved-account types via an accounts package, and adds InvalidOperatorErr. ChangesSave Statement Fix & Involved Accounts Analysis
sequenceDiagram
participant Client as Caller
participant Parse as GetInvolvedAccounts
participant Vars as parseVars
participant Eval as evalExpr
participant Stmts as evalSaveStmt/evalSendStmt
Client->>Parse: GetInvolvedAccounts(vars, program)
Parse->>Vars: parse/evaluate declared variables
Vars->>Eval: fold/convert var origins to InvolvedAccountExpr
Parse->>Stmts: iterate statements
Stmts->>Eval: evalSaveStmt / evalSendStmt (account, asset)
Eval-->>Parse: emit InvolvedAccount[], InvolvedMeta[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 #131 +/- ##
==========================================
- Coverage 68.52% 67.00% -1.52%
==========================================
Files 46 47 +1
Lines 4651 5068 +417
==========================================
+ Hits 3187 3396 +209
- Misses 1290 1476 +186
- Partials 174 196 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ascandone
force-pushed
the
feat/get-involved-accounts
branch
from
49b84b6 to
c069a0b
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/interpreter/get_involved_accounts.go`:
- Around line 582-602: The code in evalDest (cases for parser.DestinationInorder
and parser.DestinationOneof) only visits clause targets (clause.To / acc.To) and
therefore never collects accounts reachable via the clause.Remaining
destination; update those cases so after calling st.evalKeptOrDest(clause.To)
(or acc.To) you also check for and traverse clause.Remaining (or acc.Remaining)
by calling st.evalKeptOrDest on it (or iterating if Remaining can contain
multiple destinations) so Remaining destinations are visited the same way as To;
use the existing evalKeptOrDest helper and the symbols
parser.DestinationInorder, parser.DestinationOneof, clause/acc and
st.evalKeptOrDest to locate where to add this traversal.
- Around line 162-174: The case handling analysis.FnSetAccountMeta directly
indexes stmt.Args[0] and [1] and can panic on malformed input; add an arity
guard before calling st.evalExpr (e.g., if len(stmt.Args) < 2) and return a
typed interpreter error (or fmt.Errorf with clear message) if insufficient args,
otherwise proceed to eval both args and append the InvolvedMeta entry; update
the switch branch around FnSetAccountMeta and use the same error type/format the
interpreter uses elsewhere for consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3887a13f-f3b8-4b28-aa53-6abde49fdbab
⛔ Files ignored due to path filters (2)
internal/parser/__snapshots__/parser_fault_tolerance_test.snapis excluded by!**/*.snap,!**/*.snapinternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (9)
internal/analysis/check.gointernal/analysis/hover.gointernal/interpreter/batch_balances_query.gointernal/interpreter/get_involved_accounts.gointernal/interpreter/get_involved_accounts_test.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_error.gointernal/parser/ast.gointernal/parser/parser.go
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
Review from the ledger side
I analyzed this PR against the ledger integration (replacing the discoveryStore emulation + full meta() support). The static analysis approach is fundamentally cleaner and more performant than execution with infinite balances. Here's what needs adjusting to go to production.
Bug: lost : separator in interpolated accounts
get_involved_accounts.go:346-360 — For @user:$id:pending, the real interpreter does strings.Join(parts, ":") (evaluate_expr.go:38), but the analysis uses foldedAdd which produces Add{Add{AccountLiteral{user}, NumberLiteral{42}}, AccountLiteral{pending}}. When resolving this tree, you get "user42pending" instead of "user:42:pending".
Fix: replace Add with a dedicated AccountSegments type for account interpolations:
type AccountSegments struct {
Parts []InvolvedAccountExpr
}And in evalExpr for AccountInterpLiteral:
case *parser.AccountInterpLiteral:
parts := make([]InvolvedAccountExpr, 0, len(expr.Parts))
for _, part := range expr.Parts {
partExpr, err := st.evalAccountNamePart(part)
if err != nil { return nil, err }
parts = append(parts, partExpr)
}
if len(parts) == 1 {
return parts[0], nil
}
return AccountSegments{Parts: parts}, nilAdd stays for arithmetic only.
Missing: color constraints and asset scaling
batch_balances_query.go transforms the asset before querying:
SourceAccountwith color →coloredAsset(asset, color)(line 98)SourceWithScaling→assetToScaledAsset(asset)for the main address (line 113), and no query for the through account (line 106)
The static analysis completely ignores Color and doesn't distinguish SourceWithScaling cases. The consumer would preload the wrong asset.
Fix: enrich InvolvedAccount:
type InvolvedAccount struct {
AccountExpr InvolvedAccountExpr
AssetExpr InvolvedAccountExpr
Color InvolvedAccountExpr // nil if no color constraint
Scaled bool // true → consumer applies assetToScaledAsset()
NeedsBalance bool // false for unbounded overdraft, destinations, through accounts
}And adjust evalSrc to propagate source.Color, source.Bounded == nil (unbounded overdraft), and the SourceWithScaling case (address = scaled+needs balance, through = not scaled+no balance).
Missing: separate meta reads from meta writes
involvedMeta mixes meta() in var declarations (reads) and set_account_meta (writes). On the consumer side, reads block resolution (you need to read the metadata to know which account to preload), writes are just preloading for previous value capture.
Fix: either add an IsWrite bool field on InvolvedMeta, or separate into two slices in the return:
func GetInvolvedAccounts(vars VariablesMap, program parser.Program) (
[]InvolvedAccount,
[]InvolvedMeta, // reads: meta() in var declarations
[]InvolvedMeta, // writes: set_account_meta
InterpreterError,
)Minor points
set_tx_meta(line 167): the// can we ignore this ?is correct — transaction metadata doesn't affect preloading. Replace the comment with// set_tx_meta does not affect involved accounts or preloadingto be explicit.- Public API:
GetInvolvedAccountsis ininternal/interpreter/. The consumer imports the root package — it will need to be exposed innumscript.goalong with the expression types. - Coverage at 47%:
GetBalance,GetOverdraft,Sub,Div,SubPrefix, mostevalSrc/evalDestbranches are uncovered. Add tests forSourceCapped,SourceInorder,SourceOneof,DestinationInorder,DestinationOneof,savestatement.
ascandone
force-pushed
the
feat/get-involved-accounts
branch
from
c069a0b to
fef44f2
Compare
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
Second pass — deux points bloquants pour le ledger v3
L'approche est bonne et le gros du travail est fait, mais il reste deux problèmes d'API qu'il vaut mieux régler maintenant — une fois intégrés côté ledger, les changer sera beaucoup plus coûteux. Le ledger v3 ne sera pas lancé sans ces deux points.
1. Add réutilisé pour la concaténation de segments de comptes
Pour @user:$id:pending, le code produit :
Add(Add(Add(Add("user", ":"), 42), ":"), "pending")
Les séparateurs : sont bien préservés, donc les données sont correctes. Mais Add est utilisé à la fois pour l'arithmétique (100 + 42) et la concaténation de segments de comptes. Côté consommateur, il n'y a aucun moyen de distinguer les deux sans heuristiques fragiles.
Fix : un type dédié AccountSegments pour les interpolations de comptes :
type AccountSegments struct {
Parts []InvolvedAccountExpr
}Et dans evalExpr pour AccountInterpLiteral :
case *parser.AccountInterpLiteral:
parts := make([]InvolvedAccountExpr, 0, len(expr.Parts))
for _, part := range expr.Parts {
partExpr, err := st.evalAccountNamePart(part)
if err != nil { return nil, err }
parts = append(parts, partExpr)
}
if len(parts) == 1 {
return parts[0], nil
}
return AccountSegments{Parts: parts}, nilLe consommateur peut alors faire strings.Join(resolvedParts, ":") naturellement. Add reste réservé à l'arithmétique.
2. Meta reads et writes mélangés dans le même slice
GetInvolvedAccounts retourne un seul []InvolvedMeta qui mélange :
- Les lectures (
meta(@acc, "k")dans lesvars) — bloquantes, il faut lire la metadata pour résoudre les comptes impliqués - Les écritures (
set_account_meta) — juste du preloading pour capturer la valeur précédente
Le consommateur doit pouvoir les distinguer pour ordonner correctement ses opérations (les reads bloquent la résolution, les writes non).
Fix : soit un champ IsWrite bool sur InvolvedMeta, soit deux slices séparés dans le retour :
func GetInvolvedAccounts(vars VariablesMap, program parser.Program) (
[]InvolvedAccount,
[]InvolvedMeta, // reads: meta() dans les var declarations
[]InvolvedMeta, // writes: set_account_meta
InterpreterError,
)Je préfère la deuxième option (deux slices), c'est plus explicite et ça évite que le consommateur ait à filtrer.
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
Second pass — two blocking points for ledger v3
The approach is solid and the bulk of the work is done, but two API issues need to be fixed now — once integrated on the ledger side, changing them will be much more costly. Ledger v3 won't ship without these.
1. Add reused for account segment concatenation
For @user:$id:pending, the code produces:
Add(Add(Add(Add("user", ":"), 42), ":"), "pending")
The : separators are preserved, so the data is correct. But Add is used for both arithmetic (100 + 42) and account segment concatenation. On the consumer side, there's no way to distinguish the two without fragile heuristics.
Fix: a dedicated AccountSegments type for account interpolations:
type AccountSegments struct {
Parts []InvolvedAccountExpr
}And in evalExpr for AccountInterpLiteral:
case *parser.AccountInterpLiteral:
parts := make([]InvolvedAccountExpr, 0, len(expr.Parts))
for _, part := range expr.Parts {
partExpr, err := st.evalAccountNamePart(part)
if err != nil { return nil, err }
parts = append(parts, partExpr)
}
if len(parts) == 1 {
return parts[0], nil
}
return AccountSegments{Parts: parts}, nilThe consumer can then naturally do strings.Join(resolvedParts, ":"). Add stays reserved for arithmetic.
2. Meta reads and writes mixed in the same slice
GetInvolvedAccounts returns a single []InvolvedMeta that mixes:
- Reads (
meta(@acc, "k")in var declarations) — blocking, metadata must be fetched to resolve the involved accounts - Writes (
set_account_meta) — just preloading to capture the previous value
The consumer needs to distinguish them to order operations correctly (reads block resolution, writes don't).
Fix: either an IsWrite bool field on InvolvedMeta, or two separate slices in the return:
func GetInvolvedAccounts(vars VariablesMap, program parser.Program) (
[]InvolvedAccount,
[]InvolvedMeta, // reads: meta() in var declarations
[]InvolvedMeta, // writes: set_account_meta
InterpreterError,
)I prefer the second option (two slices) — it's more explicit and the consumer doesn't have to filter.
|
Regarding point 2 — // GetInvolvedAccounts, case FnSetAccountMeta:
st.involvedMeta = append(st.involvedMeta, InvolvedMeta{
Account: acc,
Key: key,
})And meta reads from var declarations go to the same slice: // evalVar, case FnVarOriginMeta:
st.involvedMeta = append(st.involvedMeta, InvolvedMeta{
Account: acc,
Key: key,
})Both end up in the single |
ascandone
force-pushed
the
feat/get-involved-accounts
branch
from
a37898e to
688c0e8
Compare
ascandone
force-pushed
the
feat/get-involved-accounts
branch
from
688c0e8 to
1832b94
Compare
|
Two things on the latest changes: 1. My mistake on meta writes — we do need themI was wrong to suggest dropping The original approach (reporting writes in 2. Bug:
|
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
Both blocking points are addressed:
ConcatAccountcleanly separates account segment concatenation from arithmeticAdd, with constant folding as a bonus.- Meta reads vs writes distinguished via nullable
Writefield — simple and effective.
LGTM, good to merge.
2 participants
No description provided.