Skip to content

fix(solana/macros): enforce exact return types in #[ibc_app] macro#1012

Open
mariuszzak wants to merge 2 commits intomainfrom
mariuszzak/33-fix-ibc-app-macro-return-type-validation
Open

fix(solana/macros): enforce exact return types in #[ibc_app] macro#1012
mariuszzak wants to merge 2 commits intomainfrom
mariuszzak/33-fix-ibc-app-macro-return-type-validation

Conversation

@mariuszzak
Copy link
Collaborator

Description

  • The #[ibc_app] macro's return type validation only checked that callbacks returned Result<_> without inspecting the inner type. This allowed on_recv_packet(...) -> Result<()> to compile but fail at runtime with
    InvalidAppResponse since the router CPI helper expects Vec<u8> return data.
  • Now is_result_vec_u8 and is_result_unit actually inspect the generic argument inside Result<T>.
  • on_recv_packet only accepts Result<Vec<u8>>, on_acknowledgement_packet and on_timeout_packet only accept Result<()>.
  • Fixed a pre-existing quote! bug in the validate_message_parameter error message.
  • Updated doc comments to list required return types explicitly.
  • Added 24 unit tests for type-checking utilities and 17 trybuild compile-fail tests covering all macro validation paths.

ref: https://github.com/zenith-security/2026-02-cosmos-labs/issues/33#issuecomment-4125162539


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Wrote unit and integration tests.
  • Added relevant natspec and godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

The `is_result_vec_u8` and `is_result_unit` validation functions both
delegated to `matches_result_type` which only checked that the outer
type was `Result<_>` without inspecting the inner type parameter. This
allowed `on_recv_packet(...) -> Result<()>` to pass the macro at
compile time while failing at runtime with `InvalidAppResponse` because
the CPI helper unconditionally expects `Vec<u8>` return data.

- Implement proper inner-type inspection via `extract_result_inner_type`
- Remove `ResultUnit` from `on_recv_packet` allowed return types
- Fix pre-existing `quote!` bug in `validate_message_parameter` error
- Update doc comments to list required return types explicitly
- Add 24 unit tests for type-checking utilities
- Add 17 trybuild compile-fail tests covering all validation paths
@mariuszzak mariuszzak self-assigned this Mar 25, 2026
@mariuszzak mariuszzak requested a review from srdtrk as a code owner March 25, 2026 14:18
Each callback now has exactly one allowed return type, so replace
the `&[ReturnTypeExpectation]` slice with a plain `ReturnTypeExpectation`
value and remove the `.any()` iteration and multi-type formatting logic.
@codecov
Copy link

codecov bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.91%. Comparing base (06999ca) to head (501b338).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1012   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files          27       27           
  Lines        1123     1123           
=======================================
  Hits         1122     1122           
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant