feat: suffix balance/amount fields with explicit unit (Sat or Msat)#2153
feat: suffix balance/amount fields with explicit unit (Sat or Msat)#2153Anshumancanrock wants to merge 1 commit intogetAlby:masterfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR systematically adds satoshi ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/models.go (1)
88-106:⚠️ Potential issue | 🟡 Minor
Appstill ships ambiguous sat fields.Line 98 already knows this value is sats (
MaxAmountSat), but it is still serialized asmaxAmount, andbudgetUsageon Line 99 is still unitless too. AddingbalanceMsatmakes this payload only partially explicit; clients will still need special cases for two common app amounts. AddmaxAmountSat/budgetUsageSataliases or rename the JSON tags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/models.go` around lines 88 - 106, The App struct exposes ambiguous sat fields; change the JSON tags on MaxAmountSat and BudgetUsage to explicit sat names (e.g., update MaxAmountSat `json:"maxAmount"` → `json:"maxAmountSat"` and BudgetUsage `json:"budgetUsage"` → `json:"budgetUsageSat"`) or add alias fields if backward compatibility is required so clients receive explicit sat units; update any code that marshals/unmarshals App to use the new JSON keys (references: struct App, fields MaxAmountSat and BudgetUsage, and consider BalanceMsat consistency).
🧹 Nitpick comments (1)
lnclient/models.go (1)
196-199: Avoid publishing new ambiguous MPP balance fields.Lines 196-199 add
NextMaxSpendableMPPandNextMaxReceivableMPPright next to the explicit*MPPMsatfields. Sinceapi/models.go:365aliaseslnclient.BalancesResponse, these tags go straight into the public HTTP schema, and the newly expanded surface is ambiguous again. Prefer exposing only the explicit-unit keys, or deprecate the unsuffixed aliases immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/models.go` around lines 196 - 199, The new unsuffixed balance fields NextMaxSpendableMPP and NextMaxReceivableMPP are ambiguous and will be published via the lnclient.BalancesResponse alias used in api/models.go:365; remove or hide those two fields from the public JSON surface by deleting them from lnclient/models.go (or changing their struct tags to json:"-" and adding a deprecation comment) and keep only the explicit-unit fields NextMaxSpendableMPPMsat and NextMaxReceivableMPPMsat; ensure any internal code referencing NextMaxSpendableMPP/NextMaxReceivableMPP is updated to use the explicit-suffixed fields or converted appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/models.go`:
- Around line 88-106: The App struct exposes ambiguous sat fields; change the
JSON tags on MaxAmountSat and BudgetUsage to explicit sat names (e.g., update
MaxAmountSat `json:"maxAmount"` → `json:"maxAmountSat"` and BudgetUsage
`json:"budgetUsage"` → `json:"budgetUsageSat"`) or add alias fields if backward
compatibility is required so clients receive explicit sat units; update any code
that marshals/unmarshals App to use the new JSON keys (references: struct App,
fields MaxAmountSat and BudgetUsage, and consider BalanceMsat consistency).
---
Nitpick comments:
In `@lnclient/models.go`:
- Around line 196-199: The new unsuffixed balance fields NextMaxSpendableMPP and
NextMaxReceivableMPP are ambiguous and will be published via the
lnclient.BalancesResponse alias used in api/models.go:365; remove or hide those
two fields from the public JSON surface by deleting them from lnclient/models.go
(or changing their struct tags to json:"-" and adding a deprecation comment) and
keep only the explicit-unit fields NextMaxSpendableMPPMsat and
NextMaxReceivableMPPMsat; ensure any internal code referencing
NextMaxSpendableMPP/NextMaxReceivableMPP is updated to use the explicit-suffixed
fields or converted appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee4d1014-7010-4769-8e4f-65d22dd8d189
📒 Files selected for processing (11)
api/api.goapi/lsp.goapi/models.goapi/transactions.gofrontend/src/types.tslnclient/cashu/cashu.golnclient/ldk/ldk.golnclient/lnd/lnd.golnclient/models.golnclient/phoenixd/phoenixd.gotests/mock_ln_client.go
| return nil, err | ||
| } | ||
| response.Balance = balance | ||
| response.BalanceMsat = balance |
There was a problem hiding this comment.
what if we have an addition field for Sat too. That way the client can choose whether it wants to use the sat or msat field. The existing e.g. Balance field is unclear if it's sats or msats.
|
Thanks for the PR! I think there are some inconsistencies currently which need to be resolved. And we need to carefully review to ensure there are no breaking changes on the API level, only additions. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/api.go (1)
1214-1228:⚠️ Potential issue | 🟠 MajorUpdate Cashu and Phoenixd GetBalances to populate all OnchainBalanceResponse fields.
The Cashu and Phoenixd implementations are incomplete. They only populate 4 fields (
Spendable,SpendableSat,Total,TotalSat) with zeros and hardcoded values, while theOnchainBalanceResponsestruct defines 15 fields. LDK and LND correctly populate all fields includingSpendableMsat,TotalMsat,Reserved*,PendingBalances*, andInternalBalances.This creates API inconsistency across backends. Per the interface design pattern, all four backend implementations (LDK, LND, Phoenixd, Cashu) must maintain consistent return values for the LNClient interface.
- Cashu (lnclient/cashu/cashu.go, lines 270-275): Missing
SpendableMsat,TotalMsat,Reserved*,PendingBalances*,InternalBalances- Phoenixd (lnclient/phoenixd/phoenixd.go, lines 116-121): Missing
SpendableMsat,TotalMsat,Reserved*,PendingBalances*,InternalBalancesUpdate both implementations to return the complete struct with meaningful values where applicable, following the LDK and LND patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/api.go` around lines 1214 - 1228, The Cashu and Phoenixd GetBalances implementations are returning only Spendable, SpendableSat, Total and TotalSat; update the GetBalances functions in the cashu and phoenixd LN client implementations (the GetBalances methods in lnclient/cashu/cashu.go and lnclient/phoenixd/phoenixd.go) to populate the full OnchainBalanceResponse struct (include SpendableMsat, TotalMsat, ReservedOnchain, ReservedOnchainSat, ReservedOnchainMsat, PendingBalancesOnchain, PendingBalancesOnchainSat, PendingBalancesOnchainMsat, InternalBalances, InternalBalancesSat, InternalBalancesMsat) following the same semantics used in LDK/LND: convert sat -> msat (msat = sat*1000) for msat fields, fill Reserved*/Pending*/Internal* with meaningful values (zero where not applicable) and ensure consistency between sat and msat fields and between Spendable/Total and their sat/msat counterparts so the LNClient GetBalances contract is met.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/api.go`:
- Around line 1214-1228: The Cashu and Phoenixd GetBalances implementations are
returning only Spendable, SpendableSat, Total and TotalSat; update the
GetBalances functions in the cashu and phoenixd LN client implementations (the
GetBalances methods in lnclient/cashu/cashu.go and
lnclient/phoenixd/phoenixd.go) to populate the full OnchainBalanceResponse
struct (include SpendableMsat, TotalMsat, ReservedOnchain, ReservedOnchainSat,
ReservedOnchainMsat, PendingBalancesOnchain, PendingBalancesOnchainSat,
PendingBalancesOnchainMsat, InternalBalances, InternalBalancesSat,
InternalBalancesMsat) following the same semantics used in LDK/LND: convert sat
-> msat (msat = sat*1000) for msat fields, fill Reserved*/Pending*/Internal*
with meaningful values (zero where not applicable) and ensure consistency
between sat and msat fields and between Spendable/Total and their sat/msat
counterparts so the LNClient GetBalances contract is met.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52dc374b-226a-4014-9da9-be41349de925
📒 Files selected for processing (12)
PR_DESCRIPTION.mdapi/api.goapi/lsp.goapi/models.goapi/transactions.gofrontend/src/types.tslnclient/cashu/cashu.golnclient/ldk/ldk.golnclient/lnd/lnd.golnclient/models.golnclient/phoenixd/phoenixd.gotests/mock_ln_client.go
✅ Files skipped from review due to trivial changes (2)
- PR_DESCRIPTION.md
- frontend/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- api/transactions.go
- lnclient/phoenixd/phoenixd.go
- tests/mock_ln_client.go
- lnclient/cashu/cashu.go
- lnclient/models.go
- api/models.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lnclient/cashu/cashu.go (1)
280-291: Explicitly set receivable*Satfields instead of relying on zero-values.
TotalReceivableSat,NextMaxReceivableSat, andNextMaxReceivableMPPSatare currently implicit. Please set them explicitly for consistency and future maintainability.Proposed patch
Lightning: lnclient.LightningBalanceResponse{ TotalSpendable: balance, TotalSpendableSat: balance / 1000, TotalSpendableMsat: balance, TotalReceivable: 0, + TotalReceivableSat: 0, TotalReceivableMsat: 0, NextMaxSpendable: balance, NextMaxSpendableSat: balance / 1000, NextMaxSpendableMsat: balance, NextMaxReceivable: 0, + NextMaxReceivableSat: 0, NextMaxReceivableMsat: 0, NextMaxSpendableMPP: balance, NextMaxSpendableMPPSat: balance / 1000, NextMaxSpendableMPPMsat: balance, NextMaxReceivableMPP: 0, + NextMaxReceivableMPPSat: 0, NextMaxReceivableMPPMsat: 0, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/cashu/cashu.go` around lines 280 - 291, The struct literal in cashu.go initializes multiple receivable and spendable fields but leaves the sat variants implicit; explicitly add TotalReceivableSat, NextMaxReceivableSat, and NextMaxReceivableMPPSat (set them to 0) alongside the existing fields (e.g., TotalReceivable, NextMaxReceivable, NextMaxReceivableMPP) so the initialization is consistent and future-proof when editing functions like the constructor/initializer that builds this balance object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/lsp.go`:
- Around line 84-86: invoiceAmount was truncated to whole sats before computing
InvoiceAmountMsat, causing loss of sub-sat precision; instead preserve the
original decoded millisat amount (e.g., decodedMsat or invoiceMsatDecoded
captured from the decoder) and use that for the InvoiceAmountMsat field while
keeping InvoiceAmount and InvoiceAmountSat set to the truncated sats variable
(invoiceAmount). Update the struct assignment where InvoiceAmountMsat is set
(the InvoiceAmountMsat field in the code near the InvoiceAmount/InvoiceAmountSat
assignments) to use the preserved decoded msat variable rather than
invoiceAmount*1000.
---
Nitpick comments:
In `@lnclient/cashu/cashu.go`:
- Around line 280-291: The struct literal in cashu.go initializes multiple
receivable and spendable fields but leaves the sat variants implicit; explicitly
add TotalReceivableSat, NextMaxReceivableSat, and NextMaxReceivableMPPSat (set
them to 0) alongside the existing fields (e.g., TotalReceivable,
NextMaxReceivable, NextMaxReceivableMPP) so the initialization is consistent and
future-proof when editing functions like the constructor/initializer that builds
this balance object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53521dc9-b094-4d49-b9b0-f4ee44d15025
📒 Files selected for processing (11)
api/api.goapi/lsp.goapi/models.goapi/transactions.gofrontend/src/types.tslnclient/cashu/cashu.golnclient/ldk/ldk.golnclient/lnd/lnd.golnclient/models.golnclient/phoenixd/phoenixd.gotests/mock_ln_client.go
✅ Files skipped from review due to trivial changes (2)
- tests/mock_ln_client.go
- lnclient/lnd/lnd.go
🚧 Files skipped from review as they are similar to previous changes (5)
- lnclient/phoenixd/phoenixd.go
- lnclient/models.go
- lnclient/ldk/ldk.go
- api/api.go
- frontend/src/types.ts
…e/amount properties
|
@rolznz Thanks for the review. I’ve addressed the inconsistencies by adding explicit Sat/Msat companion fields for ambiguous amount/balance values, while keeping existing fields unchanged for backward compatibility. I also rechecked the mapping paths to ensure this is API-additive only with no intended breaking changes. I can also add explicit regression tests for the new Sat/Msat fields if you want extra safety before merge. |
2 participants
Summary
Several API response fields had ambiguous names like
balance,amount,fee,localBalance, with no indication of whether the value is in sats or millisats. This makes it easy for clients to misinterpret values and introduce subtle bugs.What this PR does
Fixes #2146
Adds a new suffixed field alongside every ambiguous numeric property so the unit is always explicit:
balance→ also returnsbalanceMsatamount→ also returnsamountMsatfeesPaid→ also returnsfeesPaidMsatlocalBalance→ also returnslocalBalanceMsatsendAmount→ also returnssendAmountSatSummary by CodeRabbit
*Sat) and millisatoshi (*Msat) denomination fields for balances, amounts, and fees.