fix: structured error instead of panic on non-authorizer auth entry addresses - #2695
Open
Galmanus wants to merge 1 commit into
Open
fix: structured error instead of panic on non-authorizer auth entry addresses#2695Galmanus wants to merge 1 commit into
Galmanus wants to merge 1 commit into
Conversation
… auth entry addresses An auth entry whose credential address is a muxed account, claimable balance, or liquidity pool crashed the CLI with a raw `todo!()` panic and a backtrace. These ScAddress variants are values, not valid authorizers, so signing now fails with a diagnosable error naming the address kind and its strkey. Also refreshes the resolve_secret comment that pointed at the removed `todo!`. Part of stellar#2534
Contributor
There was a problem hiding this comment.
Pull request overview
Replaces panic paths for unsupported Soroban authorization addresses with structured errors.
Changes:
- Adds
UnsupportedAuthAddresshandling for three address variants. - Adds regression tests and updates related documentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cmd/soroban-cli/src/signer/mod.rs |
Returns structured errors and adds tests. |
cmd/soroban-cli/src/config/address.rs |
Updates muxed-account signing commentary. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+671
to
+694
| async fn assert_unsupported_auth_address(address: ScAddress, expected_kind: &str) { | ||
| let signer = local_signer([1u8; 32]); | ||
| let source = MuxedAccount::Ed25519(Uint256([9u8; 32])); | ||
| let contract = [42u8; 32]; | ||
|
|
||
| let entry = address_auth(address, invocation(contract, "hello")); | ||
| let host_fn = HostFunction::InvokeContract(invoke_args(contract, "hello")); | ||
| let tx = build_tx(source, host_fn, vec![entry]); | ||
|
|
||
| let res = sign_soroban_authorizations( | ||
| &tx, | ||
| &[signer], | ||
| EXPIRATION_LEDGER, | ||
| NETWORK, | ||
| false, | ||
| &Print::new(true), | ||
| ) | ||
| .await; | ||
|
|
||
| match res { | ||
| Err(Error::UnsupportedAuthAddress { kind, .. }) => assert_eq!(kind, expected_kind), | ||
| other => panic!("expected UnsupportedAuthAddress error, got: {other:?}"), | ||
| } | ||
| } |
Comment on lines
+88
to
91
| // when nothing matches. Muxed accounts (`M...`) carry no secret of | ||
| // their own and are not valid authorizers (see | ||
| // `Error::UnsupportedAuthAddress` in `sign_soroban_authorizations`), | ||
| // so they keep returning `CannotSign`. |
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
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.
What
sign_soroban_authorizationshad threetodo!()arms — muxed account, claimable balance, and liquidity pool credential addresses — so any transaction carrying such an auth entry crashed the CLI with a raw panic and backtrace. They now return a structuredError::UnsupportedAuthAddressnaming the address kind and its strkey.Why
Part of #2534. Per the ruling in that issue's discussion, muxed accounts are not valid authorizers (confirmed there with dmkozh), so the actionable half of the issue is turning the panics into diagnosable errors — the error text says these addresses "cannot authorize", not "not yet supported". Also refreshes the
resolve_secretcomment inconfig/address.rsthat pointed at the removedtodo!.Note: PR #2547 attempted this earlier but modifies
Dockerfile/entrypoint.sh/docker/README.md, which were removed from main by #2616, and has been conflicting since May. This is a minimal replacement (2 files, no unrelated changes); happy to close in its favor if it gets rebased.Testing
Three new unit tests (
test_{muxed_account,claimable_balance,liquidity_pool}_auth_address_errors_instead_of_panicking) build a transaction with an auth entry for each address kind and assert the structured error — the regression being guarded is "errors, does not panic".cargo test -p soroban-cli --lib signer::: 25 passed.cargo clippyandcargo fmt --checkclean.