-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tool Scrubber Graduation PR for V6 #1174
base: v6
Are you sure you want to change the base?
Conversation
@@ -309,15 +309,22 @@ pub fn write_account_json( | |||
let keys = KeyScheme::new(&wallet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write_account_json should return Result
@@ -9,7 +9,7 @@ use crate::prelude::app_config; | |||
use diem_types::transaction::SignedTransaction; | |||
use diem_wallet::WalletLibrary; | |||
use ol_types::{account::ValConfigs, pay_instruction::PayInstruction}; | |||
use std::path::PathBuf; | |||
use std::{path::PathBuf, process::exit}; | |||
|
|||
/// Creates an account.json file for the validator | |||
pub fn write_manifest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should return Result
@@ -41,6 +41,11 @@ impl Runnable for ValConfigCmd { | |||
None, | |||
); | |||
|
|||
let val_cfg = match val_cfg_res { | |||
Ok(cfg) => cfg, | |||
Err(error) => panic!("Could not create validator config: {:?}", error), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove panic! for exit()
@@ -85,12 +85,11 @@ impl ValConfigs { | |||
vfn_ip_address: Ipv4Addr, | |||
autopay_instructions: Option<Vec<PayInstruction>>, | |||
autopay_signed: Option<Vec<SignedTransaction>>, | |||
) -> Self { | |||
) -> Result<ValConfigs, anyhow::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result<ValConfigs...
Result<Self...
@@ -197,8 +197,8 @@ impl PayInstruction { | |||
uid = &self.uid.unwrap(), | |||
percent_balance = *&self.value_move.unwrap() as f64 /100f64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another opportunity to refactor.
@@ -197,8 +197,8 @@ impl PayInstruction { | |||
uid = &self.uid.unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opportunity
@@ -197,8 +197,8 @@ impl PayInstruction { | |||
uid = &self.uid.unwrap(), | |||
percent_balance = *&self.value_move.unwrap() as f64 /100f64, | |||
times = times, | |||
note = &self.note.clone().unwrap(), | |||
epoch_ending = &self.end_epoch.unwrap(), | |||
note = if let Some(n) = &self.note { n } else {""}, // the note is optional for the message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.unwrap_or("");
// let mut config_toml = path.to_str().unwrap().to_owned()).expect("could not parse app config from file"); | ||
|
||
let cfg_path = match path { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.unwrap_or(dirs::home_dir().expect("WTF where's home").join(".0L").join("0L.toml"));
// this unwrap is likely safe in this case. nothing to recover from.
I refactored 10 unwrap commands and 10 clone commands in various files in the 0l folder. I noticed some automatically updated binaries from the folder "DPNFramework" made it into this PR, too, maybe due to my test activities. I did not make code changes in those. I hope that's not a problem.
Motivation
Tool Scrubber Graduation
Test Plan
I complied, spun up a node and executed the shuffle e2e test. The expected result of the first test passing and the remaining 3 tests failing was observed:
running 4 tests from ./e2e/message.test.ts
Test Assert ...
------- output -------
{ chain_id: 4, ledger_version: "96", ledger_timestamp: "1664728533581067" }
----- output end -----
Test Assert ... ok (14ms)
Ability to set message ... FAILED (38ms)
Ability to set NFTs ... FAILED (11ms)
Advanced: Ability to set message from nonpublishing account ... FAILED (8ms)