chore: update to 16KB page size dependencies #1437
Conversation
WalkthroughThe PR includes a root build configuration version bump from 2.0.1 to 2.0.2-SNAPSHOT, a new shell script for verifying 64-bit ELF library alignment, and dependency updates for DashJ Android libraries and Conscrypt in the wallet module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 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: 2
🧹 Nitpick comments (4)
check-alignment.sh (4)
22-25: Specify exit code explicitly.Line 24 exits without a code, which defaults to the status of the previous command (the
usagefunction). For clarity and consistency, use an explicit exit code.Apply this diff to clarify the exit code:
if [ ${#} -ne 1 ]; then usage - exit + exit 1 fi
103-104: Add safety check for objdump output and validate alignment value.Line 103 assumes objdump produces output with a LOAD segment; if the file is malformed or objdump fails,
reswill be empty, and the regex will silently treat it as unaligned. Additionally, the regex doesn't cap the exponent, which could match unreasonably high powers of 2.Add explicit validation and a reasonable upper bound:
res="$(objdump -p "${match}" | grep LOAD | awk '{ print $NF }' | head -1)" + if [ -z "${res}" ]; then + printf "${match}: ${RED}UNALIGNED${ENDCOLOR} (no LOAD segment found)\n" + unaligned_libs+=("${match}") + continue + fi - if [[ $res =~ 2\*\*(1[4-9]|[2-9][0-9]|[1-9][0-9]{2,}) ]]; then + if [[ $res =~ 2\*\*(1[4-6]) ]]; then printf "${match}: ${GREEN}ALIGNED${ENDCOLOR} ($res)\n" else printf "${match}: ${RED}UNALIGNED${ENDCOLOR} ($res)\n" unaligned_libs+=("${match}") fiThis ensures:
- Empty results are explicitly handled with a diagnostic message
- Only reasonable alignments (2^14 = 16KB, 2^15 = 32KB, 2^16 = 64KB) are considered ALIGNED
1-117: Consider adding availability checks for required tools.The script relies on
objdump(for all ELF files),zipalign(for APK inspection), anddeapexer(for APEX extraction). If these tools are missing, the script will fail silently or with unclear error messages. A pre-flight check would improve user experience.Add helper functions to verify required tools at script startup:
check_command() { if ! command -v "$1" &> /dev/null; then echo "Error: Required tool '$1' not found. Please install it." >&2 exit 1 fi } # Check for required tools based on input type check_command objdump if [[ "${1}" == *.apk ]]; then check_command zipalign fi if [[ "${1}" == *.apex ]]; then check_command deapexer fiAlternatively, add graceful degradation with informative error messages when tools are unavailable.
43-66: Improve error handling and deapexer availability messaging.The APEX handling (line 77) uses
deapexerbut does not check if it is installed beforehand. The error message is generic. Similarly, the APK zipalign check (line 50) already provides helpful guidance when build-tools are outdated; APEX handling should offer similar assistance.Add a helper to check APEX tool availability and provide installation guidance:
if [[ "${dir}" == *.apex ]]; then trap 'cleanup_trap' EXIT if ! command -v deapexer &> /dev/null; then echo "NOTICE: APEX extraction requires 'deapexer' tool." echo " Install it as part of the Android SDK or source it from the platform tools." exit 1 fi echo echo "Recursively analyzing $dir" echo dir_filename=$(basename "${dir}") tmp=$(mktemp -d -t "${dir_filename%.apex}_out_XXXXX") deapexer extract "${dir}" "${tmp}" || { echo "Failed to deapex: check file integrity." && exit 1; } dir="${tmp}" fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build.gradle(1 hunks)check-alignment.sh(1 hunks)wallet/build.gradle(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1417
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt:0-0
Timestamp: 2025-08-08T16:48:49.964Z
Learning: In PR dashpay/dash-wallet#1417, HashEngineering chose to defer adding Room indexes for gift_card_providers (provider, denominationsType, merchantId) to a follow-up PR; do not block the current PR on this optimization. Files: features/exploredash/.../data/explore/MerchantDao.kt and features/exploredash/.../data/dashspend/GiftCardProvider.kt.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1423
File: wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt:418-446
Timestamp: 2025-08-25T14:48:39.247Z
Learning: HashEngineering prefers to refactor and reuse topup code for balance validation logic improvements in DashPay activities like EditProfileActivity, rather than implementing individual fixes in the current PR.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:134-145
Timestamp: 2025-09-05T06:16:12.649Z
Learning: The dash-wallet project has a minimum SDK version of 24, so API availability checks for methods introduced in API 23 or earlier are not needed.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsScreen.kt:122-151
Timestamp: 2025-09-05T06:47:44.508Z
Learning: HashEngineering prefers to defer UI edge case fixes like the CoinJoin finishing state display issue (showing "Finishing ...%" when mode is OFF but status is FINISHING) to later PRs when the main focus is on redesigning the settings screen.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: wallet/src/de/schildbach/wallet/data/PaymentIntent.java:476-481
Timestamp: 2025-08-25T15:04:18.461Z
Learning: HashEngineering prefers to keep payeeUsername and payeeUserId in PaymentIntent.toString() logs unredacted for debugging purposes, prioritizing troubleshooting capability over PII protection concerns in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: common/src/main/java/org/dash/wallet/common/ui/components/DashButton.kt:94-101
Timestamp: 2025-09-05T06:00:25.088Z
Learning: HashEngineering prefers to fix the DashButton alpha dimming issue (using explicit disabled colors instead of overall alpha) in a follow-up PR rather than including it in the current settings screen redesign PR.
📚 Learning: 2025-09-05T06:16:12.649Z
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:134-145
Timestamp: 2025-09-05T06:16:12.649Z
Learning: The dash-wallet project has a minimum SDK version of 24, so API availability checks for methods introduced in API 23 or earlier are not needed.
Applied to files:
wallet/build.gradle
🪛 Shellcheck (0.11.0)
check-alignment.sh
[warning] 8-8: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
(SC2166)
[warning] 38-38: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
(SC2166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
build.gradle (1)
7-7: Version update aligns with coordinated dependency changes.The bump to
2.0.2-SNAPSHOTis appropriate for development with the related dashj-android and kotlin-platform PRs. Ensure all dependent modules (wallet/build.gradle, etc.) are tested with SNAPSHOT versions before final release.wallet/build.gradle (2)
61-63: Verify SNAPSHOT libraries are available and tested before release.Major version bumps for
dashj-x11-android(0.17.5 → 1.0.0-SNAPSHOT) anddashj-scrypt-android(0.17.5 → 1.0.0-SNAPSHOT) warrant thorough validation. SNAPSHOT versions may be unstable or temporarily unavailable from Maven repositories.Confirm:
- All SNAPSHOT libraries build successfully and are available in configured repositories
- Cryptographic library changes are validated against test vectors and hardware targets
- Integration tests pass with the new 16KB page-aligned binaries
- Plan for upgrading to stable releases once the 16KB page size support is complete
82-82: Conscrypt patch update is appropriate.The bump from 2.5.2 to 2.5.3 is a patch-level update, consistent with the TLS 1.3 support goal. The existing comment adequately documents the purpose.
| # usage: check_elf_alignment.sh [path to *.so files|path to *.apk] | ||
|
|
||
| cleanup_trap() { | ||
| if [ -n "${tmp}" -a -d "${tmp}" ]; then |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use && instead of -a operator (SC2166).
The test condition -a is not POSIX-compliant; modern shell linting recommends && for logical AND.
Apply this diff to fix the shell compatibility warning:
cleanup_trap() {
- if [ -n "${tmp}" -a -d "${tmp}" ]; then
+ if [ -n "${tmp}" ] && [ -d "${tmp}" ]; then
rm -rf ${tmp}
fi
exit $1📝 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.
| if [ -n "${tmp}" -a -d "${tmp}" ]; then | |
| cleanup_trap() { | |
| if [ -n "${tmp}" ] && [ -d "${tmp}" ]; then | |
| rm -rf ${tmp} | |
| fi | |
| exit $1 | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 8-8: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
(SC2166)
🤖 Prompt for AI Agents
In check-alignment.sh around line 8, the test uses the non-POSIX `-a` operator
("if [ -n \"${tmp}\" -a -d \"${tmp}\" ]; then"); replace the `-a` with a
POSIX-friendly `&&` by splitting the conditions inside the single-bracket test
into two tests joined with `&&` (or use a single `[[ ... && ... ]]` if the
script requires bash), ensuring you preserve the existing parameter expansion
and quoting.
Issue being fixed or feature implemented
Support 16KB page size requirement. Only affects dependencies with native code.
Related PR's and Dependencies
dashpay/dashj-android#5
dashpay/kotlin-platform#29
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit