Rename payouts failure file #81
Conversation
It makes more sense for it to be called SuccessfulPayouts when it contains the addresses that have been paid and not those which failed.
There was a problem hiding this comment.
I think we should probably give each of these cases different names - we can make it easier to understand exactly where the process failed AND give the operator better intuition about whether the transaction actually made it to a validator... what do you think?
cli/src/utils.rs
Outdated
| @@ -450,7 +451,7 @@ pub async fn propose_payout_batch( | |||
| Ok(message) => message, | |||
| Err(error) => { | |||
| let date = chrono::offset::Utc::now().to_string(); | |||
| let file_path = PathBuf::from(&format!("./FailedPayouts{}", date)); | |||
| let file_path = PathBuf::from(&format!("{}{}", FAILURE_FILE, date)); | |||
There was a problem hiding this comment.
it looks like this actually should be called something like "FailedPayouts" - the error happened trying to get a signature, so I think these payouts really wouldn't have gone forward.
I do think "FailedPayouts" implies that it contains all payouts that weren't done, which is also wrong! maybe this file should be called something like "SignatureFailedPayouts"?
cli/src/utils.rs
Outdated
| @@ -466,7 +467,7 @@ pub async fn propose_payout_batch( | |||
| Ok(mpool_push) => mpool_push, | |||
| Err(error) => { | |||
| let date = chrono::offset::Utc::now().to_string(); | |||
| let file_path = PathBuf::from(&format!("./FailedPayouts{}", date)); | |||
| let file_path = PathBuf::from(&format!("{}{}", FAILURE_FILE, date)); | |||
There was a problem hiding this comment.
similarly, if this failed I think the transaction never actually made it to the network - this is a nuanced failure, as the signature succeeded so theoretically there is a valid transaction somewhere - I'm unclear whether this could be a partial failure to push into the mempool (maybe one validator got the transaction and another didn't or something?) - probably worth calling this something specific too - MempoolPushFailedPayouts or something?
cli/src/utils.rs
Outdated
| @@ -1107,7 +1108,7 @@ pub async fn deploy_payout_batch<S: Middleware + 'static>( | |||
| Ok(gas) => gas, | |||
| Err(error) => { | |||
| let date = chrono::offset::Utc::now().to_string(); | |||
| let file_path = PathBuf::from(&format!("./FailedPayouts{}", date)); | |||
| let file_path = PathBuf::from(&format!("{}{}", FAILURE_FILE, date)); | |||
There was a problem hiding this comment.
this also seems like a real failure - if gas couldn't be estimated I think we would not send the transaction? probably a good idea for this to have a bespoke name too - maybe GasEstimateFailedPayouts ?
| @@ -1128,7 +1129,7 @@ pub async fn deploy_payout_batch<S: Middleware + 'static>( | |||
| Ok(receipt) => receipt, | |||
| Err(error) => { | |||
| let date = chrono::offset::Utc::now().to_string(); | |||
| let file_path = PathBuf::from(&format!("./FailedPayouts{}", date)); | |||
There was a problem hiding this comment.
ok if I had to guess this is the one we actually saw - the transaction was send but the receipt is an error for some reason? in this case it seems possible that the transaction did in fact get sent, but we ran into an error getting the receipt - seems like it might be possible that the transactions succeeded OR failed, so maybe yet another name - ReceiptFailedPayouts or something?
It's not being used so I'd rather hide it now that we have a private function wrapping it.
You make excellent points. Indeed all failures happen in different contexts. I took your suggestions and modified filenames accordingly. Also extracted a function for that to keep it DRY. Also, given that all failure filenames are going to be different it makes sense to sort them based on when they occurred, so I moved the timestamp before the filename. Wdyt? |
|
sounds great - I've approved, thanks! |
It makes more sense for the output file to be called
SuccessfulPayoutssince it contains the addresses that have been paid and not those which failed.I left everything else the same.