Require port for Monero node URLs#9386
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.
Passing base64-encoded contents of large `strings.xml` files as command-line arguments to `jq` can exceed the kernel's 128KB per-argument limit (`MAX_ARG_STRLEN`), causing the workflow to fail. Update the script to write encoded file contents to temporary files and use `jq`'s `--rawfile` and `--slurpfile` flags to process the data, avoiding the argument length restriction.
📝 WalkthroughWalkthroughThe translation workflow now constructs GraphQL file additions from temporary JSONL data. Monero node handling normalizes endpoints, requires explicit ports during addition, updates localized validation text, and displays a URL example. ChangesTranslation commit payload
Monero node validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AddMoneroNodeScreen
participant AddMoneroNodeViewModel
participant MoneroNodeManager
participant LocalizationResources
User->>AddMoneroNodeScreen: Enter RPC node URL
AddMoneroNodeScreen->>AddMoneroNodeViewModel: Submit URL
AddMoneroNodeViewModel->>AddMoneroNodeViewModel: Parse URL and validate explicit port
AddMoneroNodeViewModel->>LocalizationResources: Request port-required error
AddMoneroNodeViewModel->>MoneroNodeManager: Check normalized endpoint with hasNode(url)
MoneroNodeManager-->>AddMoneroNodeViewModel: Return node existence
AddMoneroNodeViewModel-->>AddMoneroNodeScreen: Emit caution or continue state
Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
walletkit/src/main/java/io/horizontalsystems/walletkit/modules/moneronetwork/addnode/AddMoneroNodeScreen.kt (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the URL hint into a string resource.
This user-facing text bypasses localization and the translation workflow. Add an
AddMoneroNode_NodeUrlHintresource and usestringResource(...)here.🤖 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 AddMoneroNode screen’s input configuration into the localized string resources as AddMoneroNode_NodeUrlHint, then replace the literal with stringResource(...) at the existing hint assignment.
🤖 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 @.github/workflows/translate.yml:
- Line 60: Update the count validation in the workflow step so a mismatch emits
a clear diagnostic containing the additions count and files count before exiting
nonzero. Preserve successful validation behavior and ensure the failure path
does not rely on a bare test command under set -e.
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/MoneroNodeManager.kt`:
- Around line 157-160: Update endpoint() to lowercase the parsed Uri.host using
Locale.ROOT before constructing the host-and-port endpoint, ensuring
casing-insensitive values compare consistently in hasNode(). Add a regression
test covering equivalent uppercase and lowercase Monero node URLs.
---
Nitpick comments:
In
`@walletkit/src/main/java/io/horizontalsystems/walletkit/modules/moneronetwork/addnode/AddMoneroNodeScreen.kt`:
- Line 65: Move the hardcoded URL hint from the AddMoneroNode screen’s input
configuration into the localized string resources as AddMoneroNode_NodeUrlHint,
then replace the literal with stringResource(...) at the existing hint
assignment.
🪄 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: d8f6f0f0-39a5-471b-b89f-bd3e859a4646
⛔ Files ignored due to path filters (9)
walletkit/src/main/res/values-de/strings.xmlis excluded by!**/res/values-*/strings.xmlwalletkit/src/main/res/values-es/strings.xmlis excluded by!**/res/values-*/strings.xmlwalletkit/src/main/res/values-fa/strings.xmlis excluded by!**/res/values-*/strings.xmlwalletkit/src/main/res/values-fr/strings.xmlis excluded by!**/res/values-*/strings.xmlwalletkit/src/main/res/values-ko/strings.xmlis excluded by!**/res/values-*/strings.xmlwalletkit/src/main/res/values-pt-rBR/strings.xmlis excluded by!**/res/values-*/strings.xmlwalletkit/src/main/res/values-ru/strings.xmlis excluded by!**/res/values-*/strings.xmlwalletkit/src/main/res/values-tr/strings.xmlis excluded by!**/res/values-*/strings.xmlwalletkit/src/main/res/values-zh/strings.xmlis excluded by!**/res/values-*/strings.xml
📒 Files selected for processing (6)
.github/workflows/translate.ymltranslation_snapshot.jsonwalletkit/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
| jq -n --arg path "$f" --rawfile contents "$RUNNER_TEMP/file.b64" \ | ||
| '{path: $path, contents: $contents}' >> "$additions" | ||
| done <<< "$files" | ||
| [ "$(jq -s length "$additions")" -eq "$(echo "$files" | wc -l)" ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Count validation exits silently on mismatch.
Under GitHub Actions' default set -e, a failed [ ... -eq ... ] exits the script with no diagnostic output, making failures hard to debug.
🔧 Suggested fix
- [ "$(jq -s length "$additions")" -eq "$(echo "$files" | wc -l)" ]
+ count_jsonl=$(jq -s length "$additions")
+ count_files=$(echo "$files" | wc -l)
+ if [ "$count_jsonl" -ne "$count_files" ]; then
+ echo "Error: JSONL record count ($count_jsonl) != file count ($count_files)" >&2
+ exit 1
+ fi📝 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.
| [ "$(jq -s length "$additions")" -eq "$(echo "$files" | wc -l)" ] | |
| count_jsonl=$(jq -s length "$additions") | |
| count_files=$(echo "$files" | wc -l) | |
| if [ "$count_jsonl" -ne "$count_files" ]; then | |
| echo "Error: JSONL record count ($count_jsonl) != file count ($count_files)" >&2 | |
| exit 1 | |
| fi |
🤖 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 @.github/workflows/translate.yml at line 60, Update the count validation in
the workflow step so a mismatch emits a clear diagnostic containing the
additions count and files count before exiting nonzero. Preserve successful
validation behavior and ensure the failure path does not rely on a bare test
command under set -e.
| 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:
#!/bin/bash
set -euo pipefail
# Map the target file
ast-grep outline walletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/MoneroNodeManager.kt --view expanded || true
# Read the relevant section with line numbers
sed -n '1,260p' walletkit/src/main/java/io/horizontalsystems/walletkit/core/managers/MoneroNodeManager.kt | cat -n
# Search for endpoint comparison / normalization and tests
rg -n "endpoint\\(|toUri\\(|effectivePort\\(|host\\.|lowercase|Locale\\.ROOT|MoneroNodeManager" walletkit/src/main/java walletkit/src/test walletkit/src/androidTest || trueRepository: horizontalsystems/unstoppable-wallet-android
Length of output: 27091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Monero add-node flow and any related normalization helpers
sed -n '1,220p' walletkit/src/main/java/io/horizontalsystems/walletkit/modules/moneronetwork/addnode/AddMoneroNodeViewModel.kt | cat -n
sed -n '1,220p' walletkit/src/main/java/io/horizontalsystems/walletkit/modules/zcashnetwork/addendpoint/AddZcashEndpointViewModel.kt | cat -n
sed -n '1,220p' walletkit/src/main/java/io/horizontalsystems/walletkit/modules/moneronetwork/MoneroNetworkViewModel.kt | cat -n
# Probe Android Uri host behavior if the sandbox has a suitable Kotlin/JVM setup; otherwise fall back to docs-free text inspection
python3 - <<'PY'
from urllib.parse import urlparse
samples = [
"https://NODE.EXAMPLE:18081",
"NODE.EXAMPLE:18081",
"https://node.example:18081",
]
for s in samples:
u = urlparse(s if "://" in s else "https://" + s)
print(s, "-> hostname:", u.hostname, "netloc:", u.netloc)
PYRepository: horizontalsystems/unstoppable-wallet-android
Length of output: 12416
🌐 Web query:
Android Uri.getHost case normalization host lowercase documentation
💡 Result:
The Android android.net.Uri.getHost method does not automatically normalize the host component to lowercase [1][2]. It returns the host exactly as it is encoded within the authority component of the URI [1][2]. Because this method does not perform case normalization, developers are responsible for handling case sensitivity when validating or comparing hostnames [3]. Official Android security documentation emphasizes that when validating hosts (for example, to prevent unsafe URI loading), you should perform robust validation on the host string returned by getHost rather than relying on internal normalization [3]. In contrast, the Java standard library's java.net.URI class documentation indicates that while its getHost method also returns the host component, equality checks for hierarchical URIs (using.equals) do compare hosts without regard to case [4]. Note that android.net.Uri and java.net.URI are distinct classes with different behaviors, and android.net.Uri is generally the preferred class for handling URIs within Android applications [1][2]. If you need a normalized scheme for intent filtering, android.net.Uri provides a separate normalizeScheme method, but this method specifically targets the scheme component and does not affect the host [5].
Citations:
- 1: https://developer.android.com/reference/kotlin/android/net/Uri
- 2: https://devdoc.net/android/Android-r19/reference/android/net/Uri.html
- 3: https://p.rst.im/q/developer.android.com/privacy-and-security/risks/unsafe-uri-loading
- 4: https://developer.android.com/reference/java/net/URI
- 5: https://github.com/aosp-mirror/platform_frameworks_base/blob/master/core/java/android/net/Uri.java
Lowercase Monero node endpoints before comparing them.
endpoint() preserves the host casing from Uri.host, so https://NODE.EXAMPLE:18081 can bypass hasNode() next to https://node.example:18081. Lowercase the parsed host with Locale.ROOT before building the endpoint, and add a regression test.
🤖 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 the parsed Uri.host using
Locale.ROOT before constructing the host-and-port endpoint, ensuring
casing-insensitive values compare consistently in hasNode(). Add a regression
test covering equivalent uppercase and lowercase Monero node URLs.
2 participants
#8358