feat: add ResolveDependencies for concrete volume/metadata discovery

[gfyrag]

Context

Ledger v3 uses a Raft architecture where transactions go through two stages:

1. Admission (leader): discovers script dependencies, preloads volumes, proposes a Raft order
2. FSM (all nodes): applies the order deterministically

The admission layer needs to know which volumes to preload before executing the script — but the volumes depend on the script itself.

The current approach uses GetInvolvedAccounts which returns expression trees (InvolvedAccountExpr) that the consumer must resolve. This led to ~500 lines of expression tree walking code on the ledger side, with bugs (GetAmount/GetAsset extracting wrong component, IsValidCall never called) and limitations (balance-dependent account addresses rejected).

Proposal

Add ResolveDependencies() which returns concrete data instead of expression trees:

type ResolvedDependencies struct {
    Volumes  map[string]map[string]*big.Int  // account → asset → balance
    Metadata map[string]map[string]string    // account → key → value
}

func (p ParseResult) ResolveDependencies(ctx, vars, store) (*ResolvedDependencies, error)

The consumer provides a Store callback (same interface as Run), gets back concrete reads, and handles the rest (hashing, preloading, drift detection) on its side.

How the ledger uses it

Admission: call ResolveDependencies → get volumes + metadata → hash inputs → preload into Raft order

FSM: call ResolveDependencies again → hash inputs → compare with admission hash → if match, execute; if mismatch, reject (retryable)

This is classic optimistic concurrency control.

What this replaces on the ledger side

• ~500 lines of resolveExprToString, resolveExprWithMetadata, collectFnMetaReads, ResolveDeferredAccounts
• No more coupling with InvolvedAccountExpr types
• No more IsValidCall / non-deterministic script rejection
• All features (meta chains, balance in addresses, mid-script calls) work transparently

Implementation

• recordingStore wraps a Store and records all GetBalances/GetAccountsMetadata calls
• ResolveDependencies runs the script with the recording store, discards the execution result, returns the recorded reads
• The internal implementation can be optimized later (partial execution, static analysis) without changing the API

Test plan

• Simple transfer — records source balance
• World source — no balance recorded
• meta() call — records metadata read
• Multiple sources — records all source balances
• Variables — records resolved account balances
• balance() function — records balance read
• Multiple sends — records all reads
• set_account_meta — no metadata reads produced
• Nested meta() chain — records all chained metadata reads
• Send all (*) — records source balance
• Empty reads — world-only script produces no reads
• Full existing test suite passes

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ea6a1937-9bc7-40f3-8bae-e65faca7ce0b

📥 Commits

Reviewing files that changed from the base of the PR and between 725a05e4bc3cc158af06dfeea38698eaa6676f27 and a1f074b.

📒 Files selected for processing (4)

• internal/interpreter/recording_store.go
• internal/interpreter/resolve_dependencies.go
• internal/interpreter/resolve_dependencies_test.go
• numscript.go

🚧 Files skipped from review as they are similar to previous changes (3)

• internal/interpreter/recording_store.go
• numscript.go
• internal/interpreter/resolve_dependencies_test.go

---

Walkthrough

Adds a ResolveDependencies dry-run function to the interpreter that wraps a store in a recordingStore to capture balance and metadata reads during pre-execution, then walks SendStatement AST nodes to conservatively collect potential write targets. Result types and a public method on ParseResult are exposed in numscript.go.

Changes

Dependency Resolution Dry-Run API

Layer / File(s)	Summary
Recording store wrapper internal/interpreter/recording_store.go	Defines recordingStore embedding an inner Store with two recording maps; GetBalances deep-copies big.Int values into balanceReads with deduplication via hasRow, and GetAccountsMetadata records key/value pairs into metadataReads before returning the original results.
ResolveDependencies types, main function, and write-collection helpers internal/interpreter/resolve_dependencies.go	Declares ResolvedDependencies, ResolvedReads, ResolvedWrites, and ResolveDependenciesOptions; ResolveDependencies validates feature flags, runs pre-resolution using a recording store, then walks SendStatements through collectSendWrites, collectSourceWrites, collectDestinationWrites, collectKeptOrDestWrites, and touchAccount to build a conservative write BalanceQuery.
Public API exposure numscript.go	Exports type aliases for result types and options, and adds ParseResult.ResolveDependencies method to forward the internal implementation.
Test suite internal/interpreter/resolve_dependencies_test.go	Adds resolveTest helper, readVolume and hasWrite utilities, and 13 test cases covering simple transfers, @world source, meta() destination, multiple sources, script variables, balance() function, multiple sends, set_account_meta, meta chains, send-all (*), empty reads, nested expressions with conservative over-approximation, and mid-script balance with an experimental feature pragma.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

• Azorlogh

Poem

🐰 Hop hop, the recorder is set,
Each balance and metadata met.
Writes are collected with care,
Conservative paths everywhere.
No posting runs, just a trace—
Dependencies found at a gentle pace! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.56% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'feat: add ResolveDependencies for concrete volume/metadata discovery' clearly summarizes the main change—adding a new ResolveDependencies function for discovering concrete volume and metadata data instead of expression trees.
Description check	✅ Passed	The description provides comprehensive context, problem statement, proposed solution with concrete examples, implementation details, and a detailed test plan—all directly related to the changeset adding ResolveDependencies functionality.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/resolve-dependencies

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 53.93939% with 76 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.42%. Comparing base (cbc11e8) to head (a1f074b).

Files with missing lines	Patch %	Lines
internal/interpreter/resolve_dependencies.go	47.61%	56 Missing and 10 partials ⚠️
internal/interpreter/recording_store.go	78.37%	4 Missing and 4 partials ⚠️
numscript.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
- Coverage   67.81%   67.42%   -0.39%     
==========================================
  Files          49       51       +2     
  Lines        5223     5388     +165     
==========================================
+ Hits         3542     3633      +91     
- Misses       1472     1533      +61     
- Partials      209      222      +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gfyrag]

Note on experimental-mid-script-function-call

ResolveDependencies performs Phases 1-2 of RunProgram (variable resolution + balance preloading) but does not execute statements. This means it captures all store reads for standard scripts.

The exception is experimental-mid-script-function-call, which allows calling balance() between send statements:

#![feature("experimental-mid-script-function-call")]

send [USD/2 100] (
  source = @world
  destination = @acc
)

// This balance() reads a value that depends on the first send's result.
// It cannot be discovered without actually executing the statements.
send balance(@acc, USD/2) (
  source = @acc
  destination = @dest
)

In this case, the mid-script balance() call produces a store read that depends on the execution of prior statements. ResolveDependencies cannot capture it because it doesn't execute statements.

To handle this, ResolveDependenciesOptions includes a ForbiddenFlags field. Consumers that need a complete dependency list can forbid this feature:

deps, err := parsed.ResolveDependencies(ctx, vars, store, numscript.ResolveDependenciesOptions{
    ForbiddenFlags: map[string]struct{}{
        "experimental-mid-script-function-call": {},
    },
})

Scripts that declare this feature will be rejected with a ForbiddenFeature error, making the incompleteness explicit rather than silent.

[gfyrag]

Don't need the amounts, just need to know the touched volumes.
You can keep it if you're comfident with it, but it add you more surface to maintain.

[ascandone]

If we don't need the amount it's better to remove them so that the impl is more "conservative".

However the point here is that this test shows that we still output a "Volumes" result even if the set of involved (account,asset) pairs depends in the meta and/or balances themselves. (In the case of the test, just on the meta). We had talked about this a bit (my understanding is that, if it happens, you'll detect that and just retry the thing) but I'm asking again to double check if we are ok with this

[gfyrag]

Yes, it should work. I've not noted the missing metadata key in deps.
But it should work.
Is the resolution determinist? (same calls in the same order?)

[gfyrag]

Hum, sorry, wrong response.
The GetAccountMetadata will be exercised in the admission path AND in the hot path.
If the dependency resolution does not report the metadata, it will not be loaded in the raft command, and so, not loaded in the in memory storage of the hot path.

[gfyrag]

So, a totally valid transaction will be rejected because the metadata will not be found in ram.

[gfyrag]

@ascandone updated after the rescope discussion — context for the new approach below.

Rescope

Two concrete needs:

1. Make all store calls deterministic so the host can hash them and detect input drift between successive runs.
2. Return the (account, asset) pairs touched by the script, including those it would write to.

The hashing itself will be done in ledger-v3-poc; on the numscript side we only need to (a) emit store calls in a deterministic order and (b) expose touched accounts.

What changed vs. the previous commit

• No more full execution. Previously ResolveDependencies was running phases 1-2 (vars + balance preload) and RunProgram was an alternative idea — both wasteful. It now performs phases 1-2 (records Reads via a recordingStore wrapper) then statically walks Source / Destination ASTs to collect Writes via evaluateExprAs. No posting is simulated.
• Reads / Writes are separated in the output. Writes are a conservative over-approximation: every account that appears as a source or destination is listed, even if a real run produces no posting for it. The TestResolveDependencies_Nested case now reports {source1, source2, source3, world, dest} instead of just {source1, dest} — which is what the host actually wants for preloading / locking.
• Determinism. Audited the call sites: parseVars iterates []VarDeclaration, the AST traversal iterates []Statement and []Source slices, and runBalancesQuery is called once with a single map. The sequence of GetBalances / GetAccountsMetadata calls is therefore deterministic given identical inputs.
• ForbiddenFlags is gone. It only existed to reject experimental-mid-script-function-call, which was an issue when we did not execute statements. The static walker resolves every account expression via evaluateExprAs, which transparently handles mid-script balance() / meta() in source/destination positions, so the carve-out is no longer needed.

Re-flagging on your earlier point — the test now exercises a case where s2 = meta(@account_that_needs_meta, "k") resolves to source2. Reads.Metadata includes account_that_needs_meta.k and Writes.Volumes includes source2 (statically discovered), so the host has everything it needs to load both into RAM before the hot path runs.

[NumaryBot]

🛑 Changes requested — automated review

The new API can miss real store reads and some actual balance writes for valid scripts, which breaks the dependency discovery and drift-detection guarantees it is meant to provide.

[NumaryBot]

🔴 [blocker] Include execution-time store reads

For scripts where store-backed functions are evaluated during statement execution rather than source preloading, such as set_tx_meta("k", meta(@cfg, "v")) or caps/allotments using balance(...), Run will call the store but this path only preloads source balances and then walks send statements without executing/evaluating those expressions. In those cases ResolveDependencies omits real metadata/balance reads from the admission hash, so FSM drift can go undetected.

[NumaryBot]

🟠 [major] Record scaling swap balance writes

When a source uses with scaling through @swap, runtime emits conversion postings from the source to the swap account in scaled asset(s) and from the swap account back to the source before the final send. This records only the source/current asset, so callers relying on Writes.Volumes will miss the swap account and scaled source assets that are actually impacted for asset-scaling scripts.

[coderabbitai]

Actionable comments posted: 5

🤖 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/recording_store.go`:
- Around line 30-36: The loop iterating over result and accessing
r.balanceReads[account][asset] assumes a nested map structure, but
r.balanceReads[account] is of type BalanceRow, not a map. Verify the actual
return type of Store.GetBalances and the actual structure of BalanceRow, then
update the loop that currently treats r.balanceReads[account] as a map to
instead properly populate the BalanceRow structure according to its real fields
and methods. If needed, convert the result into the appropriate data structure
before storing or adjust how individual balance values are assigned to match the
BalanceRow representation.

In `@internal/interpreter/resolve_dependencies.go`:
- Around line 161-163: In the case statement for *parser.SourceWithScaling, the
code incorrectly accesses source.Color which does not exist on this type.
Examine the actual fields defined on the SourceWithScaling struct to determine
the correct approach. Either access the appropriate fields available on
SourceWithScaling to call touchAccount with the correct arguments, or
recursively resolve the wrapped source node that SourceWithScaling contains (to
extract the actual Address and Color values) before calling touchAccount with
those extracted values.
- Around line 83-89: The CachedBalances field initialization in the programState
struct is using the wrong type. Change the CachedBalances initialization from
Balances{} to InternalBalances{} to match the expected field type of
programState.CachedBalances. Verify this matches the type used in the normal
interpreter state construction path to maintain consistency and avoid type
drift.
- Around line 259-267: The color variable returned from evaluateOptExprAs is a
value type, not a pointer, but the code incorrectly treats it as a pointer with
nil checks and dereference operators. Replace the pointer-based nil and
dereference checks with value-based emptiness checks (checking if color is an
empty string directly). Additionally, the undefined coloredAsset function call
should be replaced with the existing color-application helper function that is
available in the codebase. Update the type handling in this block to match the
actual return type of evaluateOptExprAs and use the correct, defined helper
function for applying color to assets.
- Around line 138-143: The evaluateSentAmt method returns an Asset value type
(not a pointer), but the code attempts to dereference it with the * operator
when assigning to st.CurrentAsset. Remove the pointer dereference operator from
the assignment statement where st.CurrentAsset = *asset should be changed to
st.CurrentAsset = asset, since asset is already a value type and does not need
to be dereferenced.

🪄 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: f0d9a3f3-16de-45e2-8879-9204cfb33abf

📥 Commits

Reviewing files that changed from the base of the PR and between 7856fd0 and 725a05e.

📒 Files selected for processing (4)

• internal/interpreter/recording_store.go
• internal/interpreter/resolve_dependencies.go
• internal/interpreter/resolve_dependencies_test.go
• numscript.go

[NumaryBot]

🛑 Changes requested — automated review

ResolveDependencies can omit concrete store reads that are evaluated by Run and can under-report writes for asset scaling. These gaps break the intended dependency discovery and drift/conflict detection semantics.

[NumaryBot]

🔴 [blocker] Include reads from function and cap expressions

For scripts where a store-reading expression appears outside variable declarations, sent values, or account address expressions — for example set_tx_meta("x", meta(@cfg, "k")) or a source/destination cap using balance(@limits, USD/2) — Run will evaluate those expressions and read the store, but this resolver only performs the balance preload pass here and never evaluates those statement/cap/allotment expressions. The returned Reads can therefore omit inputs that affect execution, so the admission/FSM drift hash can miss metadata or balance changes.

[NumaryBot]

🟠 [major] Record scaling writes for swap and scaled assets

When the script uses source = @src with scaling through @swap, the interpreter can emit postings from src in the consumed scaled asset rows and from swap in the target asset before the final send. This branch records only src with CurrentAsset, so Writes.Volumes misses balances such as src/EUR and swap/EUR/2; callers using writes for preloading or conflict detection will not protect balances the script can update.