Tron: broadcast fails with NOT_ENOUGH_EFFECTIVE_CONNECTION; status lookup also logs noisy deserialization error (#4175)#4179
Conversation
…us lookup also logs noisy deserialization error (#4175)
📝 WalkthroughWalkthroughThe TronApi module adds automatic retry logic with exponential backoff when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
data/src/main/kotlin/com/vultisig/wallet/data/api/TronApi.kt (1)
67-87: Retry logic — a few smaller points worth considering.
Interface/impl return type drift. The interface declares
broadcastTransaction(tx: String): String?(line 39) but the implementation declaresString(line 66). Kotlin accepts this covariant override, but now no caller can observe anullresult, and the nullability on the interface is misleading. Consider tightening the interface toStringas well.Raw
Exception. Per coding guidelines, prefer a typed exception overthrow Exception(...). A sealed class or dedicatedIOException/domain exception also lets callers distinguish "retries exhausted" from "hard failure" (useful sincegetTransactionStatus/ retry-pending flows may want different UX).Backoff cap.
1000L shl attemptis fine forMAX_BROADCAST_RETRIES = 3(waits 1s, then 2s), but if anyone later bumps the constant,shlon aLongwill overflow around attempt 63 and the delays will grow unexpectedly. AcoerceAtMost(MAX_BACKOFF_MS)guard is cheap insurance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/main/kotlin/com/vultisig/wallet/data/api/TronApi.kt` around lines 67 - 87, The implementation of broadcastTransaction contains three issues: the interface declares broadcastTransaction(tx: String): String? while the implementation returns non-null String (tighten the interface to String to match the impl), it throws raw Exception(...) (replace with a typed exception or sealed domain error, e.g., BroadcastFailure or IOException so callers can distinguish retry-exhausted vs hard failure), and the exponential backoff uses 1000L shl attempt which can overflow (wrap the delay calculation with a cap like delayMs = (1000L shl attempt).coerceAtMost(MAX_BACKOFF_MS) and then delay(delayMs)); update references in the same function and constants MAX_BROADCAST_RETRIES, NOT_ENOUGH_EFFECTIVE_CONNECTION_ERROR_CODE, DUP_TRANSACTION_ERROR_CODE, and any callers such as getTransactionStatus to handle the new exception type and tightened return nullability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/src/main/kotlin/com/vultisig/wallet/data/api/TronApi.kt`:
- Around line 84-86: The two hardcoded throw strings in TronApi.kt must be
replaced with typed exceptions so localization happens at the UI layer: create a
sealed exception (e.g. sealed class TronBroadcastException : Exception()) with
variants like BroadcastFailed(val code: Int) and NetworkUnstable, then change
the throws inside the broadcast method (the places currently throwing "Error
broadcasting transaction: ${response.code}" and "Tron network is unstable.
Please try again shortly.") to throw BroadcastFailed(response.code) and
NetworkUnstable respectively; finally ensure callers (ViewModel/UseCase) catch
TronBroadcastException and map each variant to the appropriate string resource
(e.g. R.string.tron_broadcast_failed, R.string.tron_broadcast_network_unstable)
before presenting to the user.
---
Nitpick comments:
In `@data/src/main/kotlin/com/vultisig/wallet/data/api/TronApi.kt`:
- Around line 67-87: The implementation of broadcastTransaction contains three
issues: the interface declares broadcastTransaction(tx: String): String? while
the implementation returns non-null String (tighten the interface to String to
match the impl), it throws raw Exception(...) (replace with a typed exception or
sealed domain error, e.g., BroadcastFailure or IOException so callers can
distinguish retry-exhausted vs hard failure), and the exponential backoff uses
1000L shl attempt which can overflow (wrap the delay calculation with a cap like
delayMs = (1000L shl attempt).coerceAtMost(MAX_BACKOFF_MS) and then
delay(delayMs)); update references in the same function and constants
MAX_BROADCAST_RETRIES, NOT_ENOUGH_EFFECTIVE_CONNECTION_ERROR_CODE,
DUP_TRANSACTION_ERROR_CODE, and any callers such as getTransactionStatus to
handle the new exception type and tightened return nullability.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e8dc6e2-63c5-4976-812b-312c8e1f1319
📒 Files selected for processing (2)
data/src/main/kotlin/com/vultisig/wallet/data/api/TronApi.ktdata/src/main/kotlin/com/vultisig/wallet/data/api/models/TronResponseJson.kt
Fixes #4175
Changes
TronApiImpl.broadcastTransactionnow retries up to 3 times (MAX_BROADCAST_RETRIES) with exponential backoff (delay(1000L shl attempt)) when the Tron network returnsNOT_ENOUGH_EFFECTIVE_CONNECTION; previously it was a single attempt with no retry logic.TronApiImpl.getTransactionStatusadds.takeIf { it.txId != null }to filter out malformed/incomplete responses, returningnullinstead of a partially-deserialized object.TronTransactionStatusResponse.txIdinTronResponseJson.ktchanged from non-nullableStringtoString? = null, preventing deserialization crashes when the field is absent in the API response.Checklist
Summary by CodeRabbit