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 19 commits intogetAlby:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds explicit satoshi ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
|
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. |
|
@rolznz do we want to do it this way, or just add a unit field which returns sat/msat? Wouldn't that be a better option? |
|
@im-adithya can you give an example? how will it work in json? |
|
Whoops sorry, I saw no breaking changes note in the issue and thought this was about adding unit field to |
|
@rolznz would be great if you can have a look, should be good now! |
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 (1)
alby/alby_oauth_service.go (1)
1333-1373:⚠️ Potential issue | 🟠 MajorPopulate
FeeMsatfrom the decoded invoice amount.
FeeMsat: fee * 1000discards sub-sat precision even though this path already decodes the exact BOLT11 amount. KeepFee/FeeSatfromfee_total_sat, but usepaymentRequest.MSatoshifor the new msat field so the response matches the invoice the client will actually pay.Suggested fix
var invoice string var fee uint64 +var feeMsat uint64 if newAutoChannelResponse.Payment != nil { invoice = newAutoChannelResponse.Payment.Bolt11.Invoice fee, err = strconv.ParseUint(newAutoChannelResponse.Payment.Bolt11.FeeTotalSat, 10, 64) if err != nil { @@ paymentRequest, err := decodepay.Decodepay(invoice) if err != nil { logger.Logger.WithError(err).Error("Failed to decode bolt11 invoice") return nil, err } + feeMsat = uint64(paymentRequest.MSatoshi) if fee != uint64(paymentRequest.MSatoshi/1000) { logger.Logger.WithFields(logrus.Fields{ "invoice_amount": paymentRequest.MSatoshi / 1000, "fee": fee, @@ return &AutoChannelResponse{ Invoice: invoice, Fee: fee, FeeSat: fee, - FeeMsat: fee * 1000, + FeeMsat: feeMsat, ChannelSize: channelSize, ChannelSizeSat: channelSize, ChannelSizeMsat: channelSize * 1000, }, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@alby/alby_oauth_service.go` around lines 1333 - 1373, The FeeMsat should come from the decoded BOLT11 amount, not fee*1000; change the code that builds the AutoChannelResponse so that when newAutoChannelResponse.Payment != nil you capture paymentRequest.MSatoshi (from decodepay.Decodepay) into a variable (e.g. invoiceMsat) and later set FeeMsat: invoiceMsat, while leaving Fee and FeeSat derived from fee (fee_total_sat); ensure invoiceMsat is defined in the outer scope so AutoChannelResponse uses paymentRequest.MSatoshi when available (otherwise fall back to fee*1000).
♻️ Duplicate comments (1)
api/api.go (1)
466-467:⚠️ Potential issue | 🟠 MajorExpose an explicit
maxAmountSatfield as well.These handlers only add
maxAmountMsat. Becauseapi/models.gostill serializes the sat value as legacymaxAmount, clients still do not get a unit-suffixed sat companion for this amount field. Please addmaxAmountSatto the model/frontend type and populate it here in bothGetAppandListApps, while keepingmaxAmountas the backward-compatible alias.Also applies to: 631-632
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/api.go` around lines 466 - 467, Add an explicit maxAmountSat field to the model/frontend type (in api/models.go) and populate it in the app response objects in both GetApp and ListApps handlers: set maxAmountSat = maxAmount (satoshis) alongside the existing MaxAmount (legacy alias) and MaxAmountMsat = maxAmount * 1000; keep MaxAmount unchanged for backward compatibility and ensure the new maxAmountSat is marshaled/serialized for clients so they receive a unit-suffixed sat companion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@alby/models.go`:
- Around line 108-111: Add explicit sat-suffixed fields to the model by adding
MinimumChannelSizeSat uint64 `json:"minimumChannelSizeSat"` and
MaximumChannelSizeSat uint64 `json:"maximumChannelSizeSat"` alongside the
existing fields in the struct in alby/models.go, and then populate those fields
where the model is constructed in alby/alby_service.go (use the existing msat
values or the already-computed sat values there to derive the sat companions,
e.g., divide MinimumChannelSizeMsat/MaximumChannelSizeMsat by 1000 or copy the
existing sat variables) so the JSON output includes deterministic sat-suffixed
properties.
In `@api/rebalance.go`:
- Around line 141-145: The current code calculates TotalFeeSat by dividing the
aggregated totalFeeMsat by 1000, which can introduce a rounding difference
versus the legacy per-component sat rounding; change it to compute TotalFeeSat
by summing each component in sats (invoice sats + fee sats) then subtracting
rebalanceChannelRequest.AmountSat (i.e., use paymentRequest.MSatoshi/1000 and
payRebalanceInvoiceResponse.FeeMsat/1000 as the sat components) and assign that
value to RebalanceChannelResponse.TotalFeeSat while keeping TotalFeeMsat as the
precise aggregate (totalFeeMsat) for TotalFeeMsat.
---
Outside diff comments:
In `@alby/alby_oauth_service.go`:
- Around line 1333-1373: The FeeMsat should come from the decoded BOLT11 amount,
not fee*1000; change the code that builds the AutoChannelResponse so that when
newAutoChannelResponse.Payment != nil you capture paymentRequest.MSatoshi (from
decodepay.Decodepay) into a variable (e.g. invoiceMsat) and later set FeeMsat:
invoiceMsat, while leaving Fee and FeeSat derived from fee (fee_total_sat);
ensure invoiceMsat is defined in the outer scope so AutoChannelResponse uses
paymentRequest.MSatoshi when available (otherwise fall back to fee*1000).
---
Duplicate comments:
In `@api/api.go`:
- Around line 466-467: Add an explicit maxAmountSat field to the model/frontend
type (in api/models.go) and populate it in the app response objects in both
GetApp and ListApps handlers: set maxAmountSat = maxAmount (satoshis) alongside
the existing MaxAmount (legacy alias) and MaxAmountMsat = maxAmount * 1000; keep
MaxAmount unchanged for backward compatibility and ensure the new maxAmountSat
is marshaled/serialized for clients so they receive a unit-suffixed sat
companion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d35f0fb-74ce-4348-8869-91464c449226
📒 Files selected for processing (15)
alby/alby_oauth_service.goalby/alby_service.goalby/models.goapi/api.goapi/lsp.goapi/models.goapi/rebalance.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 (1)
- frontend/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/mock_ln_client.go
- lnclient/phoenixd/phoenixd.go
- lnclient/ldk/ldk.go
- lnclient/models.go
- lnclient/lnd/lnd.go
- api/transactions.go
- api/models.go
API Breaking Change Review 🔍I've done a thorough review of every file in this diff checking for breaking changes to the Alby Hub API. ✅ No Breaking Changes to Existing API ResponsesThe PR follows a solid backward-compatible pattern: old fields are preserved (with
This is a clean additive change — existing clients will continue to work unchanged. 👍
|
|
Update on point 1 (inconsistent The pattern should be:
So what looked like inconsistency is actually intentional and correct. Withdrawn! 🙏 |
| ForwardingFeeProportionalMillionths: channel.ForwardingFeeProportionalMillionths, | ||
| UnspendablePunishmentReserve: channel.UnspendablePunishmentReserve, | ||
| UnspendablePunishmentReserveSat: channel.UnspendablePunishmentReserve, | ||
| UnspendablePunishmentReserveMsat: channel.UnspendablePunishmentReserve * 1000, |
There was a problem hiding this comment.
does msats exist here? (same with CounterpartyUnspendablePunishmentReserve)
4 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