feat: resolve dependencies#169
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR moves interpreter expression evaluation to a standalone ChangesevalEnv refactor and dependency resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ResolveDependencies
participant recordingStore
participant evalEnv
participant Store
Caller->>ResolveDependencies: program, vars, store
ResolveDependencies->>recordingStore: wrap Store
ResolveDependencies->>evalEnv: initialize evaluation context
ResolveDependencies->>recordingStore: evaluate statements
recordingStore->>Store: query balances or account metadata
Store-->>recordingStore: return queried values
ResolveDependencies-->>Caller: return ResolvedDependencies
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #169 +/- ##
==========================================
- Coverage 70.71% 70.66% -0.05%
==========================================
Files 57 58 +1
Lines 5262 5482 +220
==========================================
+ Hits 3721 3874 +153
- Misses 1328 1370 +42
- Partials 213 238 +25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ascandone
force-pushed
the
feat/resolve-dependencies-simplified
branch
3 times, most recently
from
63d72f4 to
860fa76
Compare
✅ Approve — automated reviewAll previously identified issues have been addressed or dismissed. The major finding about unrecognized top-level function calls silently succeeding in ResolveDependencies was raised in multiple prior threads and has been marked as resolved. The suggestion to validate callees before evaluating arguments in function_exprs.go was explicitly dismissed by a project maintainer. The single remaining reviewer found no new actionable correctness issues. There are no outstanding blockers or major findings. No findings. |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #169 (comment)
ascandone
force-pushed
the
feat/resolve-dependencies-simplified
branch
from
860fa76 to
b928a5a
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #169 (comment)
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/interpreter/interpreter.go (1)
320-331: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn
UnboundFunctionErrbefore evaluating top-level call args.For unsupported statement calls, arguments are evaluated before the callee is rejected. That can mask the unbound-function error and trigger store reads for a statement that will never run.
🐛 Proposed dispatch-first structure
case *parser.FnCall: - args, err := evaluateExpressions(&st.evalEnv, statement.Args) - if err != nil { - return err - } - switch statement.Caller.Name { case analysis.FnSetTxMeta: + args, err := evaluateExpressions(&st.evalEnv, statement.Args) + if err != nil { + return err + } return setTxMeta(st, statement.Caller.Range, args) case analysis.FnSetAccountMeta: + args, err := evaluateExpressions(&st.evalEnv, statement.Args) + if err != nil { + return err + } return setAccountMeta(st, statement.Caller.Range, args) default: return UnboundFunctionErr{Name: statement.Caller.Name} }🤖 Prompt for 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. In `@internal/interpreter/interpreter.go` around lines 320 - 331, The dispatch in the statement handler is evaluating arguments before confirming the callee is supported, which can hide the unbound-function error and cause unnecessary store reads. In the statement execution path around the switch on statement.Caller.Name, check for supported functions first and return UnboundFunctionErr immediately for anything else, then only call evaluateExpressions for the recognized cases before invoking setTxMeta or setAccountMeta.internal/interpreter/batch_balances_query.go (1)
93-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMirror the asset-color feature guard during preloading.
findBalancesQueriesevaluates color expressions and batches colored balance reads before runtime checksExperimentalAssetColors. If the flag is disabled, preloading can return a color-expression error or hit storage before the feature error.🐛 Proposed guard
case *parser.SourceAccount: account, err := evaluateExprAs(&st.evalEnv, source.ValueExpr, expectAccount) if err != nil { return err } + if source.Color != nil { + if err := st.checkFeatureFlag(flags.ExperimentalAssetColors); err != nil { + return err + } + } color, err := evaluateOptExprAs(&st.evalEnv, source.Color, expectString) if err != nil { return err } st.batchQuery(account, st.CurrentAsset, color) return nil @@ account, err := evaluateExprAs(&st.evalEnv, source.Address, expectAccount) if err != nil { return err } + if source.Color != nil { + if err := st.checkFeatureFlag(flags.ExperimentalAssetColors); err != nil { + return err + } + } color, err := evaluateOptExprAs(&st.evalEnv, source.Color, expectString)Also applies to: 122-131
🤖 Prompt for 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. In `@internal/interpreter/batch_balances_query.go` around lines 93 - 103, Mirror the ExperimentalAssetColors guard in findBalancesQueries before evaluating source.Color or calling st.batchQuery, so preload behavior matches runtime checks. Update the findBalancesQueries flow (including the batch path at both affected sites) to skip colored balance preloading or return the same feature-disabled error when the flag is off, rather than evaluating the color expression or touching storage first.
🧹 Nitpick comments (2)
internal/interpreter/resolve_dependencies_test.go (1)
43-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
savecase toTestResolveDependenciesCoversRuntime.The soundness test validates that resolved writes cover runtime postings, but has no
savetest case. This gap means theSaveStatementhandler's read-only treatment of the source account (lines 136-146 inresolve_dependencies.go) is never validated against actual runtime postings. Adding asavecase would confirm whether the current read-only classification is correct or if a write is missing.🤖 Prompt for 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. In `@internal/interpreter/resolve_dependencies_test.go` around lines 43 - 177, Add a new `save` scenario to `TestResolveDependenciesCoversRuntime` so the soundness check exercises `SaveStatement` alongside the existing `send` cases. Use `parser.Parse`, `interpreter.RunProgram`, and `interpreter.ResolveDependencies` in the same pattern as the other table-driven cases, with a source account and destination/recipient setup that produces runtime postings from `save`. Then verify the resolved `AccountsReads` and `AccountsWrites` against the `result.Postings` the same way the test already does, so the `SaveStatement` handling in `resolve_dependencies.go` is covered.internal/interpreter/evaluate_expr.go (1)
63-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse the metadata cache instead of replacing it per lookup.
getMetadataalways callsStoreand overwritesCachedAccountsMeta, so repeated or multi-keymeta()calls evict prior cached values and can re-hit storage unnecessarily. MirrorgetBalanceby checking cached values first and merging query results.♻️ Proposed cache merge
func (env *evalEnv) getMetadata(account AccountAddress, key string) (string, bool, InterpreterError) { + if value, ok := env.CachedAccountsMeta.Get(account.Name, account.Scope, key); ok { + return value, true, nil + } + rows, err := env.Store.GetAccountsMetadata(env.ctx, MetadataQuery{ {Account: account.Name, Scope: account.Scope, Keys: []string{key}}, }) if err != nil { return "", false, QueryMetadataError{WrappedError: err} } - env.CachedAccountsMeta = FromAccountsMetadataRows(rows) + for k, value := range FromAccountsMetadataRows(rows) { + env.CachedAccountsMeta[k] = value + } value, ok := env.CachedAccountsMeta.Get(account.Name, account.Scope, key) return value, ok, nil }🤖 Prompt for 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. In `@internal/interpreter/evaluate_expr.go` around lines 63 - 72, getMetadata currently overwrites CachedAccountsMeta on every lookup, so repeated meta() calls can evict earlier entries and always re-query Store. Update evalEnv.getMetadata to first check env.CachedAccountsMeta for the requested account/key, and only call Store.GetAccountsMetadata when the value is missing. When new rows are fetched, merge them into the existing cache instead of replacing it, mirroring the getBalance caching pattern.
🤖 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/function_exprs.go`:
- Around line 18-27: The callee in function expression handling is being
validated too late, which allows argument evaluation to run before rejecting
unknown functions or invalid nested meta calls. In function_exprs.go, update the
function expression path around evaluateExpressions and the fnCall.Caller.Name
switch so the callee/default branch is checked first, returning
UnboundFunctionErr or InvalidNestedMeta before any argument evaluation or store
reads. Keep the guard logic near the existing fnCall.Caller.Name handling so
FunctionExpr evaluation short-circuits correctly.
In `@internal/interpreter/resolve_dependencies.go`:
- Around line 185-186: The default branch in the dependency resolver is
swallowing unknown top-level function calls by returning nil, which lets invalid
scripts pass; update the logic in resolve_dependencies.go so the function-call
handling for set_account_meta and set_tx_meta explicitly returns an error for
any other call, matching the interpreter’s UnboundFunctionErr behavior. Make the
change in the switch’s default path so dependency resolution fails consistently
when an unrecognized top-level function is encountered.
---
Outside diff comments:
In `@internal/interpreter/batch_balances_query.go`:
- Around line 93-103: Mirror the ExperimentalAssetColors guard in
findBalancesQueries before evaluating source.Color or calling st.batchQuery, so
preload behavior matches runtime checks. Update the findBalancesQueries flow
(including the batch path at both affected sites) to skip colored balance
preloading or return the same feature-disabled error when the flag is off,
rather than evaluating the color expression or touching storage first.
In `@internal/interpreter/interpreter.go`:
- Around line 320-331: The dispatch in the statement handler is evaluating
arguments before confirming the callee is supported, which can hide the
unbound-function error and cause unnecessary store reads. In the statement
execution path around the switch on statement.Caller.Name, check for supported
functions first and return UnboundFunctionErr immediately for anything else,
then only call evaluateExpressions for the recognized cases before invoking
setTxMeta or setAccountMeta.
---
Nitpick comments:
In `@internal/interpreter/evaluate_expr.go`:
- Around line 63-72: getMetadata currently overwrites CachedAccountsMeta on
every lookup, so repeated meta() calls can evict earlier entries and always
re-query Store. Update evalEnv.getMetadata to first check env.CachedAccountsMeta
for the requested account/key, and only call Store.GetAccountsMetadata when the
value is missing. When new rows are fetched, merge them into the existing cache
instead of replacing it, mirroring the getBalance caching pattern.
In `@internal/interpreter/resolve_dependencies_test.go`:
- Around line 43-177: Add a new `save` scenario to
`TestResolveDependenciesCoversRuntime` so the soundness check exercises
`SaveStatement` alongside the existing `send` cases. Use `parser.Parse`,
`interpreter.RunProgram`, and `interpreter.ResolveDependencies` in the same
pattern as the other table-driven cases, with a source account and
destination/recipient setup that produces runtime postings from `save`. Then
verify the resolved `AccountsReads` and `AccountsWrites` against the
`result.Postings` the same way the test already does, so the `SaveStatement`
handling in `resolve_dependencies.go` is covered.
🪄 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: 750cbe94-e3e4-476d-a7ac-5299ed11cc7b
📒 Files selected for processing (9)
internal/interpreter/batch_balances_query.gointernal/interpreter/evaluate_expr.gointernal/interpreter/function_exprs.gointernal/interpreter/infix_overloads.gointernal/interpreter/interpreter.gointernal/interpreter/resolve_dependencies.gointernal/interpreter/resolve_dependencies_test.gonumscript.gonumscript_test.go
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (1 fixed, 0 outdated).
Summary: #169 (comment)
|
This seems to miss a metadata read func TestResolveMultipleMetaKeysAreAllReads(t *testing.T) {
deps := resolve(t, `
vars {
account $x = meta(@config, "recipient")
account $y = meta(@config, "fallback")
}
send [USD 10] (
source = $x allowing unbounded overdraft
destination = $y
)
`, nil, interpreter.StaticStore{
Meta: interpreter.AccountsMetadata{
{Account: "config", Key: "recipient", Value: "alice"},
{Account: "config", Key: "fallback", Value: "bob"},
},
})
require.Equal(t, map[interpreter.MetaDependency]struct{}{
{Account: "config", Key: "recipient"}: {},
{Account: "config", Key: "fallback"}: {},
}, deps.MetaReads)
} |
3 participants
This PR:
internal/interpreter/evaluate_expr.go), so that the interpreter can eval exprs using that one instead of the whole interpreter stateThe exact algo for balances tracking is:
allowing unbounded overdraftorallowing overdraft up to ..isn't specified, it counts as bounded overdraft, unless it's@world. Otherwise, that's the source of truth. For example: the@world allowing overdraft up to [ASSET 0]counts as bounded overdraft (recorded as read+write)In order to get 3. properly, we make sure we eval every single expr of the ast (even though it's not used as source or dest). For example, a "max" clause could be using a balance() expr, which we need to track
Note that we explicitly avoid scaling to be implemented, and output an error. That's ok, as scaling isn't documented yet, nor properly specified