GH-662: Clean-up of the obsolete pending payable param in PayableAccount and financials command fix with some SQL work #715
Conversation
| a tx to the blockchain, and the tx is pending. | ||
| 2. `pendingTxHashOpt` is not null and `failures` is greater than 0. This means that we've made more than one attempt. | ||
| There is an alive pending tx, plus we've experienced X failures before that. | ||
| 3. `pendingTxHashOpt` is null and `failures` is greater than 0. This means that the payment mechanism is in a mid-state |
There was a problem hiding this comment.
This case is bit confusing, we should always have a transaction hash, as it is generated locally.
| "0x0290db1d56121112f4d45c1c3f36348644f6afd20b759b762f1dba9c4949066e" | ||
| .to_string(), | ||
| balance_gwei: 999888777, | ||
| tx_processing_info_opt: Some(TxProcessingInfo{pending_tx_hash_opt: Some("0x0290db1d56121112f4d45c1c3f36348644f6afd20b759b762f1dba9c4949066e".to_string()),failures: 0} |
There was a problem hiding this comment.
According to me, the inner Some shouldn't exist, tx hash should be a compulsory value.
| ) | ||
| } | ||
| }, | ||
| tx_processing_info_opt: account_with_tx_info.tx_opt, |
There was a problem hiding this comment.
Names could be consistent here.
| if gwei > 0 { | ||
| gwei | ||
| } else { | ||
| panic!( |
There was a problem hiding this comment.
This panic could be abstracted if you use a TryFrom conversion here, it'll look cleaner if you do that way.
| gwei | ||
| } else { | ||
| panic!( | ||
| "Broken code: ReceivableAccount with balance between {} and 0 gwei \ |
There was a problem hiding this comment.
You may prefer to use the same code for conversion. If you decide to do so then prefer this piece of code over the other.
| pending_payable_opt: None, | ||
| PayableAccountWithTxInfo { | ||
| account: PayableAccount { | ||
| wallet: make_wallet("ac3665c"), |
There was a problem hiding this comment.
Optional: Prefer Using Address over Wallet. (I know it's a time consuming change, that's why I kept it as optional)
| account: PayableAccount { | ||
| wallet: make_wallet("abcd123"), | ||
| balance_wei: 58_568_686_005, | ||
| last_paid_timestamp: SystemTime::now().sub(Duration::from_secs(5000)), |
There was a problem hiding this comment.
Timestamp should be an u64, instead of SystemTime because precision here is useless.
utkarshg6
left a comment
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/db_access_objects/payable_dao.rs
| }) | ||
| } | ||
| }, | ||
| tx_opt: Self::maybe_construct_tx_info(tx_hash_opt, previous_failures), |
There was a problem hiding this comment.
This function is not a good idea, there is a lot of uncertainty. In this whole PR, these Optional values are my biggest concern, prefer using absolute values so that guaranteed values are returned.
utkarshg6
left a comment
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/db_access_objects/payable_dao.rs:367
| {}, | ||
| {} | ||
| {}", | ||
| "SELECT |
There was a problem hiding this comment.
Please add a comment at the top of this fn saying:
// A dynamic SQL query that joins payable, sent_payable, and failed_payable
// to provide comprehensive information about accounts that need to be paid,
// including their current transaction processing status.This comment only explains what this SQL does, but it's still a mystery Why it exists?
The Why is a more important question to answer here. So, please write an explanation why you wrote this code.
| WHERE nonce = (SELECT MAX(nonce) FROM nonces_of_failures_by_wallet) | ||
| ) | ||
| ELSE 0 | ||
| END AS recent_failures_count |
There was a problem hiding this comment.
You can replace the entire EXISTS clause with this code:
(
SELECT COUNT(*)
FROM failed_payable
WHERE receiver_address = p.wallet_address
AND status NOT LIKE '%Concluded%'
AND nonce = (
SELECT MAX(nonce)
FROM failed_payable
WHERE receiver_address = p.wallet_address
AND status NOT LIKE '%Concluded%'
)
) AS recent_failures_countIt'll automatically return 0 if no cases match.
| CASE WHEN s.status LIKE '%Pending%' | ||
| THEN s.tx_hash | ||
| ELSE NULL | ||
| END AS pending_tx_hash, |
There was a problem hiding this comment.
Always fetch the tx_hash and store the transaction status in a boolean called is_pending. Using NULL can cause confusion about whether the hash was ever present or removed conditionally. Since NULL values can lead to complications in SQL, it's best to avoid them.
You may use this code below:
s.tx_hash AS pending_tx_hash,
CASE WHEN s.status LIKE '%Pending%'
THEN 1
ELSE 0
END AS is_pending
utkarshg6
left a comment
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/db_access_objects/payable_dao.rs:871
| let mut sent_tx_2 = make_sent_tx(345); | ||
| sent_tx_2.receiver_address = Wallet::new(wallet_addr).address(); | ||
| let sent_tx_hash_2 = sent_tx_2.hash; | ||
| let mut failed_tx_1 = make_failed_tx(123); |
There was a problem hiding this comment.
Optional: Call for a builder pattern here (everywhere in this test tbh, but it's up to you).
| assert_eq!( | ||
| result, | ||
| vec
No description provided.