Skip to content

Conversation

@apfitzge
Copy link

Problem

  • scheduler-bindings project will want to return failure reasons and status to the external process
  • for transactions that didn't get committed we do not return the error in ExecuteAndCommitTransactionsOutput

Summary of Changes

  • CommitTransactionDetails:
    • Include the error if not committed
    • Include the result if committed

Fixes #

Comment on lines +90 to +92
result: committed_tx.status.clone(),
},
Err(_) => CommitTransactionDetails::NotCommitted,
Err(err) => CommitTransactionDetails::NotCommitted(err.clone()),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clones in most cases should be fast - 32 bytes.

only one variant is potentialy slow since it includes a String 🫠

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That String has been removed though, which reduces TransactionError size to 12 bytes 🥳

Will have to wait for sdk version bump to make it to gain the benefit.

anza-xyz/solana-sdk#12

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2025

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.1%. Comparing base (3aa0872) to head (65947a9).
⚠️ Report is 27 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #7246     +/-   ##
=========================================
- Coverage    83.1%    83.1%   -0.1%     
=========================================
  Files         850      849      -1     
  Lines      369948   369869     -79     
=========================================
- Hits       307707   307608     -99     
- Misses      62241    62261     +20     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

apfitzge added a commit to apfitzge/agave that referenced this pull request Jul 30, 2025
@apfitzge apfitzge marked this pull request as ready for review July 31, 2025 18:19
@apfitzge apfitzge requested a review from jstarry July 31, 2025 18:19
@apfitzge apfitzge merged commit ecd31c1 into anza-xyz:master Aug 1, 2025
41 checks passed
@apfitzge apfitzge deleted the commit_error_return branch August 1, 2025 17:58
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.

3 participants