Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for the z_shieldcoinbase JSON-RPC method in the wallet build, enabling shielding of coinbase transparent UTXOs via the async operation framework.
Changes:
- Introduces
z_shieldcoinbaseRPC implementation (proposal construction, key selection, transaction build + broadcast). - Exposes
z_shieldcoinbaseon theWalletRpctrait and wires it into the server implementation. - Adds/updates RPC docs/parameter descriptions for OpenRPC/help generation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
zallet/src/components/json_rpc/methods/z_shieldcoinbase.rs |
New z_shieldcoinbase RPC implementation using propose_shielding + async op runner. |
zallet/src/components/json_rpc/methods.rs |
Registers the new module and adds the WalletRpc::z_shieldcoinbase method + server wiring and docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix misleading error when toaddress lookup fails: provide a specific
error message mentioning toaddress instead of inheriting the generic
"Invalid from address" from get_account_for_address.
- Fix feature gate: change #[cfg(feature = "transparent-key-import")]
to #[cfg(feature = "zcashd-import")] for the standalone key import
block and its zcash_script import. decrypt_standalone_transparent_key
is only defined behind zcashd-import, and transparent-key-import does
not imply zcashd-import.
- Propagate errors from decrypt_standalone_transparent_key instead of
silently swallowing them, matching z_send_many's error handling
pattern.
- Update RPC docs and parameter descriptions to accurately reflect
the implementation:
- toaddress must be wallet-owned and is used to select the account;
funds go to the account's internal shielded address.
- fromaddress must belong to the same account as toaddress.
- fee must be null if provided (ZIP-317 is always used).
- limit and memo are accepted for compatibility but currently ignored.
- Note that behavior differs from zcashd.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let transparent_addr = match from_address { | ||
| Address::Transparent(addr) => addr, | ||
| _ => { | ||
| return Err(LegacyCode::InvalidAddressOrKey.with_static( | ||
| "Invalid from address: z_shieldcoinbase only supports transparent source addresses.", | ||
| )); | ||
| } | ||
| }; |
There was a problem hiding this comment.
fromaddress parsing only accepts Address::Transparent(_), but elsewhere in the codebase Address::Tex(_) is treated as a transparent address too (e.g. transparent recipients in z_send_many). If callers can supply a TEX-form transparent address here, this will be rejected incorrectly. Consider accepting both Address::Transparent(addr) and Address::Tex(addr) (or otherwise documenting that TEX addresses are unsupported for this RPC).
| /// - `fee` (optional): If provided, this parameter must be `null`; any | ||
| /// non-null value will be rejected. The value of this field is ignored, | ||
| /// as Zallet always calculates and applies the ZIP-317 fee internally. |
There was a problem hiding this comment.
The fee parameter docs are a bit contradictory: it says “any non-null value will be rejected” and also that “the value of this field is ignored”. Since non-null values error, only null/omitted is accepted (and then ignored). Consider rewording to clearly state that the field must be omitted or null, and is otherwise rejected.
| /// - `fee` (optional): If provided, this parameter must be `null`; any | |
| /// non-null value will be rejected. The value of this field is ignored, | |
| /// as Zallet always calculates and applies the ZIP-317 fee internally. | |
| /// - `fee` (optional): This parameter must be omitted or set to `null`; | |
| /// any other value will be rejected. When accepted, the value of this | |
| /// field is ignored, as Zallet always calculates and applies the ZIP-317 | |
| /// fee internally. |
| // Derive the spending key for the account. | ||
| let derivation = account.source().key_derivation().ok_or_else(|| { | ||
| LegacyCode::InvalidAddressOrKey | ||
| .with_static("Invalid address, no payment source found for account.") | ||
| })?; | ||
|
|
||
| // Fetch spending key last, to avoid a keystore decryption if unnecessary. | ||
| let seed = keystore | ||
| .decrypt_seed(derivation.seed_fingerprint()) | ||
| .await | ||
| .map_err(|e| match e.kind() { | ||
| // TODO: Improve internal error types. | ||
| // https://github.com/zcash/wallet/issues/256 | ||
| crate::error::ErrorKind::Generic if e.to_string() == "Wallet is locked" => { | ||
| LegacyCode::WalletUnlockNeeded.with_message(e.to_string()) | ||
| } | ||
| _ => LegacyCode::Database.with_message(e.to_string()), | ||
| })?; | ||
| let usk = UnifiedSpendingKey::from_seed( | ||
| wallet.params(), | ||
| seed.expose_secret(), | ||
| derivation.account_index(), | ||
| ) | ||
| .map_err(|e| LegacyCode::InvalidAddressOrKey.with_message(e.to_string()))?; |
There was a problem hiding this comment.
This block repeats a lot of the same transaction-construction plumbing as z_send_many (Orchard action-limit loop, seed/USK derivation, optional standalone transparent key loading, and create_proposed_transactions + broadcast). Consider extracting shared helpers to reduce drift risk when error mapping or limits handling change.
| let account = get_account_for_address(wallet.as_ref(), &to_address).map_err(|_| { | ||
| LegacyCode::InvalidAddressOrKey | ||
| .with_static("Invalid parameter: toaddress does not belong to this wallet.") |
There was a problem hiding this comment.
get_account_for_address(...) errors are being collapsed into InvalidAddressOrKey via map_err(|_| ...), which will also mask real database failures (it can return LegacyCode::Database). Consider only translating the specific “address not found” case while propagating other errors unchanged, so operational issues aren’t reported as an invalid toaddress.
| let account = get_account_for_address(wallet.as_ref(), &to_address).map_err(|_| { | |
| LegacyCode::InvalidAddressOrKey | |
| .with_static("Invalid parameter: toaddress does not belong to this wallet.") | |
| let account = get_account_for_address(wallet.as_ref(), &to_address).map_err(|e| { | |
| match e { | |
| LegacyCode::InvalidAddressOrKey => LegacyCode::InvalidAddressOrKey | |
| .with_static("Invalid parameter: toaddress does not belong to this wallet."), | |
| other => other, | |
| } |
…coinbase-only shielding Updates z_shieldcoinbase to use the new TransparentOutputFilter enum from librustzcash to correctly restrict UTXO selection to coinbase-only outputs, matching the behavior of zcashd's z_shieldcoinbase RPC method. Changes: - Update librustzcash dependencies from pinned rev to feat/propose_shielding-parameters branch to get TransparentOutputFilter support. - Import TransparentOutputFilter in z_shieldcoinbase.rs and pass TransparentOutputFilter::CoinbaseOnly to propose_shielding to ensure only coinbase UTXOs are selected for shielding. - Update other transparent output call sites (connection.rs, list_unspent.rs) to pass TransparentOutputFilter::All since they handle general UTXO listing. - Update trait implementation in connection.rs to use TransparentKeyOrigin instead of TransparentKeyScope (API change in librustzcash). - Fix pre-existing bug in z_send_many.rs where Payment::new error handling was using ok_or_else instead of map_err on a Result type.
COR-264
Fixes #89