GH-605: Teach the PayableScanner on tx retries #663
Conversation
… FailedPayable Table
…h_same_hash_are_present_in_the_input intermittently passes or fails
…_their_fingerprints_exist
…rprints_exist passes
…txs_were_found_in_db
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); |
There was a problem hiding this comment.
Bug: Variable Shadowing Causes Compilation Error
Duplicate let declarations for failed_payable_dao_factory and sent_payable_dao_factory shadow their initial assignments. This makes the original assignments redundant and causes a compilation error.
| fn cmp(&self, other: &Self) -> Ordering { | ||
| todo!() | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Enum Comparison Methods Panic
The ValidationStatus enum's PartialOrd and Ord implementations use todo!(). This causes runtime panics when comparison methods are called, leading to crashes in contexts requiring ordering.
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); |
There was a problem hiding this comment.
Bug: DAO Factory Overwriting Causes Memory Leak
The failed_payable_dao_factory and sent_payable_dao_factory are each declared twice. The first failed_payable_dao_factory instance is immediately overwritten and leaked, while the second sent_payable_dao_factory declaration overwrites the initial one. This duplication suggests a copy-paste error, resulting in unused resources and a memory leak.
| } | ||
| if failed > 0 { | ||
| self.insert_records_in_failed_payables(&batch_results.failed_txs); | ||
| } |
There was a problem hiding this comment.
Bug: Duplicate Transaction Records in Sent Payables
The PayableScanner is redundantly inserting successful transaction records into the sent_payable table during SentPayables message processing. This occurs in both new and retry scan handlers, even though these records are already handled by the BlockchainBridge.
Additional Locations (1)
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); |
There was a problem hiding this comment.
Bug: Factory Declaration Overwrite Error
Duplicate declarations for failed_payable_dao_factory and sent_payable_dao_factory appear to be a copy-paste error. This creates redundant factory instances and overwrites their initial assignments.
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); |
There was a problem hiding this comment.
Bug: Duplicate Factory Declarations Overwrite Instances
The failed_payable_dao_factory and sent_payable_dao_factory variables are declared twice consecutively. The second declaration for each immediately shadows and overwrites the first, causing the initially created factory instances to be dropped unused. This wastes resources and appears to be a copy-paste error.
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); |
There was a problem hiding this comment.
Bug: Duplicate Factory Declarations
The failed_payable_dao_factory and sent_payable_dao_factory variables are declared and assigned twice consecutively. The second declarations shadow the initial ones, making the first factory instances redundant and unused. This appears to be a copy-paste error.
| .map(|signable_tx_template| signable_tx_template.amount_in_wei) | ||
| .max() | ||
| .expect("there aren't any templates") | ||
| } |
There was a problem hiding this comment.
Bug: Empty Collection Handling Issues
When SignableTxTemplates is empty, the largest_amount() method panics, which could crash the application. Separately, nonce_range() returns (0, 0) for an empty collection, which is ambiguous since 0 is a valid nonce.
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let failed_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); | ||
| let sent_payable_dao_factory = Box::new(Accountant::dao_factory(data_directory)); |
There was a problem hiding this comment.
Bug: Duplicate Factory Declarations Cause Compilation Error
There are duplicate let bindings for failed_payable_dao_factory and sent_payable_dao_factory. This causes a compilation error and results in redundant factory creations, as the initial declarations are overwritten and unused.
| let pending_tx_hashes_opt = self.harvest_pending_payables(); | ||
| let failure_hashes_opt = self.harvest_unproven_failures(); | ||
| eprintln!("ph: {:?}", pending_tx_hashes_opt); | ||
| eprintln!("fail: {:?}", failure_hashes_opt); |
There was a problem hiding this comment.
Bug: Sensitive Data Exposure in Debug Statements
Debug eprintln! statements were accidentally committed, printing potentially sensitive transaction hash information to stderr in production.
Additional Locations (1)
…r PartialOrd and Ord
| match self.sent_payable_dao.insert_new_records(&msg.new_sent_txs) { | ||
| let sent_txs: BTreeSet<SentTx> = msg.new_sent_txs.iter().cloned().collect(); | ||
|
|
||
| match self.sent_payable_dao.insert_new_records(&sent_txs) { |
There was a problem hiding this comment.
Bug: Silent Deduplication Causes Data Loss
Converting Vec<SentTx> to BTreeSet<SentTx> before insertion silently deduplicates transactions. This can lead to data loss if legitimate duplicate entries need individual processing, or mask error conditions where duplicates should be explicitly flagged by insert_new_records.
Note
Implement a new templated New/Retry payable pipeline with batch submission and BTreeSet-backed DAOs, refactoring scanners, bridge/interfaces, and error/typing across the stack.
BatchResults(sent/failed txs); add detailed error typing (LocalPayableError,AppRpcErrorKind, etc.).Eitherpriced templates; add gas-price ceiling handling and transmission logging.BTreeSet, addOrd/PartialOrdimpls and deterministic ordering for tx/status types.FailedTxconversions and new retrieval/update helpers (e.g., by receiver addresses); enhancePayableDaoretrieval with conditions.join_with_commas/join_with_separatorandbtreeset!macro; remove old scanner utils.Written by Cursor Bugbot for commit 02d98fb. This will update automatically on new commits. Configure here.