Return transaction status from all bank process tx methods#6561
Return transaction status from all bank process tx methods#6561jstarry merged 1 commit intoanza-xyz:masterfrom
Conversation
jstarry
force-pushed
the
refactor/bank-process-tx
branch
from
2b2fa7b to
6aacc73
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6561 +/- ##
========================================
Coverage 82.8% 82.8%
========================================
Files 849 849
Lines 379196 379205 +9
========================================
+ Hits 314029 314140 +111
+ Misses 65167 65065 -102 🚀 New features to boost your workflow:
|
jstarry
force-pushed
the
refactor/bank-process-tx
branch
4 times, most recently
from
412b9ff to
a90fed1
Compare
jstarry
changed the title
jstarry
force-pushed
the
refactor/bank-process-tx
branch
from
a90fed1 to
934c270
Compare
jstarry
changed the title
| .0 | ||
| .into_iter() | ||
| .map(|commit_result| commit_result.map(|_| ())) | ||
| .map(|commit_result| commit_result.and_then(|committed_tx| committed_tx.status)) |
There was a problem hiding this comment.
Changing this revealed a few tests that were processing transactions unsuccessfully. I think it makes sense to always bubble up the execution status.
| vote_state::create_account(&vote_pubkey1, &authorized_voter.pubkey(), 0, 100); | ||
| let vote_account2 = | ||
| vote_state::create_account(&vote_pubkey2, &authorized_voter.pubkey(), 0, 100); | ||
| let vote_account0 = vote_state::create_account_with_authorized( |
There was a problem hiding this comment.
This test doesn't technically require all of the vote transactions to succeed, but I thought it was worth cleaning up all the issues causing them to fail. One issue with this test was that vote_state::create_account sets the authorized voter as the vote account itself. So I updated to create_account_with_authorized to set the authorized voter to authorized_voter as I assume was intended by the original author.
steviez
left a comment
steviez
left a comment
There was a problem hiding this comment.
Cleanup generally looks good, just a couple questions to confirm my understanding
| /// Process a Transaction. This is used for unit tests and simply calls the vector | ||
| /// Bank::process_transactions method. | ||
| pub fn process_transaction(&self, tx: &Transaction) -> Result<()> { | ||
| self.try_process_transactions(std::iter::once(tx))?[0].clone()?; |
There was a problem hiding this comment.
Out of scope for this PR, but given the "This is used for unit tests and ..." comment, this should seemingly be DCOU
3 participants
Problem
The transaction status isn't returned consistently from bank process transaction methods.
Summary of Changes
Bank::process_transactionFixes #