feat(wallet): add accounts.rename call - #1583
Conversation
WalkthroughAdds an accounts.rename feature across the stack: JSON-RPC endpoint, walletd handler with admin auth, SDK API to rename accounts, storage updates to accept borrowed AccountUpdate, Rust and TypeScript client types and methods, and version bumps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App / SDK User
participant JS as WalletDaemonClient (JS/Rust)
participant RPC as walletd JSON-RPC
participant H as accounts::handle_rename
participant SDK as SDK Accounts API
participant DB as Storage (Writer)
App->>JS: accountsRename({ account, new_name })
JS->>RPC: JSON-RPC "accounts.rename" (AccountsRenameRequest)
RPC->>H: Dispatch request
H->>H: Authenticate (Admin)
H->>SDK: rename_account(account_addr, new_name)
SDK->>DB: accounts_update(AccountUpdate { name: Some(&new_name), ... })
DB-->>SDK: Ok / NotFound error
SDK-->>H: Result
H-->>RPC: AccountsRenameResponse (empty on success)
RPC-->>JS: RPC result
JS-->>App: Resolve Promise<AccountsRenameResponse>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 3
🧹 Nitpick comments (2)
clients/javascript/wallet_daemon_client/src/index.ts (1)
172-174: Add admin-permission note and basic client-side validation for new_name.
Server requires Admin; add a short JSDoc note and reject empty names to save a roundtrip.- public accountsRename(params: AccountsRenameRequest): Promise<AccountsRenameResponse> { - return this.__invokeRpc("accounts.rename", params); - } + /** + * Renames an account. Requires Admin permission on the token. + */ + public accountsRename(params: AccountsRenameRequest): Promise<AccountsRenameResponse> { + if (!params.new_name?.trim()) { + return Promise.reject(new Error("new_name must be a non-empty string")); + } + return this.__invokeRpc("accounts.rename", params); + }Ensure rpc.discover includes "accounts.rename" so clients can feature-detect it.
crates/wallet/storage_sqlite/src/writer.rs (1)
526-563: Unique index already exists; map DB unique-violation to AccountNameAlreadyExistsMigration already enforces uniqueness (crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql:
CREATE UNIQUE INDEX accounts_uniq_name ON accounts (name) WHERE name IS NOT NULL;). Still handle races by converting DB unique-constraint failures into the API-level duplicate-name error: catch Diesel'sDatabaseErrorKind::UniqueViolationon name INSERT/UPDATE (crates/wallet/storage_sqlite/src/writer.rs — accounts_insert & accounts_update) and either return a clear store error or have the SDK convert that store error toAccountsApiError::AccountNameAlreadyExistswheretx.accounts_insert/tx.accounts_updateare called (crates/wallet/sdk/src/apis/accounts.rs — create_account / add_account).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
applications/tari_walletd/src/handlers/accounts.rs(2 hunks)applications/tari_walletd/src/jrpc_server.rs(1 hunks)bindings/package.json(1 hunks)bindings/src/types/wallet-daemon-client/AccountsRenameRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/AccountsRenameResponse.ts(1 hunks)bindings/src/wallet-daemon-client.ts(2 hunks)clients/javascript/wallet_daemon_client/package.json(1 hunks)clients/javascript/wallet_daemon_client/src/index.ts(2 hunks)clients/wallet_daemon_client/src/lib.rs(2 hunks)clients/wallet_daemon_client/src/types.rs(1 hunks)crates/wallet/sdk/src/apis/accounts.rs(2 hunks)crates/wallet/sdk/src/models/account.rs(1 hunks)crates/wallet/sdk/src/storage.rs(1 hunks)crates/wallet/storage_sqlite/src/writer.rs(1 hunks)crates/wallet/storage_sqlite/tests/accounts.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
clients/javascript/wallet_daemon_client/src/index.ts (2)
bindings/src/types/wallet-daemon-client/AccountsRenameRequest.ts (1)
AccountsRenameRequest(4-4)bindings/src/types/wallet-daemon-client/AccountsRenameResponse.ts (1)
AccountsRenameResponse(3-3)
applications/tari_walletd/src/jrpc_server.rs (1)
applications/tari_walletd/src/handlers/accounts.rs (2)
accounts(241-250)handle_rename(218-229)
clients/wallet_daemon_client/src/types.rs (3)
bindings/src/types/wallet-daemon-client/AccountsRenameRequest.ts (1)
AccountsRenameRequest(4-4)bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
ComponentAddressOrName(4-4)bindings/src/types/wallet-daemon-client/AccountsRenameResponse.ts (1)
AccountsRenameResponse(3-3)
crates/wallet/storage_sqlite/src/writer.rs (1)
crates/wallet/sdk/src/storage.rs (1)
accounts_update(367-371)
bindings/src/types/wallet-daemon-client/AccountsRenameRequest.ts (1)
bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
ComponentAddressOrName(4-4)
applications/tari_walletd/src/handlers/accounts.rs (4)
bindings/src/types/wallet-daemon-client/AccountsRenameRequest.ts (1)
AccountsRenameRequest(4-4)bindings/src/types/wallet-daemon-client/AccountsRenameResponse.ts (1)
AccountsRenameResponse(3-3)bindings/src/types/JrpcPermission.ts (1)
JrpcPermission(6-18)applications/tari_walletd/src/handlers/helpers.rs (1)
get_account(110-121)
clients/wallet_daemon_client/src/lib.rs (3)
bindings/src/types/wallet-daemon-client/AccountsRenameRequest.ts (1)
AccountsRenameRequest(4-4)bindings/src/types/wallet-daemon-client/AccountsRenameResponse.ts (1)
AccountsRenameResponse(3-3)bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
ComponentAddressOrName(4-4)
crates/wallet/sdk/src/apis/accounts.rs (1)
bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)
⏰ 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). (5)
- GitHub Check: check nightly
- GitHub Check: check stable
- GitHub Check: machete
- GitHub Check: clippy
- GitHub Check: test
🔇 Additional comments (15)
bindings/package.json (1)
3-3: Version bump looks good; confirm publish artifacts and changelog.
Ensure the new types (AccountsRename*) are included in the built dist and update CHANGELOG/release notes accordingly.clients/javascript/wallet_daemon_client/package.json (1)
3-3: Version bump OK; verify workspace resolution.
Confirm consumers resolve the updated @tari-project/typescript-bindings version and that CI publishes 1.9.1 with the new accountsRename surface.clients/javascript/wallet_daemon_client/src/index.ts (1)
19-20: Type imports for AccountsRename{Request,Response} are correct.
Matches bindings’ exports; no issues.crates/wallet/storage_sqlite/tests/accounts.rs (1)
23-23: LGTM: aligns with AccountUpdate<'_> borrowing.
Using &str here matches the lifetime-parametrized AccountUpdate.bindings/src/types/wallet-daemon-client/AccountsRenameResponse.ts (1)
1-4: Empty-object response type is fine.
Consistent with “void” responses elsewhere; no issues.bindings/src/types/wallet-daemon-client/AccountsRenameRequest.ts (1)
1-5: Request shape looks right; confirm field naming consistency with Rust.
Ensure the Rust AccountsRenameRequest uses new_name (not name) so ts-rs stays in sync.bindings/src/wallet-daemon-client.ts (2)
64-64: Export of AccountsRenameRequest added — OK.
Public surface now includes the request type.
99-99: Export of AccountsRenameResponse added — OK.
Matches the new API; no issues.crates/wallet/sdk/src/models/account.rs (1)
101-105: BorrowednameinAccountUpdate<'a>is appropriate.The lifetime-generic form with
Option<&'a str>is consistent with the storage API changes and avoids needless string copies.applications/tari_walletd/src/handlers/accounts.rs (1)
62-64: Type exports for rename requests/responses are correctly wired.crates/wallet/sdk/src/apis/accounts.rs (1)
126-135: BorrowedAccountUpdate<'_>inupdate_accountis correct.clients/wallet_daemon_client/src/types.rs (1)
455-466: Rename request/response types look correct and match TS bindings.Serde helper for
accountis consistent with other endpoints.clients/wallet_daemon_client/src/lib.rs (2)
89-91: New type imports are correct.
323-331: Verify server registration for "accounts.rename"; optional ergonomic change
- Repository search shows only the client call (clients/wallet_daemon_client/src/lib.rs:328). Confirm the server exposes/registers the "accounts.rename" JSON‑RPC method or update the client to match.
- Optional: change the signature to accept new_name: impl Into to improve caller ergonomics.
crates/wallet/sdk/src/storage.rs (1)
367-371: Verified: AccountUpdate<'_> change is safe — no owned String usages foundFound call sites:
- crates/wallet/sdk/src/apis/accounts.rs — rename_account: name: Some(new_name)
- crates/wallet/storage_sqlite/tests/accounts.rs — test: name: Some("foo")
- applications/tari_walletd/src/services/account_monitor.rs — sets is_account_on_chain only
No constructions using owned String (to_string()/String::from) for name were found; no changes required.
| pub async fn handle_rename( | ||
| context: &HandlerContext, | ||
| token: Option<&Bearer>, | ||
| req: AccountsRenameRequest, | ||
| ) -> Result<AccountsRenameResponse, anyhow::Error> { | ||
| context.check_auth(token, &[JrpcPermission::Admin])?; | ||
| let sdk = context.wallet_sdk(); | ||
| let account = get_account(&req.account, &sdk.accounts_api())?; | ||
| sdk.accounts_api() | ||
| .rename_account(account.component_address(), &req.new_name)?; | ||
| Ok(AccountsRenameResponse {}) | ||
| } |
There was a problem hiding this comment.
Map duplicate-name error to a client error (consistency with create).
rename_account may return AccountNameAlreadyExists. Like handle_create, map it to invalid_request instead of propagating a generic error.
Apply this diff:
- sdk.accounts_api()
- .rename_account(account.component_address(), &req.new_name)?;
+ sdk.accounts_api()
+ .rename_account(account.component_address(), &req.new_name)
+ .map_err(|e| {
+ if e.is_name_exists_error() {
+ invalid_request(e)
+ } else {
+ general_error(e)
+ }
+ })?;📝 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.
| pub async fn handle_rename( | |
| context: &HandlerContext, | |
| token: Option<&Bearer>, | |
| req: AccountsRenameRequest, | |
| ) -> Result<AccountsRenameResponse, anyhow::Error> { | |
| context.check_auth(token, &[JrpcPermission::Admin])?; | |
| let sdk = context.wallet_sdk(); | |
| let account = get_account(&req.account, &sdk.accounts_api())?; | |
| sdk.accounts_api() | |
| .rename_account(account.component_address(), &req.new_name)?; | |
| Ok(AccountsRenameResponse {}) | |
| } | |
| pub async fn handle_rename( | |
| context: &HandlerContext, | |
| token: Option<&Bearer>, | |
| req: AccountsRenameRequest, | |
| ) -> Result<AccountsRenameResponse, anyhow::Error> { | |
| context.check_auth(token, &[JrpcPermission::Admin])?; | |
| let sdk = context.wallet_sdk(); | |
| let account = get_account(&req.account, &sdk.accounts_api())?; | |
| sdk.accounts_api() | |
| .rename_account(account.component_address(), &req.new_name) | |
| .map_err(|e| { | |
| if e.is_name_exists_error() { | |
| invalid_request(e) | |
| } else { | |
| general_error(e) | |
| } | |
| })?; | |
| Ok(AccountsRenameResponse {}) | |
| } |
🤖 Prompt for AI Agents
In applications/tari_walletd/src/handlers/accounts.rs around lines 218 to 229,
the call to sdk.accounts_api().rename_account can return
AccountNameAlreadyExists but is currently propagated as a generic error; catch
the specific AccountNameAlreadyExists error and convert it into the same
client-facing invalid_request error used by handle_create (preserving a clear
message like "account name already exists"), returning that client error instead
of propagating the raw error; keep other errors propagated as before.
| }, | ||
| "stealth_transfer" => call_handler(context, value, token, accounts::handle_stealth_transfer).await, | ||
| "set_default" => call_handler(context, value, token, accounts::handle_set_default).await, | ||
| "rename" => call_handler(context, value, token, accounts::handle_rename).await, |
There was a problem hiding this comment.
🧩 Analysis chain
Route wiring for accounts.rename looks correct; verify validation/uniqueness.
Handler enforces Admin. Please confirm SDK/storage rejects empty or duplicate names and surfaces meaningful errors to JSON-RPC.
🏁 Script executed:
#!/bin/bash
# Locate rename_account implementation and callers for validation/uniqueness checks
rg -n -C3 --type=rust '\brename_account\s*\('
rg -n -C3 --type=rust 'struct\s+AccountsRenameRequest|enum\s+AccountsRenameRequest'
rg -n -C3 --type=rust 'UNIQUE|unique|accounts_get_by_name'Length of output: 35011
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== crates/wallet/sdk/src/apis/accounts.rs ==="
sed -n '1,420p' crates/wallet/sdk/src/apis/accounts.rs || true
echo
echo "=== applications/tari_walletd/src/handlers/accounts.rs ==="
sed -n '1,360p' applications/tari_walletd/src/handlers/accounts.rs || true
echo
echo "=== crates/wallet/sdk/src/storage.rs ==="
sed -n '1,260p' crates/wallet/sdk/src/storage.rs || true
echo
echo "=== crates/wallet/storage_sqlite/src/reader.rs (accounts_get_by_name) ==="
sed -n '380,460p' crates/wallet/storage_sqlite/src/reader.rs || true
echo
echo "=== crates/wallet/storage_sqlite/src/writer.rs (accounts_update) ==="
sed -n '1,320p' crates/wallet/storage_sqlite/src/writer.rs || true
echo
echo "=== crates/wallet/sdk/src/apis/errors.rs (if exists) & AccountsApiError in accounts.rs ==="
rg -n "enum\s+AccountsApiError" || true
rg -n "AccountsApiError" crates/wallet/sdk/src -n -S || trueLength of output: 56713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for accounts_update definitions/calls ==="
rg -n --type=rust '\bfn\s+accounts_update\b' || true
rg -n --type=rust '\baccounts_update\s*\(' || true
echo
echo "=== Show surrounding code for writer::accounts_update (if found) ==="
FILES=$(rg -n --type=rust '\bfn\s+accounts_update\b' -g 'crates/**' -g 'applications/**' -g '*/src/**' -S --no-line-number || true)
if [ -n "$FILES" ]; then
for f in $(echo "$FILES" | cut -d: -f1 | sort -u); do
echo "----- $f -----"
sed -n '1,240p' "$f" || true
echo
done
else
echo "No explicit fn accounts_update found; searching for trait method implementation 'accounts_update' in WalletStoreWriter impls..."
rg -n --type=rust 'accounts_update' -S || true
fi
echo
echo "=== Search for AccountUpdate struct/type ==="
rg -n --type=rust 'struct\s+AccountUpdate\b' || true
rg -n --type=rust 'pub struct\s+AccountUpdate\b' || true
rg -n --type=rust '\benum\s+AccountUpdate\b' || true
rg -n --type=rust '\bAccountUpdate\b' -S || true
echo
echo "=== Show models file(s) that define Account/AccountUpdate ==="
rg -n --type=rust 'struct\s+Account\b' -S || true
rg -n --type=rust 'mod models' -S || true
rg -n --hidden --no-ignore -g 'crates/**/src/**' 'AccountUpdate' -n -S || trueLength of output: 21275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== AccountUpdate struct ==="
sed -n '1,240p' crates/wallet/sdk/src/models/account.rs || true
echo
echo "=== storage_sqlite writer.accounts_update implementation ==="
sed -n '500,660p' crates/wallet/storage_sqlite/src/writer.rs || true
echo
echo "=== storage_sqlite models/account.rs ==="
sed -n '1,240p' crates/wallet/storage_sqlite/src/models/account.rs || true
echo
echo "=== Diesel schema for accounts (schema.rs) ==="
rg -n --hidden -S "table!.*accounts" || true
sed -n '1,240p' crates/wallet/storage_sqlite/src/schema.rs || true
echo
echo "=== storage_sqlite tests for accounts ==="
sed -n '1,240p' crates/wallet/storage_sqlite/tests/accounts.rs || trueLength of output: 16863
Reject empty/duplicate account names on rename (server-side)
rename_account (crates/wallet/sdk/src/apis/accounts.rs) calls accounts_update directly and does not validate new_name or check for existing names; the storage writer accepts empty/duplicate names. Add checks in rename_account to reject new_name.trim().is_empty() and to call tx.accounts_get_by_name(name).optional()? and return AccountsApiError::AccountNameAlreadyExists on duplicates. Update applications/tari_walletd/src/handlers/accounts.rs::handle_rename to map that error to an invalid_request (same mapping as create_account) so JSON‑RPC returns a clear client error.
🤖 Prompt for AI Agents
In applications/tari_walletd/src/jrpc_server.rs around line 146 (affects rename
flow), the server currently allows empty or duplicate account names on rename;
update the SDK method crates/wallet/sdk/src/apis/accounts.rs::rename_account to
validate new_name by rejecting new_name.trim().is_empty(), and before calling
accounts_update call tx.accounts_get_by_name(name).optional()? to detect
existing names and return AccountsApiError::AccountNameAlreadyExists on
duplicates; then update
applications/tari_walletd/src/handlers/accounts.rs::handle_rename to map
AccountsApiError::AccountNameAlreadyExists to an invalid_request JSON‑RPC error
the same way create_account does so the client receives a clear validation
error.
| pub fn rename_account(&self, account_addr: &ComponentAddress, new_name: &str) -> Result<(), AccountsApiError> { | ||
| let mut tx = self.store.create_write_tx()?; | ||
| tx.accounts_update(account_addr, AccountUpdate { | ||
| name: Some(new_name), | ||
| ..Default::default() | ||
| })?; | ||
| tx.commit()?; | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Prevent duplicate account names and validate input on rename.
Currently, rename does not check for an existing account with the same name. This can lead to duplicates or a DB constraint violation if a unique index exists. Also trim the new name and allow no-op when unchanged.
Apply this diff:
pub fn rename_account(&self, account_addr: &ComponentAddress, new_name: &str) -> Result<(), AccountsApiError> {
- let mut tx = self.store.create_write_tx()?;
- tx.accounts_update(account_addr, AccountUpdate {
- name: Some(new_name),
- ..Default::default()
- })?;
- tx.commit()?;
- Ok(())
+ let new_name = new_name.trim();
+ if new_name.is_empty() {
+ // Mirror other API patterns by surfacing a store error for bad input without adding a new variant
+ return Err(WalletStorageError::bad_query("accounts_rename", "new_name cannot be empty").into());
+ }
+
+ let mut tx = self.store.create_write_tx()?;
+ if let Some(existing) = tx.accounts_get_by_name(new_name).optional()? {
+ // Allow renaming to the same name on the same account, otherwise reject
+ if existing.component_address != *account_addr {
+ return Err(AccountsApiError::AccountNameAlreadyExists { name: new_name.to_string() });
+ }
+ }
+
+ tx.accounts_update(
+ account_addr,
+ AccountUpdate {
+ name: Some(new_name),
+ ..Default::default()
+ },
+ )?;
+ tx.commit()?;
+ Ok(())
}📝 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.
| pub fn rename_account(&self, account_addr: &ComponentAddress, new_name: &str) -> Result<(), AccountsApiError> { | |
| let mut tx = self.store.create_write_tx()?; | |
| tx.accounts_update(account_addr, AccountUpdate { | |
| name: Some(new_name), | |
| ..Default::default() | |
| })?; | |
| tx.commit()?; | |
| Ok(()) | |
| } | |
| pub fn rename_account(&self, account_addr: &ComponentAddress, new_name: &str) -> Result<(), AccountsApiError> { | |
| let new_name = new_name.trim(); | |
| if new_name.is_empty() { | |
| // Mirror other API patterns by surfacing a store error for bad input without adding a new variant | |
| return Err(WalletStorageError::bad_query("accounts_rename", "new_name cannot be empty").into()); | |
| } | |
| let mut tx = self.store.create_write_tx()?; | |
| if let Some(existing) = tx.accounts_get_by_name(new_name).optional()? { | |
| // Allow renaming to the same name on the same account, otherwise reject | |
| if existing.component_address != *account_addr { | |
| return Err(AccountsApiError::AccountNameAlreadyExists { name: new_name.to_string() }); | |
| } | |
| } | |
| tx.accounts_update( | |
| account_addr, | |
| AccountUpdate { | |
| name: Some(new_name), | |
| ..Default::default() | |
| }, | |
| )?; | |
| tx.commit()?; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In crates/wallet/sdk/src/apis/accounts.rs around lines 285 to 294, update
rename_account to trim the provided new_name, reject empty names after trimming,
treat renaming to the same trimmed name as a no-op (return Ok), and check the
store for any other account already using that trimmed name before applying the
update; if a duplicate exists return an appropriate AccountsApiError (e.g.,
DuplicateName or Conflict) instead of attempting the update. Ensure validation
happens before creating the write transaction and keep existing
transaction/commit logic for the actual update when all checks pass.
2 participants
Description
feat(wallet): add
accounts.renamecallMotivation and Context
Allows clients to rename the account.
How Has This Been Tested?
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Chores