fix: change on_auth to return Result<(), PluginRejection> and fix error classification - #115
Open
alberto-crossmint wants to merge 5 commits into
Open
fix: change on_auth to return Result<(), PluginRejection> and fix error classification#115alberto-crossmint wants to merge 5 commits into
alberto-crossmint wants to merge 5 commits into
Conversation
The contractclient macro sets E = soroban_sdk::Error for void-returning functions like on_auth, so TryFrom<Error> for Error is the identity and ALL errors land in Err(Ok(soroban_sdk::Error)). The Err(Err(_)) arms were dead code. Replace the four-arm match with an inspection of Error.is_type(): - ScErrorType::Contract → deliberate rejection (panic_with_error! or contracterror Result::Err) — block authorization - Anything else → technical failure (bare panic!, missing function, host trap, budget exhaustion) — skip plugin This fixes a bug where a plugin using panic_with_error! with its own #[contracterror] type (code outside SmartAccountError) had its rejection silently ignored, and also a bug where bare panic! was incorrectly blocking auth via the dead Err(Ok(_)) catch-all. Tests added: - Plugin with custom #[contracterror] (code 200) blocks auth - Bare panic!() is skipped as technical failure - Missing on_auth callback is skipped as technical failure - Mixed plugins: crashing plugin doesn't block others
Change the plugin on_auth interface from returning () to Result<(), PluginRejection>. This is ABI backwards compatible: old plugins returning void produce the same wire format (Void) as new plugins returning Ok(()). The dedicated PluginRejection error type gives new plugins a clean rejection mechanism without relying on panic_with_error!. With E = PluginRejection (a real #[contracterror]), the InvokeError::Abort vs InvokeError::Contract distinction is now reachable and all five match arms are meaningful: - Ok(Ok(_)): approved (new or old-style) - Ok(Err(_)): ABI mismatch — skip - Err(Ok(_)): new-style Err(PluginRejection) — block - Err(Err(Contract)): old-style panic_with_error! — block (compat) - Err(Err(Abort)): technical failure — skip Add uninstall bypass: call_plugins_on_auth inspects auth contexts for an uninstall_plugin call and skips the target plugin so it cannot veto its own removal. This eliminates the permanent wallet-lock scenario regardless of error classification. Tests added: - New-style rejection blocks auth - Old-style panic_with_error! still blocks (backwards compat) - Old void-returning plugin still works (ABI compat) - Uninstall bypasses the rejecting plugin (escape hatch) - Uninstall bypass only skips the target, not other plugins
alberto-crossmint
changed the title
Remove the uninstall bypass logic — a plugin vetoing its own removal is a valid use case. Keep the on_auth -> Result<(), PluginRejection> interface change and the five-arm InvokeError match.
alberto-crossmint
changed the title
Add 6 new tests (14 total plugin tests) to verify that every old-style plugin pattern works correctly after on_auth changed to return Result<(), PluginRejection>: - test_try_on_auth_result_variants: calls try_on_auth directly on 9 different plugin types and asserts the exact Result variant (Ok(Ok), Err(Ok), Err(Err(Contract)), Err(Err(Abort))) - test_old_void_plugin_with_storage_and_events_works: void plugin that writes storage and emits events — verifies side effects run - test_old_result_ok_plugin_works: plugin returning Result::Ok(()) - test_old_result_err_plugin_blocks_auth: plugin returning Result::Err - test_old_panic_with_known_error_blocks_auth: panic_with_error! with SmartAccountError (the exact pre-#115 RejectingPlugin pattern) - test_mixed_old_and_new_plugins_both_approve: three plugins (void, Result::Ok, void+storage) installed together all pass auth
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
1 participant
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.
Summary
Fixes plugin error classification in
call_plugins_on_authby changing theon_authinterface and replacing the dead-code match arms.Problems solved
Dead-code match arms —
on_authreturned(), so the#[contractclient]macro setE = soroban_sdk::Error. SinceTryFrom<Error> for Erroris the identity, all errors landed inErr(Ok(_))and theInvokeError::Abort/InvokeError::Contractarms were unreachable.Custom error rejections ignored — A plugin using
panic_with_error!with its own#[contracterror]type had its rejection silently skipped.How it's fixed
on_authnow returnsResult<(), PluginRejection>— ABI backwards compatible becauseOk(())produces the same wire format (Void) as the old()return. Old deployed plugins continue working without recompilation.With
E = PluginRejection(a real#[contracterror]), all five match arms are now reachable:Ok(Ok(_))Ok(Err(_))Err(Ok(_))Err(PluginRejection::Rejected)Err(Err(Contract(_)))panic_with_error!(backwards compat)Err(Err(Abort))Test plan
14 plugin tests covering every old-style plugin pattern:
test_try_on_auth_result_variantstry_on_authResultvariant for eachtest_old_void_plugin_still_works()returntest_old_void_plugin_with_storage_and_events_works()with storage writes + eventstest_old_result_ok_plugin_worksResult<(), Error>returningOktest_old_result_err_plugin_blocks_authResult<(), Error>returningErrtest_old_panic_with_known_error_blocks_authpanic_with_error!(SmartAccountError)test_plugin_old_style_rejection_still_blockspanic_with_error!(CustomError)code 200test_plugin_new_style_rejection_blocks_authErr(PluginRejection::Rejected)test_plugin_bare_panic_skippedpanic!("crashed")test_plugin_missing_on_auth_skippedon_authfunctiontest_mixed_plugins_crash_does_not_blocktest_mixed_old_and_new_plugins_both_approvetest_uninstall_plugin_persists_removaltest_max_plugins_limitcargo test --workspace— all 167 tests passcargo fmt --check— clean