feat!: expose color as a first-class Posting property#139
Open
gfyrag wants to merge 6 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
WalkthroughThis PR introduces per-asset-per-color balance tracking throughout the interpreter. 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 Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.21% +0.24%
==========================================
Files 47 47
Lines 5068 5109 +41
==========================================
+ Hits 3394 3434 +40
- Misses 1477 1479 +2
+ Partials 197 196 -1 ☔ View full report in Codecov by Sentry. 🚀 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.
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