Compare Monero node endpoints when checking duplicates#9382
Conversation
Add validation to ensure that manually entered Monero node URLs include a port number. If a port is missing, an error message is displayed to the user. - Update `AddMoneroNodeViewModel` to validate the presence of a port in the RPC URL. - Update `AddMoneroNodeScreen` input hint to show the expected format (e.g., `https://node.com:port`). - Add a fallback to port 443 in `MoneroNodeManager` for nodes saved before this validation was implemented. - Add `AddMoneroNode_Error_PortRequired` string resource.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMonero node endpoints now normalize unspecified ports to 443 for serialization and duplicate checks. Adding a node requires an explicit port, with updated input guidance and a dedicated validation message. ChangesMonero node validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 4
🤖 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/core/managers/MoneroNodeManager.kt`:
- Around line 157-160: Update endpoint() to lowercase uri.host before composing
the canonical host-and-port string, ensuring hostnames with different input
casing produce the same endpoint key while preserving the existing fallback for
invalid or hostless inputs.
- Around line 162-164: Rename the local String variable in hasNode so it no
longer shadows the endpoint helper function, then compare endpoint(it.host)
against the renamed value while preserving the existing node-matching behavior.
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/moneronetwork/addnode/AddMoneroNodeScreen.kt`:
- Line 65: Move the hardcoded URL hint from the add-node screen into a localized
string resource, then update the hint assignment in AddMoneroNodeScreen to use
stringResource(...) with that resource. Preserve the displayed text as
“https://node.com:port”.
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/moneronetwork/addnode/AddMoneroNodeViewModel.kt`:
- Around line 61-65: Update the port validation in the AddMoneroNodeViewModel
flow to accept only values in the range 1..65535, rejecting both the
missing-port sentinel and port 0. Change the Caution message to use the “valid
port” wording while preserving the existing error state emission and early
return.
🪄 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: 049b4baf-3fbc-4761-94f2-7573703b41d6
⛔ Files ignored due to path filters (1)
walletkit/src/main/res/values-fa/strings.xmlis excluded by!**/res/values-*/strings.xml
📒 Files selected for processing (4)
walletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/MoneroNodeManager.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/moneronetwork/addnode/AddMoneroNodeScreen.ktwalletkit/src/main/java/io/horizontalsystems/walletkit/modules/moneronetwork/addnode/AddMoneroNodeViewModel.ktwalletkit/src/main/res/values/strings.xml
| private fun endpoint(hostOrUrl: String): String { | ||
| val uri = hostOrUrl.toUri() | ||
| return uri.host?.let { "$it:${effectivePort(uri)}" } ?: hostOrUrl | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -n -C 3 'fun endpoint|toUri\(\)|hasNode' walletkit --glob '*Test.kt' --glob '*Spec.kt' || trueRepository: horizontalsystems/unstoppable-wallet-android
Length of output: 182
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the manager implementation and any duplicate-check logic/tests.
git ls-files 'walletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/MoneroNodeManager.kt' \
'walletkit/src/test/**' 'walletkit/src/androidTest/**' | sed -n '1,200p'
echo '--- MoneroNodeManager outline ---'
ast-grep outline walletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/MoneroNodeManager.kt --view expanded || true
echo '--- Relevant lines ---'
nl -ba walletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/MoneroNodeManager.kt | sed -n '1,260p'
echo '--- Search for endpoint / duplicate logic ---'
rg -n -C 3 'endpoint\(|hasNode|duplicate|node.*key|toUri\(' walletkit/src/main/java walletkit/src/test walletkit/src/androidTest || trueRepository: horizontalsystems/unstoppable-wallet-android
Length of output: 1859
🌐 Web query:
Android Uri host lowercase preserves casing Uri.host case normalization
💡 Result:
The Android android.net.Uri class does not automatically perform case normalization (lowercasing) on the host component when using getHost [1][2][3]. The method returns the encoded host as it appears in the URI's authority component, preserving the original casing [2][4][5]. Key details: - Preservation: If the URI string contains "Example.com", getHost will return "Example.com" [3][5]. - Implementation: The getHost method parses the authority component and extracts the host string, applying decoding if necessary, but it does not modify the casing of the returned string [3][4]. - Comparison: While getHost preserves case, developers should be aware that the Uri class's equals method is case-sensitive, meaning two URIs with different casing in their host components will not be considered equal [2][6]. - Normalization: Because the class is designed to be "very forgiving" and performs little validation, it generally does not attempt to normalize components like the host [1][6][3]. If case-insensitive handling of hostnames is required for your application, you must manually convert the string returned by getHost to lowercase using standard Java methods (e.g., host.toLowerCase(Locale.US)) [2][3].
Citations:
- 1: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/net/Uri.java
- 2: https://developer.android.com/reference/kotlin/android/net/Uri
- 3: https://android.googlesource.com/platform/frameworks/base/+/c53962d/core/java/android/net/Uri.java
- 4: https://android.googlesource.com/platform/prebuilts/fullsdk/sources/+/refs/heads/androidx-xr-arcore-release/android-34/android/net/Uri.java
- 5: https://github.com/android/platform_frameworks_base/blob/master/core/java/android/net/Uri.java
- 6: https://devdoc.net/android/Android-r19/reference/android/net/Uri.html
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('walletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/MoneroNodeManager.kt')
lines = p.read_text().splitlines()
for start, end in [(145, 175), (175, 205)]:
print(f'লines {start}-{end}')
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
print()
PY
echo '--- tests mentioning MoneroNodeManager ---'
rg -n -C 3 'MoneroNodeManager|hasNode\(|addMoneroNode\(|endpoint\(' walletkit/src/test walletkit/src/androidTest || trueRepository: horizontalsystems/unstoppable-wallet-android
Length of output: 2738
Normalize hostname casing in endpoint().
uri.host preserves the input casing, so https://Node.Example:18081 and https://node.example:18081 can produce different duplicate-check keys. Lowercase the host before composing the canonical endpoint.
🤖 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/managers/MoneroNodeManager.kt`
around lines 157 - 160, Update endpoint() to lowercase uri.host before composing
the canonical host-and-port string, ensuring hostnames with different input
casing produce the same endpoint key while preserving the existing fallback for
invalid or hostless inputs.
| fun hasNode(url: String): Boolean { | ||
| val endpoint = endpoint(url) | ||
| return allNodes.any { endpoint(it.host) == endpoint } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -n -C 2 'val endpoint = endpoint|endpoint\(it\.host\)' walletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/MoneroNodeManager.ktRepository: horizontalsystems/unstoppable-wallet-android
Length of output: 353
🏁 Script executed:
sed -n '1,260p' walletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/MoneroNodeManager.ktRepository: horizontalsystems/unstoppable-wallet-android
Length of output: 8469
🏁 Script executed:
python3 - <<'PY'
import shutil, subprocess, textwrap, tempfile, os, sys
print("kotlinc:", shutil.which("kotlinc"))
print("kotlin:", shutil.which("kotlin"))
code = textwrap.dedent("""
fun endpoint(x: String): String = x
fun test(url: String, items: List<String>): Boolean {
val endpoint = endpoint(url)
return items.any { endpoint(it) == endpoint }
}
""")
with tempfile.TemporaryDirectory() as d:
path = os.path.join(d, "Test.kt")
with open(path, "w") as f:
f.write(code)
if shutil.which("kotlinc"):
p = subprocess.run(["kotlinc", path, "-d", os.path.join(d, "out.jar")], capture_output=True, text=True)
print("kotlinc exit:", p.returncode)
print("stdout:\n", p.stdout)
print("stderr:\n", p.stderr)
else:
print("kotlinc not available")
PYRepository: horizontalsystems/unstoppable-wallet-android
Length of output: 231
🌐 Web query:
Kotlin name resolution local variable shadows function call same name compiler error official docs
💡 Result:
In Kotlin, it is not a compiler error to declare a local variable that has the same name as a function [1]. Kotlin allows this because local declarations are permitted to shadow declarations from outer scopes [2][3]. When a local variable and a function share the same name in the same scope, the local variable takes precedence (shadows the function) [4][5][2]. This is a standard part of Kotlin's name resolution rules, where "local declaration wins" [4]. If you declare a local variable with the same name as a function in the same scope: 1. The compiler will resolve the name to the local variable [4][5]. 2. If you then attempt to call that name as a function (e.g., name), the compiler will attempt to resolve the property/variable as a function-like value using the invoke convention [4][5]. 3. If the local variable is not a function type (i.e., it does not have an invoke operator), the compiler will report an error because the variable shadows the function, and the variable itself cannot be invoked [5]. To access the shadowed function, you cannot simply use the name directly because it is hidden by the local variable [2]. Instead, you must use a fully qualified name (if applicable) or rename one of the declarations to avoid the collision [2].
Citations:
- 1: https://stackoverflow.com/questions/55623857/why-are-there-two-functions-which-do-the-exact-same-thing-why-must-be-invoked-d
- 2: https://gorkemkara.net/kotlin-shadowing-fully-qualified-names/
- 3: https://www.baeldung.com/kotlin/variable-shadowing
- 4: https://github.com/JetBrains/kotlin/blob/master/spec-docs/NameResolution.adoc
- 5: https://fossies.org/linux/kotlin/spec-docs/name-resolution-motivation.md
Rename the local endpoint variable
endpoint(it.host) is resolved against the local String, so this call is invalid. Rename the local value and keep the helper callable.
🤖 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/managers/MoneroNodeManager.kt`
around lines 162 - 164, Rename the local String variable in hasNode so it no
longer shadows the endpoint helper function, then compare endpoint(it.host)
against the renamed value while preserving the existing node-matching behavior.
| qrScannerEnabled = true, | ||
| onValueChange = viewModel::onEnterRpcUrl, | ||
| hint = "", | ||
| hint = "https://node.com:port", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the URL hint into a string resource.
This hardcoded visible string bypasses localization. Add a resource and reference it with stringResource(...).
Proposed fix
- hint = "https://node.com:port",
+ hint = stringResource(R.string.AddMoneroNode_UrlHint),<string name="AddMoneroNode_UrlHint">https://node.com:port</string>📝 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.
| hint = "https://node.com:port", | |
| hint = stringResource(R.string.AddMoneroNode_UrlHint), |
🤖 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/moneronetwork/addnode/AddMoneroNodeScreen.kt`
at line 65, Move the hardcoded URL hint from the add-node screen into a
localized string resource, then update the hint assignment in
AddMoneroNodeScreen to use stringResource(...) with that resource. Preserve the
displayed text as “https://node.com:port”.
| if (sourceUri.port == -1) { | ||
| urlCaution = Caution(Translator.getString(R.string.AddMoneroNode_Error_PortRequired), Caution.Type.Error) | ||
| emitState() | ||
| return | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the cited lines and identify sourceUri type/imports.
FILE='walletkit/src/main/java/io/horizontalsystems/walletkit/modules/moneronetwork/addnode/AddMoneroNodeViewModel.kt'
echo '--- file outline ---'
ast-grep outline "$FILE" --view expanded || true
echo
echo '--- relevant lines ---'
sed -n '1,140p' "$FILE" | cat -n
echo
echo '--- search for sourceUri declarations/usages ---'
rg -n "sourceUri|URI\.port|port == -1|port !in" "$FILE" -n -C 3 || true
echo
echo '--- search for Uri/URI imports in file ---'
rg -n "import .*Uri|import .*URI" "$FILE" || trueRepository: horizontalsystems/unstoppable-wallet-android
Length of output: 4824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe URI semantics in the runtime available in the sandbox.
python3 - <<'PY'
from urllib.parse import urlparse
tests = [
"https://node.example:0",
"https://node.example",
"https://node.example:443",
]
for t in tests:
u = urlparse(t)
print(t, "-> port attr?", u.port, "netloc", u.netloc, "hostname", u.hostname)
PYRepository: horizontalsystems/unstoppable-wallet-android
Length of output: 436
Reject port 0 here. sourceUri.port == -1 still accepts https://...:0, so an unusable node can be saved. Require 1..65535 and update the message to say “valid port.”
🤖 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/moneronetwork/addnode/AddMoneroNodeViewModel.kt`
around lines 61 - 65, Update the port validation in the AddMoneroNodeViewModel
flow to accept only values in the range 1..65535, rejecting both the
missing-port sentinel and port 0. Change the Caution message to use the “valid
port” wording while preserving the existing error state emission and early
return.
1 participant
#8358
Summary by CodeRabbit
New Features
Bug Fixes