Add Zano alias resolution to send flow#9393
Conversation
📝 WalkthroughWalkthroughAdds Zano alias normalization and JSON-RPC address resolution, caches resolved addresses through a new address handler, and wires that handler into Zano address parsing with coverage for valid and invalid inputs. ChangesZano alias support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant EnterAddressViewModel
participant AddressParserChain
participant AddressHandlerZanoAlias
participant ZanoAliasResolver
participant ZanoNode
EnterAddressViewModel->>AddressParserChain: create Zano parser chain
AddressParserChain->>AddressHandlerZanoAlias: isSupported(alias)
AddressHandlerZanoAlias->>ZanoAliasResolver: resolve(normalized alias)
ZanoAliasResolver->>ZanoNode: JSON-RPC alias lookup
ZanoNode-->>ZanoAliasResolver: alias response
ZanoAliasResolver-->>AddressHandlerZanoAlias: address or null
AddressHandlerZanoAlias-->>AddressParserChain: support result and cached address
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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)
walletkit/src/main/java/io/horizontalsystems/walletkit/core/address/ZanoAliasResolver.kt (1)
38-46: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffSynchronous network call can cause thread starvation.
This executes a blocking network request. When called from a coroutine on
Dispatchers.Default(as it is viaEnterAddressViewModel->AddressParserChain), it blocks a CPU-bound thread, which can lead to thread starvation.Consider refactoring the
IAddressHandlercontract to supportsuspendfunctions for address resolution, allowing this to useawait()orwithContext(Dispatchers.IO).🤖 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 `@walletkit/src/main/java/io/horizontalsystems/walletkit/core/address/ZanoAliasResolver.kt` around lines 38 - 46, Refactor the IAddressHandler address-resolution contract and its implementations, including ZanoAliasResolver, to use suspend functions. Update AddressParserChain and EnterAddressViewModel call sites to propagate suspension, and execute the blocking OkHttp request in ZanoAliasResolver with withContext(Dispatchers.IO) (or an equivalent async await approach) so Dispatchers.Default is not blocked.
🤖 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
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/address/IAddressHandler.kt`:
- Line 366: Replace the mutableMapOf-backed cache in the address handler with a
ConcurrentHashMap, preserving the existing String-to-Address cache behavior and
access patterns.
---
Nitpick comments:
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/core/address/ZanoAliasResolver.kt`:
- Around line 38-46: Refactor the IAddressHandler address-resolution contract
and its implementations, including ZanoAliasResolver, to use suspend functions.
Update AddressParserChain and EnterAddressViewModel call sites to propagate
suspension, and execute the blocking OkHttp request in ZanoAliasResolver with
withContext(Dispatchers.IO) (or an equivalent async await approach) so
Dispatchers.Default is not blocked.
🪄 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 Plus
Run ID: 63a60ee3-9464-44b7-9908-852adea4e127
📒 Files selected for processing (5)
walletkit/src/main/java/io/horizontalsystems/walletkit/core/address/ZanoAliasResolver.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/address/AddressHandlerFactory.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/address/IAddressHandler.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/enteraddress/EnterAddressViewModel.ktwalletkit/src/test/java/io/horizontalsystems/walletkit/modules/address/ZanoAliasTest.kt
|
|
||
| class AddressHandlerZanoAlias(private val resolver: ZanoAliasResolver) : IAddressHandler { | ||
| override val blockchainType = BlockchainType.Zano | ||
| private val cache = mutableMapOf<String, Address>() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Data race on non-thread-safe map.
Since isSupported makes a blocking network call that doesn't cooperatively check for coroutine cancellation, typing quickly can spawn multiple coroutines that block simultaneously. When these network calls complete, they will write to this map concurrently from different threads on Dispatchers.Default, which can cause a ConcurrentModificationException or corrupt the map.
Use a ConcurrentHashMap to ensure thread safety.
🛠️ Proposed fix to ensure thread safety
- private val cache = mutableMapOf<String, Address>()
+ private val cache = java.util.concurrent.ConcurrentHashMap<String, Address>()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private val cache = mutableMapOf<String, Address>() | |
| private val cache = java.util.concurrent.ConcurrentHashMap<String, Address>() |
🤖 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
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/address/IAddressHandler.kt`
at line 366, Replace the mutableMapOf-backed cache in the address handler with a
ConcurrentHashMap, preserving the existing String-to-Address cache behavior and
access patterns.
Summary by CodeRabbit
New Features
@.Bug Fixes