feat: add managed_core_account_set_transaction_label FFI function#618
Open
xdustinface wants to merge 4 commits intov0.42-devfrom
Open
feat: add managed_core_account_set_transaction_label FFI function#618xdustinface wants to merge 4 commits intov0.42-devfrom
managed_core_account_set_transaction_label FFI function#618xdustinface wants to merge 4 commits intov0.42-devfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 7 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a new FFI function Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (FFI caller)
participant FFI as FFI layer
participant Manager as WalletManager
participant Account as ManagedAccount
participant Storage as Account Transactions
rect rgba(200,200,255,0.5)
Client->>FFI: wallet_manager_set_transaction_label(manager, wallet_id, account_type, account_index, txid, label)
end
rect rgba(200,255,200,0.5)
FFI->>Manager: acquire write lock (async)
Manager->>Manager: locate wallet by wallet_id
Manager->>Account: select account by type/index
Account->>Storage: find transaction by txid
Storage-->>Account: transaction record (or not found)
Account-->>Manager: update record.label (set or clear)
Manager->>FFI: release lock, return success/failure
end
rect rgba(255,200,200,0.5)
FFI-->>Client: boolean + FFIError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #618 +/- ##
=============================================
+ Coverage 67.65% 68.13% +0.48%
=============================================
Files 319 319
Lines 67894 67938 +44
=============================================
+ Hits 45932 46289 +357
+ Misses 21962 21649 -313
|
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
Add mutable access to `FFIManagedCoreAccount` via `inner_mut()` and expose a function to set or clear transaction labels by txid. Also add comprehensive tests for: - `managed_core_account_get_transactions` with real transaction data - `managed_core_account_free_transactions` verifying no crash on free - Label round-trip (set, read back, clear, verify cleared) - Error cases (null account, missing txid)
xdustinface
force-pushed
the
feat/transaction-label-ffi
branch
from
4b5f8c7 to
f272538
Compare
github-actions
bot
removed
the
merge-conflict
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 896-969: The setter managed_core_account_set_transaction_label is
mutating only the cloned per-handle snapshot (FFIManagedCoreAccount built in
managed_wallet_get_account via Arc::new(account.clone())), so changes are not
persisted; update the function to locate and mutate the shared managed-wallet
storage instead of the local cloned record: look up the account in the wallet
manager using the same identifier used by managed_wallet_get_account (or accept
a wallet/account handle that references the shared storage), then fetch the
transaction record from managed.transactions in that shared account and call
record.set_label(...) on that shared record (handling UTF-8 and null label cases
as before), and only then return success so the label persists across handles
and future managed_wallet_get_account calls.
🪄 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
Run ID: fb5cc841-5a97-4e0b-8ec7-f3eae7b62486
📒 Files selected for processing (2)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/managed_account.rs
CodeRabbit flagged that the original `managed_core_account_set_transaction_label` mutated only a detached snapshot. `wallet_manager_get_managed_wallet_info` returns a cloned `ManagedWalletInfo`, and `FFIManagedCoreAccount::new` wraps each account in `Arc::new(account.clone())`, so writes through the handle never reached the shared `WalletManager::wallet_infos` map and were silently discarded when the handle was freed. Replace the broken FFI with `wallet_manager_set_transaction_label`, which acquires the manager write lock, navigates to the canonical `ManagedCoreAccount` via `get_wallet_info_mut`, and mutates the shared `TransactionRecord` in place. Drop the vacuous `FFIManagedCoreAccount::inner_mut` (the `Arc::get_mut` guard always succeeded on a freshly cloned `Arc`). Promote `get_managed_account_by_type[_mut]` to `pub(crate)` so the new FFI can reuse the existing dispatch. Rewrite the tests to insert the `TransactionRecord` through the manager's write lock and verify the label round-trips across handle frees by re-fetching the account after each mutation. The broader `FFIManagedWalletInfo` dual-path hazard (other mutating FFIs lose writes when the handle comes from the manager) is tracked separately in xdustinface#110. Addresses CodeRabbit review comment on PR #618 #618 (comment)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/FFI_API.md`:
- Around line 719-733: The PR/README exposes the FFI symbol
wallet_manager_set_transaction_label but the PR title and release wording
reference managed_core_account_set_transaction_label; update the PR
title/release notes to reference wallet_manager_set_transaction_label (or
alternatively add a thin exported wrapper function named
managed_core_account_set_transaction_label that forwards to
wallet_manager_set_transaction_label and document it), and ensure the
pr-title.yml allowed prefix list is consistent with the chosen public symbol
name.
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 2313-2380: The test test_set_transaction_label_errors needs to be
updated to reflect that wallet_manager_set_transaction_label should return
FFIErrorCode::NotFound for missing wallets/txids: change the expected error code
for the case passing an all-zero wallet_id and for the fake_txid (the last two
assertions after calls to wallet_manager_set_transaction_label) from
InvalidInput to NotFound, and add an additional assertion/case that calls
wallet_manager_set_transaction_label with a valid manager and wallet_ids_out but
a non-existent txid for that real wallet to assert NotFound (so null-pointer
cases still assert InvalidInput while existence-mismatch cases assert NotFound).
- Around line 945-971: The lookup failures inside the
manager_ref.runtime.block_on closure currently return plain Err(String) which is
always reported as FFIErrorCode::InvalidInput; change the closure to return a
richer error so the caller can distinguish NotFound vs InvalidInput (e.g. make
the closure return Result<(), (FFIErrorCode, String)> or a small enum), replace
the three .ok_or_else(...) calls to produce Err((FFIErrorCode::NotFound, "Wallet
not found".to_string())) / Err((FFIErrorCode::NotFound, "Account not
found".to_string())) / Err((FFIErrorCode::NotFound, "Transaction not
found".to_string())), and then in the outer error branch call
FFIError::set_error(error, code, msg) using the propagated code; update the
test_set_transaction_label_errors test expectation accordingly to expect
NotFound for the fake-txid case.
🪄 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
Run ID: 12ac23bd-a28b-4030-a390-2d6d21c1362d
📒 Files selected for processing (3)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rs
Comment on lines
+719
to
+733
| #### `wallet_manager_set_transaction_label` | ||
|
|
||
| ```c | ||
| wallet_manager_set_transaction_label(manager: *mut FFIWalletManager, wallet_id: *const u8, account_type: FFIAccountType, account_index: c_uint, txid: *const u8, label: *const c_char, error: *mut FFIError,) -> bool | ||
| ``` | ||
|
|
||
| **Description:** | ||
| Set or clear a label on a transaction record in the shared wallet manager state # Safety - `manager` must be a valid pointer to an FFIWalletManager instance - `wallet_id` must be a valid pointer to a 32-byte wallet ID - `txid` must be a valid pointer to a 32-byte transaction ID - `label` must be a valid null-terminated UTF-8 string, or null to clear the label - `error` must be a valid pointer to an FFIError structure or null - The caller must ensure all pointers remain valid for the duration of this call | ||
|
|
||
| **Safety:** | ||
| - `manager` must be a valid pointer to an FFIWalletManager instance - `wallet_id` must be a valid pointer to a 32-byte wallet ID - `txid` must be a valid pointer to a 32-byte transaction ID - `label` must be a valid null-terminated UTF-8 string, or null to clear the label - `error` must be a valid pointer to an FFIError structure or null - The caller must ensure all pointers remain valid for the duration of this call | ||
|
|
||
| **Module:** `managed_account` | ||
|
|
||
| --- |
Contributor
There was a problem hiding this comment.
Align the PR title with the exported FFI symbol.
This documentation exposes wallet_manager_set_transaction_label, but the PR title names managed_core_account_set_transaction_label. Please rename the PR title/release wording, or add the managed_core_account_* wrapper if that was the intended public API. As per coding guidelines, "**: Check whether the PR title prefix allowed in the pr-title.yml workflow accurately describes the changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-ffi/FFI_API.md` around lines 719 - 733, The PR/README exposes the
FFI symbol wallet_manager_set_transaction_label but the PR title and release
wording reference managed_core_account_set_transaction_label; update the PR
title/release notes to reference wallet_manager_set_transaction_label (or
alternatively add a thin exported wrapper function named
managed_core_account_set_transaction_label that forwards to
wallet_manager_set_transaction_label and document it), and ensure the
pr-title.yml allowed prefix list is consistent with the chosen public symbol
name.
…let_manager_set_transaction_label`
Lookup failures inside the async closure were all surfaced as `FFIErrorCode::InvalidInput`, conflating genuine caller mistakes (null pointers, bad UTF-8, bad txid length) with missing-resource conditions. Other FFI lookup functions in the same file and `WalletError::{WalletNotFound, AccountNotFound}` already map to `FFIErrorCode::NotFound`, so callers branching on the error code could not distinguish programmer errors from an expected "no such txid/wallet/account" outcome.
Propagate a typed `(FFIErrorCode, String)` pair out of the closure so the three lookup failures emit `NotFound` while `set_label` validation continues to emit `InvalidInput`. Update `test_set_transaction_label_errors` to assert `NotFound` for the real-wallet/fake-txid case and add a real-manager/unknown-wallet_id case that exercises the "Wallet not found" path.
Addresses CodeRabbit review comment on PR #618
#618 (comment)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 943-946: The code creates an unnecessary exclusive borrow with
`&mut *manager` before calling `runtime.block_on` and acquiring
`manager.write()`; replace those exclusive borrows with shared borrows (e.g.
`let manager_ref = &*manager`) so `runtime.block_on` and the async
`manager.write().await` use a shared reference instead of a mutable one; update
both occurrences (the one used before `runtime.block_on` and the second
occurrence around line 2191) to use the shared reference `manager_ref` and
remove any `&mut *manager` usage.
🪄 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
Run ID: 890447ea-a94a-4a76-a161-b2739590386e
📒 Files selected for processing (1)
key-wallet-ffi/src/managed_account.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add mutable access to
FFIManagedCoreAccountviainner_mut()and expose a function to set or clear transaction labels by txid.Also add comprehensive tests for:
managed_core_account_get_transactionswith real transaction datamanaged_core_account_free_transactionsverifying no crash on freeBased on:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores