Skip to content

Make use of StoredTransactionError when converting between TransactionError and generated::TransactionError#6704

Open
steveluscher wants to merge 1 commit intoanza-xyz:masterfrom
steveluscher:actually-use-stored-transaction-error
Open

Make use of StoredTransactionError when converting between TransactionError and generated::TransactionError#6704
steveluscher wants to merge 1 commit intoanza-xyz:masterfrom
steveluscher:actually-use-stored-transaction-error

Conversation

@steveluscher
Copy link
Copy Markdown

Problem

This is a follow-on from #6434 in which I wrote a newtype to help facilitate serialization/deserialization between TransactionError and generated::TransactionError, and wrote tests for it, but then forgot to actually use it.

Summary of Changes

Replace manual deseriailze/serialize calls in the code that converts TransactionError object for storage in blockstore with calls to into().

@steveluscher
Copy link
Copy Markdown
Author

steveluscher commented Aug 28, 2025

@joncinque ping for assist (re: #6434 (review))

@steveluscher steveluscher added the automerge automerge Merge this Pull Request automatically once CI passes label Aug 28, 2025
@steveluscher steveluscher requested a review from joncinque August 28, 2025 19:14
Copy link
Copy Markdown

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

This looks correct to me, but I'm not familiar with every place that converts between stored and sdk types.

Have you also looked at StoredTransactionStatusMeta as mentioned in the previous PR? Its status field is currently a Result<(), TransactionError>:

pub status: Result<()>,

Based on its usage in blockstore.rs, I'm not sure if it's supposed to be deprecated or only for older versions of the software:

.get_raw_protobuf_or_bincode::<StoredTransactionStatusMeta>(

@steviez do you know what StoredTransactionStatusMeta is for? It looks to be deprecated, so maybe we shouldn't touch it, out of fear of breaking old ledgers. On the other hand, will any modern ledgers ever contain that type anymore?

@steveluscher
Copy link
Copy Markdown
Author

Ping, @steviez?

@github-actions
Copy link
Copy Markdown

This pull request is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

…tionError` and `generated::TransactionError`
@joncinque joncinque force-pushed the actually-use-stored-transaction-error branch from 2a9b75d to 925aa78 Compare March 24, 2026 17:14
@anza-team anza-team removed the automerge automerge Merge this Pull Request automatically once CI passes label Mar 24, 2026
@anza-team
Copy link
Copy Markdown
Collaborator

😱 New commits were pushed while the automerge label was present.

@joncinque joncinque added the CI Pull Request is ready to enter CI label Mar 24, 2026
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 24, 2026
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.3%. Comparing base (d55741a) to head (925aa78).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6704   +/-   ##
=======================================
  Coverage    83.3%    83.3%           
=======================================
  Files         845      845           
  Lines      319955   319951    -4     
=======================================
+ Hits       266772   266778    +6     
+ Misses      53183    53173   -10     
🚀 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants