Repository navigation
Show complementary amount instead of block number in transaction rows - #553
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a computed secondaryAmount (alternate-currency display) across Android and iOS transaction views, updates widget signatures/call sites to pass/render it, adds direction-aware display helpers in Rust and Kotlin FFI bindings, and surfaces an alert on unsigned-transaction deletion failure. Build config updated. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Mobile UI (Android/iOS)
participant Kotlin as RustWalletManager (Kotlin)
participant FFI as Uniffi FFI Bridge
participant Rust as Rust WalletManager
UI->>Kotlin: request displayAmountWithDirection / displayFiatAmountWithDirection
Kotlin->>FFI: call corresponding uniffi_*_with_direction(...)
FFI->>Rust: invoke Rust formatting helper (amount with direction)
Rust-->>FFI: formatted string (with +/- or direction)
FFI-->>Kotlin: RustBuffer result
Kotlin-->>UI: lifted String (secondaryAmount) for rendering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87ef2088b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+149
to
+153
| private var secondaryAmount: String { | ||
| if case .btc = metadata.fiatOrBtc { | ||
| // primary is BTC, secondary is fiat | ||
| guard let fiatAmount = txn.fiatAmount() else { return privateShow("---") } | ||
| return privateShow(manager.rust.displayFiatAmount(amount: fiatAmount.amount)) |
There was a problem hiding this comment.
When metadata.fiatOrBtc == .btc, the secondary amount is built with displayFiatAmount, which formats the absolute fiat value without any sign (see display_fiat_amount in rust/src/manager/wallet_manager.rs). That means outgoing transactions will show a positive fiat secondary line even though the primary BTC line is negative, which is misleading for spends. Consider adding a - prefix when txn.sentAndReceived().direction() == .outgoing (this same pattern is repeated in the unconfirmed/unsigned views).
Useful? React with 👍 / 👎.
Greptile OverviewGreptile SummaryThis PR enhances transaction list UX by replacing block height with complementary amount (fiat when in BTC mode, BTC when in fiat mode) and improves error handling for unsigned transaction deletion. Key Changes:
Implementation Quality:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Transaction UI<br/>(Android/iOS)
participant WM as WalletManager
participant Rust as RustWalletManager
Note over UI,Rust: Displaying Transaction Row
UI->>UI: Determine fiatOrBtc mode
alt Primary Amount (BTC mode)
UI->>Rust: displaySentAndReceivedAmount(sentAndReceived)
Rust->>Rust: Check sensitive_visible
Rust->>Rust: Apply direction prefix (- for outgoing)
Rust-->>UI: "-0.00050000 BTC"
else Primary Amount (Fiat mode)
UI->>Rust: displayFiatAmountWithDirection(amount, direction)
Rust->>Rust: Check sensitive_visible
Rust->>Rust: Apply direction prefix (- for outgoing)
Rust-->>UI: "-$50.00"
end
alt Secondary Amount (BTC mode → show fiat)
UI->>Rust: displayFiatAmountWithDirection(fiatAmount, direction)
Rust-->>UI: "-$50.00"
else Secondary Amount (Fiat mode → show BTC)
UI->>Rust: displaySentAndReceivedAmount(sentAndReceived)
Rust-->>UI: "-0.00050000 BTC"
end
UI->>UI: Render primary + secondary amounts
Note over UI,Rust: Delete Unsigned Transaction
UI->>UI: User long-press/context menu
UI->>Rust: deleteUnsignedTransaction(txId)
alt Success
Rust-->>UI: Transaction deleted
UI->>UI: Refresh list
else Error
Rust-->>UI: Exception
UI->>WM: Set alertState
UI->>UI: Show error alert to user
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+693
to
+710
| val secondaryAmount = | ||
| manager?.let { | ||
| when (fiatOrBtc) { | ||
| FiatOrBtc.BTC -> { | ||
| // primary is BTC, secondary is fiat | ||
| val amount = fiatAmount | ||
| if (amount != null) { | ||
| it.rust.displayFiatAmount(amount) | ||
| } else { | ||
| "---" | ||
| } | ||
| } | ||
| FiatOrBtc.FIAT -> { | ||
| // primary is fiat, secondary is BTC | ||
| it.displayAmount(txn.spendingAmount(), showUnit = true) | ||
| } | ||
| } | ||
| } ?: "---" |
There was a problem hiding this comment.
Unsigned transactions don't add prefix (+ or -) to secondaryAmount, while confirmed and unconfirmed transactions do (lines 318, 392). This creates inconsistent display behavior.
Suggested change
| val secondaryAmount = | |
| manager?.let { | |
| when (fiatOrBtc) { | |
| FiatOrBtc.BTC -> { | |
| // primary is BTC, secondary is fiat | |
| val amount = fiatAmount | |
| if (amount != null) { | |
| it.rust.displayFiatAmount(amount) | |
| } else { | |
| "---" | |
| } | |
| } | |
| FiatOrBtc.FIAT -> { | |
| // primary is fiat, secondary is BTC | |
| it.displayAmount(txn.spendingAmount(), showUnit = true) | |
| } | |
| } | |
| } ?: "---" | |
| val secondaryAmount = | |
| manager?.let { | |
| val prefix = "-" | |
| when (fiatOrBtc) { | |
| FiatOrBtc.BTC -> { | |
| // primary is BTC, secondary is fiat | |
| val amount = fiatAmount | |
| if (amount != null) { | |
| prefix + it.rust.displayFiatAmount(amount) | |
| } else { | |
| "---" | |
| } | |
| } | |
| FiatOrBtc.FIAT -> { | |
| // primary is fiat, secondary is BTC | |
| prefix + it.displayAmount(txn.spendingAmount(), showUnit = true) | |
| } | |
| } | |
| } ?: "---" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionsCardView.kt
Line: 693:710
Comment:
Unsigned transactions don't add prefix (+ or -) to `secondaryAmount`, while confirmed and unconfirmed transactions do (lines 318, 392). This creates inconsistent display behavior.
```suggestion
val secondaryAmount =
manager?.let {
val prefix = "-"
when (fiatOrBtc) {
FiatOrBtc.BTC -> {
// primary is BTC, secondary is fiat
val amount = fiatAmount
if (amount != null) {
prefix + it.rust.displayFiatAmount(amount)
} else {
"---"
}
}
FiatOrBtc.FIAT -> {
// primary is fiat, secondary is BTC
prefix + it.displayAmount(txn.spendingAmount(), showUnit = true)
}
}
} ?: "---"
```
How can I resolve this? If you propose a fix, please make it concise.
praveenperera
force-pushed
the
fiat-value-wallet-screen
branch
from
February 3, 2026 21:02
87ef208 to
7364836
Compare
Member
Author
|
@greptile-apps re-review |
Comment on lines
+318
to
+331
| val prefix = if (direction == TransactionDirection.OUTGOING) "-" else "" | ||
| when (fiatOrBtc) { | ||
| FiatOrBtc.BTC -> { | ||
| // primary is BTC, secondary is fiat | ||
| val fiatAmount = txn.v1.fiatAmount() | ||
| if (fiatAmount != null) { | ||
| prefix + it.rust.displayFiatAmount(fiatAmount.amount) | ||
| } else { | ||
| "---" | ||
| } | ||
| } | ||
| FiatOrBtc.FIAT -> { | ||
| // primary is fiat, secondary is BTC | ||
| prefix + it.displayAmount(amount, showUnit = true) |
There was a problem hiding this comment.
Manual prefix addition causes double-prefix when displaying amounts. The displayAmount() method is meant for raw amounts without direction indicators, but here you're adding a prefix. In iOS (lines 157-159), displaySentAndReceivedAmount() is used which already includes the prefix via Rust's SentAndReceived::amount_fmt(). This inconsistency means Android adds prefix manually while iOS relies on the Rust method.
Suggested change
| val prefix = if (direction == TransactionDirection.OUTGOING) "-" else "" | |
| when (fiatOrBtc) { | |
| FiatOrBtc.BTC -> { | |
| // primary is BTC, secondary is fiat | |
| val fiatAmount = txn.v1.fiatAmount() | |
| if (fiatAmount != null) { | |
| prefix + it.rust.displayFiatAmount(fiatAmount.amount) | |
| } else { | |
| "---" | |
| } | |
| } | |
| FiatOrBtc.FIAT -> { | |
| // primary is fiat, secondary is BTC | |
| prefix + it.displayAmount(amount, showUnit = true) | |
| when (fiatOrBtc) { | |
| FiatOrBtc.BTC -> { | |
| // primary is BTC, secondary is fiat | |
| val fiatAmount = txn.v1.fiatAmount() | |
| if (fiatAmount != null) { | |
| prefix + it.rust.displayFiatAmount(fiatAmount.amount) | |
| } else { | |
| "---" | |
| } | |
| } | |
| FiatOrBtc.FIAT -> { | |
| // primary is fiat, secondary is BTC | |
| it.rust.displaySentAndReceivedAmount(txn.v1.sentAndReceived()) | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionsCardView.kt
Line: 318:331
Comment:
Manual prefix addition causes double-prefix when displaying amounts. The `displayAmount()` method is meant for raw amounts without direction indicators, but here you're adding a prefix. In iOS (lines 157-159), `displaySentAndReceivedAmount()` is used which already includes the prefix via Rust's `SentAndReceived::amount_fmt()`. This inconsistency means Android adds prefix manually while iOS relies on the Rust method.
```suggestion
when (fiatOrBtc) {
FiatOrBtc.BTC -> {
// primary is BTC, secondary is fiat
val fiatAmount = txn.v1.fiatAmount()
if (fiatAmount != null) {
prefix + it.rust.displayFiatAmount(fiatAmount.amount)
} else {
"---"
}
}
FiatOrBtc.FIAT -> {
// primary is fiat, secondary is BTC
it.rust.displaySentAndReceivedAmount(txn.v1.sentAndReceived())
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+392
to
+405
| val prefix = if (direction == TransactionDirection.OUTGOING) "-" else "" | ||
| when (fiatOrBtc) { | ||
| FiatOrBtc.BTC -> { | ||
| // primary is BTC, secondary is fiat | ||
| val fiatAmount = txn.v1.fiatAmount() | ||
| if (fiatAmount != null) { | ||
| prefix + it.rust.displayFiatAmount(fiatAmount.amount) | ||
| } else { | ||
| "---" | ||
| } | ||
| } | ||
| FiatOrBtc.FIAT -> { | ||
| // primary is fiat, secondary is BTC | ||
| prefix + it.displayAmount(amount, showUnit = true) |
There was a problem hiding this comment.
Same issue as confirmed transactions - use displaySentAndReceivedAmount() for consistency with iOS.
Suggested change
| val prefix = if (direction == TransactionDirection.OUTGOING) "-" else "" | |
| when (fiatOrBtc) { | |
| FiatOrBtc.BTC -> { | |
| // primary is BTC, secondary is fiat | |
| val fiatAmount = txn.v1.fiatAmount() | |
| if (fiatAmount != null) { | |
| prefix + it.rust.displayFiatAmount(fiatAmount.amount) | |
| } else { | |
| "---" | |
| } | |
| } | |
| FiatOrBtc.FIAT -> { | |
| // primary is fiat, secondary is BTC | |
| prefix + it.displayAmount(amount, showUnit = true) | |
| when (fiatOrBtc) { | |
| FiatOrBtc.BTC -> { | |
| // primary is BTC, secondary is fiat | |
| val fiatAmount = txn.v1.fiatAmount() | |
| if (fiatAmount != null) { | |
| prefix + it.rust.displayFiatAmount(fiatAmount.amount) | |
| } else { | |
| "---" | |
| } | |
| } | |
| FiatOrBtc.FIAT -> { | |
| // primary is fiat, secondary is BTC | |
| it.rust.displaySentAndReceivedAmount(txn.v1.sentAndReceived()) | |
| } | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionsCardView.kt
Line: 392:405
Comment:
Same issue as confirmed transactions - use `displaySentAndReceivedAmount()` for consistency with iOS.
```suggestion
when (fiatOrBtc) {
FiatOrBtc.BTC -> {
// primary is BTC, secondary is fiat
val fiatAmount = txn.v1.fiatAmount()
if (fiatAmount != null) {
prefix + it.rust.displayFiatAmount(fiatAmount.amount)
} else {
"---"
}
}
FiatOrBtc.FIAT -> {
// primary is fiat, secondary is BTC
it.rust.displaySentAndReceivedAmount(txn.v1.sentAndReceived())
}
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Display fiat amount when in BTC mode, and BTC/sats amount when in fiat mode as the secondary line in transaction rows on both platforms
praveenperera
force-pushed
the
fiat-value-wallet-screen
branch
from
February 4, 2026 02:36
7364836 to
d14183a
Compare
Member
Author
|
@greptile-apps re-review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ios/Cove/Flows/SelectedWalletFlow/TransactionsCardView.swift`:
- Around line 355-364: The secondaryAmount getter currently formats amounts
without a minus for unsigned (always outgoing) transactions; update it to prefix
"-" for unsigned transactions so it matches Android. In the secondaryAmount
logic (property name secondaryAmount) check the unsigned flag on the transaction
metadata (e.g., metadata.isUnsigned or equivalent field on metadata/txn) and,
when true, prepend "-" to the string returned by
privateShow(manager.rust.displayFiatAmount(amount: fiatAmount)) for the fiat
branch and to privateShow(manager.amountFmtUnit(txn.spendingAmount())) for the
fiat-primary branch; ensure you only add the prefix when unsigned and preserve
existing privateShow wrapping and formatting calls.
🧹 Nitpick comments (1)
android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionsCardView.kt (1)
298-331: Consider extracting duplicated amount formatting logic.The
formattedAmountandsecondaryAmountcomputation for confirmed transactions (lines 298-331) is nearly identical to unconfirmed transactions (lines 369-402). This could be extracted to a helper function to reduce duplication.Suggested helper function
private data class FormattedAmounts(val primary: String, val secondary: String) `@Composable` private fun formatTransactionAmounts( direction: TransactionDirection, fiatOrBtc: FiatOrBtc, fiatAmount: FiatAmount?, sentAndReceived: SentAndReceived, manager: WalletManager?, ): FormattedAmounts { val fiatPrefix = if (direction == TransactionDirection.OUTGOING) "-" else "" val primary = manager?.let { when (fiatOrBtc) { FiatOrBtc.BTC -> it.rust.displaySentAndReceivedAmount(sentAndReceived) FiatOrBtc.FIAT -> fiatAmount?.let { fa -> fiatPrefix + it.rust.displayFiatAmount(fa.amount) } ?: "---" } } ?: sentAndReceived.label() val secondary = manager?.let { when (fiatOrBtc) { FiatOrBtc.BTC -> fiatAmount?.let { fa -> fiatPrefix + it.rust.displayFiatAmount(fa.amount) } ?: "---" FiatOrBtc.FIAT -> it.rust.displaySentAndReceivedAmount(sentAndReceived) } } ?: "---" return FormattedAmounts(primary, secondary) }Also applies to: 369-402
| if case .btc = metadata.fiatOrBtc { | ||
| // primary is BTC, secondary is fiat | ||
| guard let fiatAmount else { return privateShow("---") } | ||
| return privateShow(manager.rust.displayFiatAmount(amount: fiatAmount)) |
There was a problem hiding this comment.
secondary fiat amounts don't include "-" prefix, creating inconsistency with Android (line 694)
Suggested change
| return privateShow(manager.rust.displayFiatAmount(amount: fiatAmount)) | |
| return privateShow("-" + manager.rust.displayFiatAmount(amount: fiatAmount)) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/Cove/Flows/SelectedWalletFlow/TransactionsCardView.swift
Line: 359:359
Comment:
secondary fiat amounts don't include "-" prefix, creating inconsistency with Android (line 694)
```suggestion
return privateShow("-" + manager.rust.displayFiatAmount(amount: fiatAmount))
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: ios/Cove/Flows/SelectedWalletFlow/TransactionsCardView.swift
Line: 347:347
Comment:
unsigned transactions don't include "-" prefix for outgoing amounts, unlike Android (line 675) which explicitly adds it
```suggestion
return privateShow("-" + manager.amountFmtUnit(txn.spendingAmount()))
```
How can I resolve this? If you propose a fix, please make it concise. |
Move fiat prefix logic ("-" for outgoing) from iOS/Android into Rust,
mirroring how displaySentAndReceivedAmount handles BTC amounts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rust/src/manager/wallet_manager.rs`:
- Around line 723-739: The function display_fiat_amount_with_direction currently
prepends a "-" for outgoing transactions even when display_fiat_amount returns
the masked sensitive string (e.g., "**************"), producing
"-**************"; change it to check the output of display_fiat_amount (or the
sensitive_visible state) and only prepend the direction prefix when the
formatted result is not the masked/sensitive placeholder so masked outputs
remain unprefixed like display_sent_and_received_amount; reference
display_fiat_amount_with_direction and display_fiat_amount and implement the
conditional check to skip adding the prefix when the returned string starts with
"*" or equals the known mask.
Member
Author
|
@greptile-apps re-review |
Unsigned transactions were manually adding "-" prefix for BTC amounts while fiat used the Rust method. This adds a matching Rust method for BTC amounts to keep formatting consistent across both platforms.
Replace silent try? with proper error handling that logs the error and shows an alert to the user on both iOS and Android.
praveenperera
force-pushed
the
fiat-value-wallet-screen
branch
from
February 4, 2026 15:32
fbc702e to
00988d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/TransactionsCardView.kt`:
- Around line 811-819: Replace the hardcoded delete-failure strings in
TransactionsCardView (where app?.let { it.alertState =
TaggedItem(AppAlertState.General(...)) }) with localized string resources: add
delete_failed_title, delete_failed_message, and unknown_error to
res/values/strings.xml and use context.getString(R.string.delete_failed_title)
and context.getString(R.string.delete_failed_message, errorText) (formatting the
message) and fallback to context.getString(R.string.unknown_error) for the
unknown case; update the TaggedItem/AppAlertState.General instantiation to
reference these string resources instead of the literal text.
🧹 Nitpick comments (1)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (1)
18157-18182: Clarify KDoc to reference Kotlin method names and new direction helpers.
The doc currently points to the snake_case Rust name, which can mislead Kotlin callers. Consider updating both the interface and override KDocs to reference the Kotlin APIs.✏️ Suggested KDoc update (apply to both interface + override)
- * Does NOT include direction prefix - use `display_sent_and_received_amount` - * for transaction amounts that need +/- indicators. + * Does NOT include direction prefix; for transaction amounts that need +/- indicators, + * use `displayAmountWithDirection`, `displayFiatAmountWithDirection`, + * or `displaySentAndReceivedAmount`.Also applies to: 18621-18627
Member
Author
|
@greptile-apps re-review |
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
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.
Display fiat amount when in BTC mode, and BTC/sats amount when in fiat mode as the secondary line in transaction rows on both platforms
Summary by CodeRabbit
New Features
Bug Fixes