Accept 72-char hex and strkey balance IDs in claim-claimable-balance and ledger entry fetch - #2683
Accept 72-char hex and strkey balance IDs in claim-claimable-balance and ledger entry fetch#2683Galmanus wants to merge 3 commits into
Conversation
…and ledger entry fetch `clawback-claimable-balance` accepts three balance-id formats via its `parse_balance_id` helper: the 64-char hex hash, the 72-char hex with type prefix returned by Horizon, and the B... strkey returned by `getTransaction`. `claim-claimable-balance` only did a raw hex decode with a 32-byte check, so the two formats users actually get from APIs were rejected. `ledger entry fetch claimable-balance --id` had the same restriction. Reuse `parse_balance_id` in both places. The fetch command keeps its pre-existing padded-hex fallback for short hex input. Fixes stellar#2451
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR expands claimable balance ID handling across tx and ledger entry fetch commands to accept multiple real-world formats (Horizon-prefixed hex and StrKey) and updates help/docs accordingly.
Changes:
- Accept 64-char hash hex, 72-char Horizon type-prefixed hex, and
B...StrKey formats for claimable balance IDs. - Reuse existing balance-id parsing logic from
clawback-claimable-balanceforclaim-claimable-balance. - Add regression tests and update help documentation to reflect supported formats.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/soroban-cli/src/commands/tx/new/claim_claimable_balance.rs | Switches claimable balance parsing to shared parser, adds helper + tests for all supported formats. |
| cmd/soroban-cli/src/commands/ledger/entry/fetch/claimable_balance.rs | Extends --id parsing to accept the same formats (while preserving padded-hex behavior) and adds tests. |
| FULL_HELP_DOCS.md | Updates CLI help text to document newly accepted ID formats. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fn claimable_balance_id(balance_id: &str) -> Result<xdr::ClaimableBalanceId, tx::args::Error> { | ||
| let balance_id_bytes = super::clawback_claimable_balance::parse_balance_id(balance_id)?; | ||
|
|
||
| let mut balance_id_array = [0u8; 32]; | ||
| balance_id_array.copy_from_slice(&balance_id_bytes); |
There was a problem hiding this comment.
Done in 9cfe8fb — replaced copy_from_slice with try_into() into [u8; 32], mapping a length mismatch to tx::args::Error::InvalidHex instead of panicking. In practice parse_balance_id returns 32 bytes, but this turns the invariant into a returned error rather than a process panic if it ever regresses.
| if let Ok(bytes) = crate::commands::tx::new::clawback_claimable_balance::parse_balance_id(x) { | ||
| return bytes.try_into().map_err(|_| Error::InvalidHash(x.into())); | ||
| } |
There was a problem hiding this comment.
Fair observation - parse_balance_id is a general balance-ID parser that happens to live in the clawback module. Two reasons I kept it here for this focused #2451 fix: (1) claim and clawback are both under tx::new, so that share is local (super::); only the ledger-fetch call crosses trees, and it deliberately discards the tx::args::Error and falls back to padded-hex, so there's no error-type coupling - just one pub fn call. (2) A proper shared util should also carry a neutral error type (it currently returns tx::args::Error, which is wrong outside tx args), and picking the home + error type feels like more than this bugfix should decide on its own. Happy to extract it into commands::tx::utils (or wherever you prefer) - in this PR or a follow-up - if you'd like it in scope.
| ###### **Options:** | ||
|
|
||
| - `--balance-id <BALANCE_ID>` — Balance ID of the claimable balance to claim (64-character hex string) | ||
| - `--balance-id <BALANCE_ID>` — Balance ID of the claimable balance to claim. Accepts multiple formats: - API format with type prefix (72 chars): 000000006f2179b31311fa8064760b48942c8e166702ba0b8fbe7358c4fd570421840461 - Direct hash format (64 chars): 6f2179b31311fa8064760b48942c8e166702ba0b8fbe7358c4fd570421840461 - Address format (base32): BAAMLBZI42AD52HKGIZOU7WFVZM6BPEJCLPL44QU2AT6TY3P57I5QDNYIA |
There was a problem hiding this comment.
Agreed the collapsed line reads poorly. Two things worth flagging before changing it: (1) this is the pre-existing rendering - clawback-claimable-balance already produces the identical collapsed line on main (FULL_HELP_DOCS.md around line 2327), and this PR intentionally makes claim consistent with it. (2) verbatim_doc_comment preserves newlines in --help, but the markdown generator emits each option as a single inline - + help entry, so embedded newlines land inside one list item rather than becoming nested bullets - it likely won't render as the clean list you're after. A proper fix (condense to one sentence, or teach the generator to emit sub-bullets) is best applied to claim and clawback together in a separate change so the two stay consistent. If you'd prefer it now, I can condense both help strings to a one-line form, e.g. (64-char hex, 72-char hex with type prefix, or B... strkey address).
Copilot review: copy_from_slice panics if parse_balance_id ever yields a slice whose length != 32. Use try_into into [u8; 32] and map a length mismatch to tx::args::Error::InvalidHex, so a bad input is a returned error rather than a process panic.
Fixes #2451.
clawback-claimable-balanceaccepts three balance-id formats via itsparse_balance_idhelper:00000000type prefixB...strkeygetTransactionclaim-claimable-balanceonly did a rawhex::decode+ 32-byte check, so the two formats users actually get from APIs were rejected.ledger entry fetch claimable-balance --idhad the same restriction.Fix
claim-claimable-balance: reuseparse_balance_id, and extract the op construction into a testableclaimable_balance_idhelper.ledger entry fetch claimable-balance: accept the same three formats, falling back to the pre-existing padded-hex behavior for other input so current usage keeps working.FULL_HELP_DOCS.mdregenerated (separate commit).Tests
InvalidHexbefore the fix), invalid inputs still rejected.short_hex_is_still_paddedcovering the preserved padded-hex fallback.