Uh oh!
There was an error while loading. Please reload this page.
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
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.
Uh oh!
There was an error while loading. Please reload this page.
| 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.
Uh oh!
There was an error while loading. Please reload this page.
…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. Fixesstellar#2451
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.
9cfe8fb to
1351fefCompare| .try_into() | ||
| .map_err(|_| Error::InvalidHash(x.clone()))?; | ||
| let hash = Hash(hash_bytes); | ||
| let hash = Hash(parse_id(x)?); |
| Ok(xdr::OperationBody::ClaimClaimableBalance( | ||
| xdr::ClaimClaimableBalanceOp { | ||
| balance_id: claimable_balance_id, | ||
| balance_id: claimable_balance_id(balance_id)?, |
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.