fix(soroban): stop declaring an error the registry cannot return - #72
Merged
alex-predicate merged 3 commits intoAug 27, 2026
Merged
Conversation
…D-013) RegistryError::InvalidSignature could never be returned. A failed ed25519 verification traps: the host builds a HostError (soroban-env-host crypto/mod.rs) and soroban-sdk discards it (`let _ = ...`), so a contract never sees the failure as a value. The declared error told integrators to write handling that could not fire, and validate_attestation's Result<bool, RegistryError> compounded it by implying an Ok(false) that never existed either. Removes the variant from RegistryError and from predicate-client's mirror of it, and narrows the return type to Result<(), RegistryError>. Discriminant 8 is left reserved rather than renumbered: renumbering would silently change the meaning of error codes already observed on-chain, which is a worse break than a gap. validate_attestation now documents both failure paths, since callers have to handle a typed error and an abort. The audit's fourth recommendation — map a failed verification onto InvalidSignature — is not available in soroban-sdk 23.5.3, which exposes no fallible ed25519 API. The only route would be a self-call through try_invoke_contract to catch the trap, paying a cross-contract invocation on every validation for an error code integrators cannot act on differently. Documented instead. test_invalid_signature_aborts_and_leaves_uuid_unspent pins both halves of the new documentation: try_validate_attestation returns Err(Err(InvokeError::Abort)) rather than a contract error, which is the mismatch itself, and the same uuid still validates afterwards with a correct signature. That second property comes from the host rolling back the invocation rather than from check ordering. Verified the test fails when the forged signature is made genuine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comments explained the code by contrast with earlier shapes of it — a boolean return that no longer exists, a network passphrase in the digest preimage, tokens deployed before the constructor registered their policy. Nothing integrates with this registry yet, so there is no history for a reader to need, and the audit ticket ids are no more use to them than the history is. Two consequences of there being no integrations, beyond wording: RegistryError keeps no reserved gap at 8. Nothing observes these codes on-chain yet, so nothing would misread a renumbered one, and NotInitialized simply takes 8. AlreadyInitialized is removed. It was never constructed — __constructor runs once at deploy, so it is unreachable — which is the same defect as declaring an InvalidSignature the host never lets the contract return. No test asserts codes 9 or 10, and 1-7 are unchanged.
douglasmakey
left a comment
douglasmakey
left a comment
There was a problem hiding this comment.
Can we review the code comments.
The registry and client carried roughly 360 lines of comment for 400 lines of code: numbered step markers above each guard clause, section banners, and doc comments that repeated the function name back. Reading validate meant reading past a summary of itself. What remains states something the code cannot: units and encodings on Statement fields, why the digest binds the caller rather than the statement's target, that a failed ed25519 verification traps instead of returning, that the golden-vector constants must never be regenerated from hash_statement, and the trust boundary integrators have to respect. RegistryError keeps its per-variant lines because Soroban puts them in the contract spec. verify-golden-vector-detects-drift.sh anchored one of its mutations on a deleted comment. It failed loudly rather than reporting a mutation it had not applied; the anchor is now code-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
douglasmakey
approved these changes
Aug 26, 2026
alex-predicate
deleted the
alex-predicate/soroban-find-013-invalid-signature-abi
branch
August 27, 2026 16:17
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
2 participants
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.
RegistryError::InvalidSignature could never be returned. A failed ed25519 verification traps: the host builds a HostError (soroban-env-host crypto/mod.rs) and soroban-sdk discards it (
let _ = ...), so a contract never sees the failure as a value. The declared error told integrators to write handling that could not fire, and validate_attestation's Result<bool, RegistryError> compounded it by implying an Ok(false) that never existed either.Removes the variant from RegistryError and from predicate-client's mirror of it, and narrows the return type to Result<(), RegistryError>. Discriminant 8 is left reserved rather than renumbered: renumbering would silently change the meaning of error codes already observed on-chain, which is a worse break than a gap. validate_attestation now documents both failure paths, since callers have to handle a typed error and an abort.