fix: save & overdraft interaction + world var error #1177
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughOP_SAVE now guards balance writes by checking for existing account/balance entries before updating or zeroing balances; ResolveBalances initializes m.Balances earlier and rejects the special "world" account in certain contexts. Tests added for world-variable usage and a "save all and overdraft" scenario. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1177 +/- ##
==========================================
+ Coverage 81.43% 81.45% +0.02%
==========================================
Files 190 190
Lines 9334 9334
==========================================
+ Hits 7601 7603 +2
+ Misses 1228 1227 -1
+ Partials 505 504 -1 ☔ 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: 0
🧹 Nitpick comments (2)
internal/machine/vm/machine.go (1)
434-456: OP_SAVE guards look correct; consider tightening semantics and readability slightlyThe new guarded writes in
OP_SAVEmatch the intent:
- Only mutate
m.Balanceswhen an account/asset entry already exists, sosavedoesn’t “materialize” balances for accounts that were never tracked.- For
save [ASSET *], only zeroing positive balances and leaving zero/negative untouched correctly fixes the overdraft case where a negative balance was previously reset to 0 before overdraft checks.- For
saveof amachine.Monetary, subtracting from the existing tracked balance keeps the existing “save more than balance” behavior (balance can go negative and is then accounted for by later withdrawal logic).Two minor follow‑ups you might consider:
For the Asset branch, using the numeric helpers would be clearer and avoids the
ToBigInt()detour, e.g.:if balance.Gt(machine.Zero) { balances[v] = machine.Zero }If
Program.NeededBalanceswere ever incomplete, these silent no‑ops whenbalances[a]orbalances[a][asset]is missing could mask miscompilations. If that’s a concern, you could optionally add an internal assertion/log whenDEBUGis on rather than failing hard in production.Given the current design, the behavior looks correct and fixes the save/overdraft interaction covered by the new test.
Please double‑check (via compiler tests) that any account/asset pair reachable in
OP_SAVEis always present inm.Balanceswhen it is meant to affect funding, so the new guards don’t accidentally turn a validsaveinto a no‑op in edge cases.internal/machine/vm/machine_test.go (1)
135-141: Error assertion remains correct;%where is only cosmeticUsing
errors.Is(err, expected.Error)is the right way to compare the error type (including wrapped errors). Changing the format string to use%win the failure message is harmless but slightly unconventional, since wrapping semantics only matter infmt.Errorf. It prints like%vhere.If you want to avoid potential confusion for readers skimming tests, you could keep
%v:require.True(t, errors.Is(err, expected.Error), "got wrong error, want: %v, got: %v", expected.Error, err)Functionally, the current code is fine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (2)
internal/machine/vm/machine.go(2 hunks)internal/machine/vm/machine_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/machine/vm/machine.go (3)
internal/machine/monetary.go (5)
Zero(26-26)Monetary(9-12)Monetary(14-14)MonetaryInt(41-41)MonetaryInt(43-43)internal/machine/vm/program/program.go (1)
Program(10-14)internal/machine/errors.go (1)
NewErrInvalidVars(108-112)
internal/machine/vm/machine_test.go (2)
internal/machine/vm/run.go (1)
Run(57-87)internal/machine/errors.go (1)
ErrInsufficientFund(34-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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (2)
internal/machine/vm/machine.go (1)
509-520: Unconditional Balances init and world-account guard are sensible; ensure coverage of world use-casesInitializing
m.Balancesup front inResolveBalancesis a solid improvement:
- Ensures
Execute’sErrBalancesNotInitializedcheck behaves purely as “ResolveBalances was never called”, not “no balances were requested”.- Avoids accidental writes to a nil map if future code starts assigning outside the current guarded block.
The new
@worldguard in theNeededBalancesloop also makes sense conceptually: it prevents the compiler/runtime from treatingworldas a “locked/bounded” account when it shows up via variables, while direct@worlduses andbalance(@world, …)viaUnresolvedResourceBalancesremain allowed.One thing to confirm:
- For scripts like
monetary $amount = balance(@world, USD/2)or other places where@worldparticipates only inbalance()or as an unbounded source,Program.NeededBalancesshould not contain theworldresource address. Otherwise this new guard would reject previously valid programs and conflict with tests that rely onbalance(@world, …)semantics.If that invariant holds, this change is well-scoped and aligns with the “special unbounded account” semantics of
world.Consider adding (or pointing to) a small focused test that exercises a variable-resolved account equal to
"world"in a context that should now be rejected (e.g., bounded overdraft via a var), to make the new error path explicit.internal/machine/vm/machine_test.go (1)
2271-2288: New “save all and overdraft” test accurately captures the previous bugThis subtest nicely encodes the regression you’re fixing:
- Initial balance
@alice = -10 USDwith an overdraft limit of[USD 10]means there should be no remaining capacity to send funds.save [USD *] from @alicemust not reset the tracked balance to zero when it’s already negative.- The subsequent
send [USD 10]withallowing overdraft up to [USD 10]should therefore fail withErrInsufficientFundand an “insufficient funds” message, as asserted.This directly guards the interaction between
OP_SAVE(asset branch) andwithdrawAlloverdraft logic and will prevent regressions in the scenario the PR targets.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="internal/machine/vm/machine.go">
<violation number="1" location="internal/machine/vm/machine.go:518">
P1: ResolveBalances now rejects `@world` entirely, but other VM code and tests still compile programs that read `balance(@world, …)`, so those scripts will now fail before execution with ErrInvalidVars instead of the expected behavior.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/machine/vm/machine.go (1)
435-456: OP_SAVE now respects tracked balances and preserves existing overdraft usageThe new OP_SAVE logic only mutates
m.Balanceswhen an account/asset pair is already tracked and:
- For
machine.Assetsaves, it zeroes the balance only when the current tracked balance is strictly positive.- For
machine.Monetarysaves, it subtracts the saved amount only when a tracked balance for that asset exists.This aligns with the goal of:
- Avoiding creation of synthetic balances for accounts/assets that are never used as sources.
- Not “resetting” negative balances, which would effectively erase already-used overdraft (the scenario covered by the new “save all and overdraft” test).
Given how
ResolveBalancespopulatesm.Balancesonly forProgram.NeededBalances, these guards look correct and avoid side effects on untracked or world-like accounts while preserving existing tests that rely on saving from positive balances.If you want to tighten this further, you could consider centralizing the “only touch if tracked” pattern (asset + monetary) into a small helper to keep OP_SAVE branches symmetrical and easier to audit in the future, but functionally this is fine as-is.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (2)
internal/machine/vm/machine.go(2 hunks)internal/machine/vm/machine_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/machine/vm/machine.go (3)
internal/machine/monetary.go (5)
Zero(26-26)Monetary(9-12)Monetary(14-14)MonetaryInt(41-41)MonetaryInt(43-43)internal/machine/vm/program/program.go (1)
Program(10-14)internal/machine/errors.go (1)
NewErrInvalidVars(108-112)
internal/machine/vm/machine_test.go (4)
internal/machine/vm/machine.go (1)
Posting(45-50)internal/controller/ledger/errors.go (1)
ErrInvalidVars(148-148)internal/machine/errors.go (2)
ErrInvalidVars(95-97)ErrInsufficientFund(34-36)internal/machine/vm/run.go (1)
Run(57-87)
⏰ 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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (5)
internal/machine/vm/machine_test.go (4)
135-141: Improved error diagnostics for expected vs actual error type/value look correctUsing
errors.Is(err, expected.Error)is consistent with other tests, and the new format string (%[1]v (%[1]T)) avoids the previous misuse of%wwhile giving clear visibility into both value and dynamic type of the expected error.
1702-1721: Good coverage for invalid@worldusage via account variable as sourceThis test precisely exercises the new constraint in
ResolveBalanceswhere@worldcoming from an account variable and used as a source yieldsErrInvalidVarswith the specific explanatory message. This should guard against accidentally treating@worldas a bounded account in the standard interpreter.
1723-1748: Nice positive test ensuring@worldis allowed as a non‑source via variablesThis test complements
TestWorldSourceVariableby validating that@worldused only as a destination via an account variable remains valid and produces the expected posting. This matches the intended “no balances / no restriction when not a source” behavior.
2319-2336: New “save all and overdraft” regression test correctly captures the overdraft bugStarting Alice at
-10and then doingsave [USD *] from @alicefollowed by a bounded overdraft send is a good reproducer: previously, zeroing the balance would effectively reset the overdraft usage. AssertingErrInsufficientFundwith"insufficient funds"ensures the new OP_SAVE semantics preserve existing negative balances and prevent double‑spending across saves + overdraft.internal/machine/vm/machine.go (1)
509-520: ResolveBalances initialization and@worldguard match the intended invariantsTwo behaviors here look appropriate:
m.Balances = make(...)before processing needed balances ensures that even scripts that don’t require any balances will havem.Balancesnon‑nil, soExecuteno longer fails withErrBalancesNotInitializedfor “no-source” programs.- Rejecting any account whose resolved address is
"world"inProgram.NeededBalanceswithNewErrInvalidVarsenforces that@worldcannot appear as a bounded source in the standard interpreter. Literal@worldsources (which are excluded fromNeededBalances) andbalance(@world, …)look to continue flowing through theUnresolvedResourceBalancespath, preserving existing behaviors and the negative‑balance check used byTestVariableBalance.Overall this is consistent with the new tests guarding world-as-variable usage and should close the ambiguity around treating
@worldas a normal account in the standard VM.
Summary by cubic
Fixes save + overdraft interaction and enforces proper @world handling. Save no longer hides negative balances or touches missing balances, so overdraft limits work as expected.
@worldas a variable now errors when used as a source (allowed as destination) with a clearer message.@worldvariable behavior; tweaked error assertion formatting.Written for commit c0f3f4b. Summary will update automatically on new commits.