feat!: expose color as a first-class Posting property#139
Open
gfyrag wants to merge 12 commits into
Open
Conversation
Color is now carried as a dedicated field on the output Posting type and as
a separate dimension throughout the interpreter, instead of being encoded as
an asset suffix (`USD_RED`). This makes the numscript ↔ host contract clean:
downstream consumers (ledgers, indexers, reporting) can segregate balances
by (account, asset, color) without having to parse asset strings.
BREAKING CHANGES:
- Posting gains `Color string` (json `"color"`).
- BalanceQuery shape: `map[string][]string` → `map[string][]AssetColor`
where `AssetColor{Asset, Color string}`. Store implementations must
honor color as a separate filter rather than splitting suffixed assets.
- Balances shape: `map[account]map[asset]*big.Int` →
`map[account]map[asset]map[color]*big.Int`. The (asset, color) pair is
now an explicit key. `Balances.UnmarshalJSON` accepts both the new
colored form and the legacy `{asset: amount}` shorthand (parsed as the
uncolored bucket) so existing JSON fixtures keep working without a
forced migration.
- `coloredAsset()` helper removed; nothing in the public or internal API
encodes color into the asset string anymore.
- New public helper `Uncolored(amount)` to ergonomically build a
single-bucket `ColorBalance` in tests / static fixtures.
The semantics already exercised by the experimental-asset-colors feature
flag stay the same — only the wire format changes. Existing colored .num
specs were migrated to the new shape and now assert on `posting.color` in
addition to `posting.asset`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Linter diff in the way? Review this PR in Change Stack to focus on meaningful changes and expand context only when needed. 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 ignored due to path filters (5)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR makes balances color-aware: balances map account→asset→color→amount, queries use AssetColor, postings carry Color, JSON marshal/unmarshal and pretty-printing are updated, the funds queue and batching are refactored, and tests and tooling are updated for the new model. ChangesAsset-Color Balance Model
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 #139 +/- ##
==========================================
+ Coverage 66.96% 67.79% +0.82%
==========================================
Files 47 48 +1
Lines 5068 5192 +124
==========================================
+ Hits 3394 3520 +126
+ Misses 1477 1467 -10
- Partials 197 205 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/asset_scaling.go`:
- Around line 46-55: The loop that selects assets uses strings.HasPrefix(asset,
baseAsset) which wrongly includes assets like "USDT" for baseAsset "USD"; update
the condition in the loop (the block iterating over balance and referencing
getAssetScale and result) to accept only the exact base asset or children in the
form baseAsset + "/" (i.e., asset == baseAsset || strings.HasPrefix(asset,
baseAsset + "/")), leaving the rest of the logic (retrieving amount from
colorMap, calling getAssetScale, and assigning result[scale]) unchanged.
- Around line 41-55: getAssets is dropping the color dimension by hardcoding
colorMap[""], which allows colored scaling paths to improperly use uncolored
buckets; fix by making getAssets color-aware: change its signature to accept a
color string (e.g., getAssets(balance AccountBalance, baseAsset, color string)
map[int64]*big.Int), look up amount := colorMap[color] instead of colorMap[""],
and propagate this new parameter to all callers (or alternatively validate and
reject non-empty colors before calling getAssets if you prefer to keep the old
signature); ensure callers in scaling lookup pass source.Color so the correct
color bucket is used.
In `@internal/mcp_impl/handlers.go`:
- Around line 40-57: The code truncates float64 to *big.Int (via
big.NewFloat(...).Int) for both the top-level amount (perAssetRaw -> amount) and
per-color amounts (colorMap -> amountRaw -> amount), allowing fractional values
like 100.9 and losing precision for large integers; change the parsing to
require exact integers: use json.Number (or decoder.UseNumber) or otherwise
validate that the incoming numeric value is an integer (no fractional part and
within exact range) before converting to big.Int, and return a
NewToolResultError when a non-integer or out-of-range value is encountered;
update the branches handling perAssetRaw and amountRaw and ensure conversion to
big.Int uses a string-to-big.Int path (e.g., SetString) rather than
big.NewFloat(...).Int to avoid truncation and rounding issues.
🪄 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: 3ea33a0b-01a1-4d14-9fef-bd22c31487a7
⛔ Files ignored due to path filters (12)
internal/interpreter/__snapshots__/balances_test.snapis excluded by!**/*.snap,!**/*.snapinternal/interpreter/testdata/script-tests/experimental/asset-colors/color-inorder-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance-when-missing-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restriction-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-send-overdrat.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-with-asset-precision.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/no-double-spending-in-colored-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/no-double-spending-in-colored-send.num.specs.jsonis excluded by!**/*.jsoninternal/specs_format/__snapshots__/runner_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (16)
internal/cmd/test_init.gointernal/cmd/test_init_test.gointernal/interpreter/asset_scaling.gointernal/interpreter/balances.gointernal/interpreter/balances_test.gointernal/interpreter/batch_balances_query.gointernal/interpreter/color_semantics_test.gointernal/interpreter/funds_queue_color_test.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_test.gointernal/mcp_impl/handlers.gointernal/specs_format/index.gointernal/specs_format/parse_test.gointernal/specs_format/run_test.gonumscript.gonumscript_test.go
Comment on lines
+41
to
+55
| // getAssets returns, for a given baseAsset, the per-scale uncolored balance. | ||
| // Asset scaling operates on the uncolored bucket only — colored funds are not | ||
| // implicitly converted across scales. | ||
| func getAssets(balance AccountBalance, baseAsset string) map[int64]*big.Int { | ||
| result := make(map[int64]*big.Int) | ||
| for asset, amount := range balance { | ||
| if strings.HasPrefix(asset, baseAsset) { | ||
| _, scale := getAssetScale(asset) | ||
| result[scale] = amount | ||
| for asset, colorMap := range balance { | ||
| if !strings.HasPrefix(asset, baseAsset) { | ||
| continue | ||
| } | ||
| amount, ok := colorMap[""] | ||
| if !ok { | ||
| continue | ||
| } | ||
| _, scale := getAssetScale(asset) | ||
| result[scale] = amount |
Contributor
There was a problem hiding this comment.
Don't drop the color dimension during scaling lookup.
getAssets now hardcodes colorMap[""], but scaling sources still accept source.Color. That means a colored scaling path can convert uncolored balances and then withdraw nothing from the requested color bucket, which is enough for send all to emit bogus intermediary conversion postings. Either make this helper color-aware or reject non-empty colors before calling it.
🤖 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/asset_scaling.go` around lines 41 - 55, getAssets is
dropping the color dimension by hardcoding colorMap[""], which allows colored
scaling paths to improperly use uncolored buckets; fix by making getAssets
color-aware: change its signature to accept a color string (e.g.,
getAssets(balance AccountBalance, baseAsset, color string) map[int64]*big.Int),
look up amount := colorMap[color] instead of colorMap[""], and propagate this
new parameter to all callers (or alternatively validate and reject non-empty
colors before calling getAssets if you prefer to keep the old signature); ensure
callers in scaling lookup pass source.Color so the correct color bucket is used.
Comment on lines
+46
to
+55
| for asset, colorMap := range balance { | ||
| if !strings.HasPrefix(asset, baseAsset) { | ||
| continue | ||
| } | ||
| amount, ok := colorMap[""] | ||
| if !ok { | ||
| continue | ||
| } | ||
| _, scale := getAssetScale(asset) | ||
| result[scale] = amount |
Contributor
There was a problem hiding this comment.
Use exact base-asset matching here.
strings.HasPrefix(asset, baseAsset) will treat unrelated assets like USDT as part of the USD scaling pool. Match only the base asset itself or baseAsset/… children.
Suggested fix
for asset, colorMap := range balance {
- if !strings.HasPrefix(asset, baseAsset) {
+ if asset != baseAsset && !strings.HasPrefix(asset, baseAsset+"/") {
continue
}
amount, ok := colorMap[""]
if !ok {
continue🤖 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/asset_scaling.go` around lines 46 - 55, The loop that
selects assets uses strings.HasPrefix(asset, baseAsset) which wrongly includes
assets like "USDT" for baseAsset "USD"; update the condition in the loop (the
block iterating over balance and referencing getAssetScale and result) to accept
only the exact base asset or children in the form baseAsset + "/" (i.e., asset
== baseAsset || strings.HasPrefix(asset, baseAsset + "/")), leaving the rest of
the logic (retrieving amount from colorMap, calling getAssetScale, and assigning
result[scale]) unchanged.
Per ledger #234 feedback: the dual JSON parse path was a backward-compat
shim that masked the schema break. With the new (asset, color) keying,
the only canonical shape is
{"<account>": {"<asset>": {"<color>": <amount>, ...}}}
where color "" is the uncolored bucket. The flat shorthand
\`{"<asset>": <amount>}\` is no longer accepted; balances must spell out
the empty-color key explicitly.
Changes:
- Remove \`Balances.UnmarshalJSON\` and \`decodeColorBalance\`. Go's default
JSON unmarshal walks the natural 3-level map directly.
- \`mcp_impl.parseBalancesJson\` rejects the shorthand with a clearer
error message (the helper still hand-parses a generic \`map[string]any\`
coming from the MCP transport).
- Sweep the 121 \`.num.specs.json\` fixtures via jq to convert every
\`"<asset>": <number>\` to \`"<asset>": {"": <number>}\`. The walk only
touches \`balances\` (top-level and \`testCases[].balances\`) — movements
and post-commit volumes keep their native shapes.
- Update \`specs_format\` table tests for the new shape (run_test.go,
runner_test.go, parse_test.go).
- Refresh the runner_test snapshot (\`TestSchemaErrSpecs\` now reports
the friendlier \`cannot unmarshal number into ColorBalance\` instead of
going through the bespoke shorthand path).
- Update \`color_semantics_test.go\`: a single \`TestBalancesJSONShape\`
asserts the canonical parse, plus \`TestBalancesJSONRejectsFlatShorthand\`
pins the rejection.
All numscript test packages green; pre-commit clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… invariant The Pull(amount, color *string) signature accepted an optional color filter that is never used by the interpreter — the only caller is pushReceiver, which always passes nil through PullAnything. Whatever balance check a colored source needs has already happened in tryTakingFromAccount before any Sender is pushed onto the queue: CalculateSafeWithdraw caps the pushed amount at the source's (asset, color) balance, and tryTakingExact raises MissingFundsErr if the queue can't supply the requested total. Cleaning up: - Pull's signature is now Pull(requiredAmount *big.Int) — the unused color filter branch is removed and the function gets a doc-comment spelling out the upstream-bounded / caller-validated contract. - PullColored and PullUncolored wrappers are dropped (no in-tree callers outside the tests that exercised them directly). - funds_queue_test.go: the TestPullColored* + TestReconcileColored* tests are removed (they covered the removed branch). TestPush switches from PullUncolored to PullAnything (functionally identical). - funds_queue_color_test.go: collapses to a single TestFundsQueueCompactDoesNotMergeAcrossColors that still pins the invariant we actually care about — compactTop never merges across colors. The PullColored / PullUncolored selectivity tests went away with their subject. No behavioral change for the interpreter: the removed branch was unreachable from anywhere except the deleted tests, and all color-segregation integration tests (color_semantics_test.go, testdata/script-tests/experimental/asset-colors/) still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
gfyrag
added a commit
that referenced
this pull request
Jun 3, 2026
`getAssets(balance, baseAsset)` uses `strings.HasPrefix(asset, baseAsset)`,
which accidentally matches any asset whose key shares the base name as a
string prefix:
- `USDT` and `USDT/2` are matched as scaled variants of `USD`
- `USD_RED` and `USD_RED/2` (suffix-encoded color variants emitted by
the experimental asset-colors feature flag) are also matched
Worse: because the result map is keyed by precision scale only, an entry
like `USD_RED/2 = 500` overwrites the true `USD/2 = 2` value in the
scaling pool. A user holding both `USD/2 = 2` and `USD_RED/2 = 500` on
the same account will see scaling compute conversions against the wrong
amount — silent corruption.
This commit introduces a failing regression test
`TestGetAssetsRejectsSpuriousPrefixMatches` that pins the bug. CI is
intentionally red on this branch.
The fix is in a stacked PR that targets this branch — merging it
flips the test green.
Flagged by CodeRabbit during review of #139.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
flemzord
reviewed
Jun 4, 2026
| if !ok { | ||
| return interpreter.Balances{}, mcp.NewToolResultError(fmt.Sprintf("Expected float for amount: %v", amountRaw)) | ||
| } | ||
| n, _ := big.NewFloat(amount).Int(new(big.Int)) |
Member
There was a problem hiding this comment.
This still converts JSON numbers through float64 and then big.NewFloat(...).Int(...), which silently truncates fractions and can round large integers before conversion. Since balances are ledger amounts, this should reject non-integer or non-exact JSON numbers, or include the validation from PR #143 before #139 is merged standalone.
flemzord
reviewed
Jun 4, 2026
| _, scale := getAssetScale(asset) | ||
| result[scale] = amount | ||
| for asset, colorMap := range balance { | ||
| if !strings.HasPrefix(asset, baseAsset) { |
Member
There was a problem hiding this comment.
This matches any asset starting with baseAsset, so getAssets(balance, "USD") also includes assets like USDC or USDX/2 in the scaling pool. The match should be exact base asset or baseAsset + "/"; otherwise unrelated asset balances can be converted during asset scaling.
flemzord
reviewed
Jun 4, 2026
|
|
||
| n, _ := big.NewFloat(amount).Int(new(big.Int)) | ||
| iBalances[account][asset] = n | ||
| // Expected shape: { "USD/2": { "": 100, "RED": 50 } }. |
Member
There was a problem hiding this comment.
The parser now expects the strict colored shape, but the MCP evaluate tool description still documents the old flat shape ({"USD/2": 100}). Callers following the advertised schema will get runtime errors. Please update the tool description/example to show {"USD/2": {"": 100}} and mention that "" is the uncolored bucket.
flemzord
reviewed
Jun 4, 2026
| Destination string `json:"destination"` | ||
| Amount *big.Int `json:"amount"` | ||
| Asset string `json:"asset"` | ||
| Color string `json:"color"` |
Member
There was a problem hiding this comment.
Since Color is now part of the posting contract, pretty output should expose it too. PrettyPrintPostings still renders only source, destination, asset, and amount, so numscript run --output-format pretty and spec failure output can hide the only difference between two colored postings.
The previous shape forced every balance entry into a nested
{color: amount} map and rejected the bare-number shorthand. Two problems
surfaced in the PR thread:
1. `""` is an awkward JSON key for the uncolored bucket — it leaks an
internal representation choice into every specs.json / inputs.json.
2. Adding orthogonal dimensions later (scopes, scales) would require
another level of nesting and break readers again.
The wire format becomes value-object based:
- bare integer for the uncolored shorthand: "USD/2": 100
- single value-object: "USD/2": { "color": "RED", "amount": 50 }
- array of value-objects (canonical multi-color): "USD/2": [{ "amount": 100 }, { "color": "RED", "amount": 50 }]
Reading is tolerant of all three forms; writing always emits the
canonical form (bare number when uncolored-only, sorted array
otherwise). Future dimensions land as new fields on the value-object,
not as new map levels.
The 116 .num.specs.json fixtures + inline test strings are updated.
JSON schemas (specs / inputs) gain a BalanceEntry definition.
When color is part of the Posting contract but absent from the pretty printer, two postings that differ only by color render identically — the spec failure output (and `numscript run -o pretty`) hide the only distinguishing field. The Color column is added conditionally: present as soon as any posting in the batch carries a non-empty color; hidden otherwise so the uncolored output is byte-identical to the previous behaviour.
parseBalancesJson now re-marshals the decoded MCP payload through Balances.UnmarshalJSON, so the tool accepts the same three forms documented elsewhere (bare number / value-object / array). The decoded amount path goes via *big.Int directly rather than the previous float64 → big.Int truncation. The tool description and example block are rewritten to match.
Existing downstream specs files using {color: amount} maps can be
upgraded in place with `scripts/migrate-specs-to-color.sh path/...`.
The jq rules mirror the canonical write form: uncolored-only collapses
to a bare number, single color emits one value-object, multi-color
emits an array sorted by color.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/migrate-specs-to-color.sh (1)
56-61: 💤 Low valueTemp file not cleaned up on jq failure.
If
jqfails (e.g., malformed JSON), the script exits due toset -ebut leaves the temp file behind. For a one-off migration script this is acceptable, but consider using a trap for cleanup.🛠️ Optional improvement
for f in "$@"; do tmp="$(mktemp)" + trap 'rm -f "$tmp"' EXIT jq --indent 2 "$JQ_FILTER" "$f" > "$tmp" # Preserve trailing newline behaviour of jq output. mv "$tmp" "$f" + trap - EXIT done🤖 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 `@scripts/migrate-specs-to-color.sh` around lines 56 - 61, The script creates a per-file temp file in the loop using tmp="$(mktemp)" and can leave that temp behind if jq fails (set -e); add a cleanup trap to remove temp files on exit. Implement a small mechanism (e.g., an array TMP_FILES or a function register_tmp) to record each tmp created, set trap 'rm -f "${TMP_FILES[@]:-}"' EXIT before the loop, and ensure you append each tmp to TMP_FILES right after mktemp; then continue writing jq output to "$tmp" and mv on success as before so any tmp is removed on script exit or failure.
🤖 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.
Nitpick comments:
In `@scripts/migrate-specs-to-color.sh`:
- Around line 56-61: The script creates a per-file temp file in the loop using
tmp="$(mktemp)" and can leave that temp behind if jq fails (set -e); add a
cleanup trap to remove temp files on exit. Implement a small mechanism (e.g., an
array TMP_FILES or a function register_tmp) to record each tmp created, set trap
'rm -f "${TMP_FILES[@]:-}"' EXIT before the loop, and ensure you append each tmp
to TMP_FILES right after mktemp; then continue writing jq output to "$tmp" and
mv on success as before so any tmp is removed on script exit or failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17264693-5a68-489a-aa95-482f529ac13a
⛔ Files ignored due to path filters (47)
inputs.schema.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/mixed-source-prefer-single-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up-many.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/transfer-example.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/send-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/balance-not-found.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/bigint-literal.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/empty-postings.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/account-interpolation/account-interp.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-inorder-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance-when-missing-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restriction-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/no-double-spending-in-colored-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/no-double-spending-in-colored-send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/no-solution.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-all-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-with-oneof.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/update-swap-account-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-amount-function/get-amount-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-asset-function/get-asset-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/mid-script-function-call/expr-in-var-origin.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-all-failing.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-positive.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/reach-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/feature-flag-syntax.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/floating-perc.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/floating-perc2.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-infix-monetary.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-infix-number.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-prefix-number.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/neg-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/negative-max-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-when-negative.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-allt-max-when-no-amount.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings-explicit-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings-explicit-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/specs_format/__snapshots__/runner_test.snapis excluded by!**/*.snap,!**/*.snapspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (9)
internal/interpreter/balances.gointernal/interpreter/balances_json.gointernal/interpreter/color_semantics_test.gointernal/interpreter/funds_queue.gointernal/interpreter/funds_queue_color_test.gointernal/interpreter/funds_queue_test.gointernal/interpreter/interpreter.gointernal/mcp_impl/handlers.goscripts/migrate-specs-to-color.sh
💤 Files with no reviewable changes (1)
- internal/interpreter/balances.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/interpreter/interpreter.go
`"color": ""` in every uncolored posting is noise: an absent color field carries the same meaning as the empty-string bucket, so emitting it leaks an internal convention into every JSON payload. With omitempty, uncolored postings (the dominant case) serialize as they did before color became first-class. Round-trip stays correct because Go's zero-value for the missing field is the empty string, which is exactly the uncolored-bucket key on the decode side.
The fixtures that exercise the experimental color split kept the `"color": ""` entry on uncolored postings — a leftover from the first migration pass when `COIN_BLUE` was split into `asset: COIN` + `color: <suffix>`. With Posting.Color now `omitempty`, the runtime no longer emits the empty field, so the assertion fixtures shouldn't either.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Color is now carried as a dedicated field on the output
Postingtype andas a separate dimension throughout the interpreter, instead of being
encoded as an asset suffix (
USD_RED). The numscript ↔ host contractbecomes a clean two-dimensional
(asset, color)model: downstreamconsumers (ledgers, indexers, reporting) can segregate balances by
(account, asset, color)without parsing asset strings.The semantics already exercised by the
experimental-asset-colorsfeature flag are unchanged. Only the wire format and Go API change.
Breaking changes
Posting{Source, Destination, Amount, Asset}Color string(json"color")BalanceQuerymap[string][]string(asset strings, color encoded as suffix)map[string][]AssetColorwithAssetColor{Asset, Color string}Balances(in-memory)map[account]map[asset]*big.Intmap[account]map[asset]map[color]*big.IntBalances(JSON wire){"alice": {"USD/2": 100}}{"alice": {"USD/2": {"": 100, "RED": 50}}}coloredAsset()helperASSET_COLORin asset stringsfundsQueue.PullsignaturePull(amount, color *string)Pull(amount)— the color filter parameter was dead code, droppedfundsQueue.PullColored,PullUncoloredNo backwards-compatibility shim. The flat-shorthand JSON shape
(
{"USD/2": 100}) is not accepted — every balance amount must bewritten under an explicit color, with
""for the uncolored bucket. The130 fixture
.num.specs.jsonfiles in the repo were rewrittenaccordingly in a single sweep (jq one-liner over the tree).
A small public helper
Uncolored(amount)is added so building testbalances stays terse:
Uncolored(big.NewInt(100))producesColorBalance{"": amount}.Why now
A consumer ledger (POC) needs strict color-segregated balances stored as
(account, asset, color)keys, with color flowing through the emittedposting. As long as numscript strips color from postings and folds it
into the asset string, consumers cannot do that cleanly. This PR fixes
the contract upstream so we do not need to translate at the boundary.
What's covered by tests
color_semantics_test.go(black-box, 17 tests): color propagationthrough emitted postings, isolation between colors, source-restriction
semantics, allocation distributing color across legs, send-all
draining only the targeted color, JSON shape acceptance/rejection,
charset enforcement (
^[A-Z]*$).funds_queue_color_test.go(white-box, 1 test):compactTopnever merges senders across colors. The
PullColored/PullUncoloredselectivity tests are gone with their subjects — the upstream
invariant (
tryTakingFromAccountcaps push at the per-color balance,tryTakingExactvalidates completeness on pull) makes the filterparameter unnecessary.
balances_test.go,interpreter_test.go,numscript_test.go,specs_format/*_test.go, plus the 130.num.specs.jsonfixtures.Test plan
just pre-commitclean.via a
replace/pseudo-version pin and exercises segregatedbalances end-to-end (formancehq/ledger-v3-poc#234).
- Color +
with scaling(Critical): the grammar alreadyrefuses the combination, and the
SourceWithScaling.Colorfield is never populated by the parser — leaving as-is.
-
HasPrefixexact-match ingetAssets(Major): fixed instacked PR #140
on top of failing test #141.
- MCP
float64 → big.Inttruncation (Major): fixed instacked PR #143
on top of failing test #142.
🤖 Generated with Claude Code