feat: record DAPI address ban reason and expose via platform-wallet FFI#3890
Conversation
|
Warning Review limit reached
More reviews will be available in 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds optional ban-reason tracking to DAPI address banning. ChangesDAPI Address Ban Reason Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (1)
packages/rs-dapi-client/src/dapi_client.rs (1)
190-190: ⚡ Quick winAdd a direct regression check for reason propagation in this call path.
Line 190 changes behavior materially, but current tests only verify that banning happened. Add one assertion in the retryable-error test that
address_list.ban_info()[0].reasonis populated afterupdate_address_ban_status.🤖 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 `@packages/rs-dapi-client/src/dapi_client.rs` at line 190, The ban_with_reason call on line 190 now passes error details as the reason, but the existing retryable-error test only verifies that banning occurred without checking if the reason was actually propagated. Add an assertion in the retryable-error test that directly verifies the reason field is populated by accessing address_list.ban_info()[0].reason after the update_address_ban_status call completes, ensuring the error message was correctly stored in the ban information.
🤖 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 `@packages/rs-platform-wallet-ffi/src/manager_diagnostics.rs`:
- Around line 820-824: The ban_count field assignment uses an unsafe raw cast
(as u32) on s.ban_count which can silently truncate values on 64-bit targets,
causing incorrect diagnostics to be reported through FFI. Replace the raw cast
in the ban_count: s.ban_count as u32 line with a checked conversion method (such
as try_into() with appropriate error handling, or use .min(u32::MAX) for
explicit saturation) to prevent truncation and ensure accurate diagnostic
information is passed to the FFI caller.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerDiagnostics.swift`:
- Line 515: The current code in PlatformWalletManagerDiagnostics.swift converts
NULL addresses to empty strings using optional mapping with nil coalescing,
which creates non-actionable diagnostic entries. Replace this mapping approach
with compactMap to filter out entries where entry.address is NULL entirely,
ensuring only entries with valid addresses are retained and included in the
diagnostic output. This will prevent empty string addresses from appearing in
the ban info entries.
---
Nitpick comments:
In `@packages/rs-dapi-client/src/dapi_client.rs`:
- Line 190: The ban_with_reason call on line 190 now passes error details as the
reason, but the existing retryable-error test only verifies that banning
occurred without checking if the reason was actually propagated. Add an
assertion in the retryable-error test that directly verifies the reason field is
populated by accessing address_list.ban_info()[0].reason after the
update_address_ban_status call completes, ensuring the error message was
correctly stored in the ban information.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3da25ac1-f764-4101-a91c-9aa69ff9ae6a
📒 Files selected for processing (8)
packages/rs-dapi-client/src/address_list.rspackages/rs-dapi-client/src/dapi_client.rspackages/rs-dapi-client/src/lib.rspackages/rs-platform-wallet-ffi/src/core_wallet_types.rspackages/rs-platform-wallet-ffi/src/manager_diagnostics.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-sdk/src/sdk.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerDiagnostics.swift
The DAPI client's ban list previously tracked only ban_count and
banned_until; the error that caused a ban was logged then discarded.
Capture it so it can be surfaced to clients (e.g. the iOS app's
diagnostics) when a node becomes unavailable.
- rs-dapi-client: add AddressStatus.ban_reason; ban_with_reason() stores
it (ban() kept as a None-reason shim); unban() clears it. New public
AddressBanInfo { uri, banned, ban_count, banned_until, reason } and
AddressList::ban_info() snapshot getter. The runtime ban site in
update_address_ban_status now records error.to_string().
- dash-sdk: Sdk::address_ban_info() over address_list().ban_info().
- platform-wallet: PlatformWalletManager::address_ban_info_blocking()
returning an owned snapshot (banned_until projected to epoch-ms).
- platform-wallet-ffi: platform_wallet_manager_address_ban_info() +
_free, struct AddressBanInfoFFI { address, banned, ban_count,
banned_until_ms, reason }, following the tracked-asset-locks array
convention.
- swift-sdk: PlatformWalletManager.addressBanInfo() -> [AddressBanInfo]
thin bridge.
Additive and non-breaking (ban() signature preserved; new field is
private). rs-dapi-client gains 8 unit tests covering reason set/clear
and ban_info reflection.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
36bdc55 to
c4c372e
Compare
|
Rebased onto latest Verified end-to-end on this rebased branch: |
- platform-wallet-ffi: saturate ban_count usize->u32 conversion (u32::try_from(..).unwrap_or(u32::MAX)) instead of a raw cast that could silently truncate on 64-bit. - swift-sdk: addressBanInfo() uses compactMap and skips entries with a NULL address rather than surfacing a non-actionable blank row. - rs-dapi-client: the retryable-error ban test now also asserts the ban reason is propagated through update_address_ban_status. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split the public AddressBanInfo snapshot out of address_list.rs into address_ban_info.rs (address_list.rs now holds only Address / AddressStatus / AddressList). The crate-root re-export path (rs_dapi_client::AddressBanInfo) is unchanged, so rs-sdk / platform-wallet consumers are unaffected. Drive-by: fixed a pre-existing broken intra-doc link ([get_live_address] -> [`Self::get_live_address`]) in the same file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Surface the DAPI address ban list (with reasons) in Settings → Data, as the last diagnostic explorer before "Manage Local Data". The view is pure presentation over the existing PlatformWalletManager.addressBanInfo() wrapper: lists each address with a Banned/Live badge, ban count, banned-until, and the recorded reason; refresh via toolbar + pull-to- refresh; ContentUnavailableView empty state noting an empty list can mean no bans or an unseeded address pool. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "593b5946c8f07d28fae1da04f58ea5001a222aea9694aab68397293a4af05aca"
)Xcode manual integration:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3890 +/- ##
=============================================
- Coverage 87.22% 71.20% -16.03%
=============================================
Files 2641 20 -2621
Lines 328569 2837 -325732
=============================================
- Hits 286597 2020 -284577
+ Misses 41972 817 -41155
🚀 New features to boost your workflow:
|
1 participant
Issue being fixed or feature implemented
When the DAPI client bans an address, it tracked only
ban_countandbanned_until— the underlying error (the reason) was logged attracing::warn!and then discarded. This surfaced during iOS QA: a shielded operation failed withDapi client error: no available addresses to use, and there was no way for the app to show why the nodes were banned. This change records the ban reason and exposes the ban list (with reasons) up throughplatform-walletand its FFI so clients like the iOS app can read it.What was done?
Threaded a ban reason through the stack (additive, non-breaking):
address_list.rs): addedAddressStatus.ban_reason: Option<String>; newban_with_reason()stores it (ban()preserved as aNone-reason shim);unban()clears it. Added publicAddressBanInfo { uri, banned, ban_count, banned_until, reason }andAddressList::ban_info() -> Vec<AddressBanInfo>snapshot getter (exported from the crate root). The runtime ban site inupdate_address_ban_status(dapi_client.rs) now passeserror.to_string().Sdk::address_ban_info()delegating toaddress_list().ban_info().PlatformWalletManager::address_ban_info_blocking() -> Vec<AddressBanInfoSnapshot>from itsArc<Sdk>(banned_untilprojected to epoch-ms so the FFI stays chrono-free).platform_wallet_manager_address_ban_info(...)+_free, structAddressBanInfoFFI { address, banned, ban_count, banned_until_ms, reason }, following the existingplatform_wallet_tracked_asset_locks_list/_freearray convention.PlatformWalletManager.addressBanInfo() -> [AddressBanInfo]— a thin bridge (no logic), per the swift-sdk architecture rules.How Has This Been Tested?
cargo fmt --all --checkclean.cargo check --all-targets+cargo clippy --all-targetsclean onrs-dapi-client,dash-sdk,platform-wallet,platform-wallet-ffi.cargo test -p rs-dapi-client→ 94 pass, including 8 new tests (ban stores reason / ban-without-reason isNone/unbanclears it at bothAddressStatusandAddressListlevels /ban_inforeflects banned/unbanned/empty).aarch64-apple-ios-sim); unified xcframework assembled; the Swift wrapper matches the regenerated header.Breaking Changes
None.
ban()keeps its signature; the newAddressStatusfield is private; all additions are new public items.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests