Skip to content

Commit 32ca6e5

Browse files
jferrantjferrant
committed
Code review comments
Signed-off-by: Jacinta Ferrant <[email protected]>
1 parent 272dd27 commit 32ca6e5

File tree

24 files changed

+412
-520
lines changed

24 files changed

+412
-520
lines changed

‎clarity/src/vm/tests/assets.rs

Lines changed: 26 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -155,24 +155,20 @@ fn test_native_stx_ops(epoch: StacksEpochId, mut env_factory: TopLevelMemoryEnvi
155155
let p2 = execute("'SM2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQVX8X0G");
156156
let p3 = execute("'SP3X6QWWETNBZWGBK6DRGTR1KX50S74D3433WDGJY");
157157

158-
let p1_std_principal_data = match p1 {
159-
Value::Principal(PrincipalData::Standard(ref data)) => data.clone(),
160-
_ => panic!(),
158+
let Value::Principal(PrincipalData::Standard(p1_std_principal_data)) = p1.clone() else {
159+
panic!("Expected standard principal data");
161160
};
162161

163-
let p1_principal = match p1 {
164-
Value::Principal(ref data) => data.clone(),
165-
_ => panic!(),
162+
let Value::Principal(p1_principal) = p1.clone() else {
163+
panic!("Expected principal data");
166164
};
167165

168-
let p2_principal = match p2 {
169-
Value::Principal(ref data) => data.clone(),
170-
_ => panic!(),
166+
let Value::Principal(p2_principal) = p2.clone() else {
167+
panic!("Expected principal data");
171168
};
172169

173-
let p3_principal = match p3 {
174-
Value::Principal(ref data) => data.clone(),
175-
_ => panic!(),
170+
let Value::Principal(p3_principal) = p3.clone() else {
171+
panic!("Expected principal data");
176172
};
177173

178174
let token_contract_id =
@@ -525,19 +521,16 @@ fn test_simple_token_system(
525521
let p1 = execute("'SZ2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQ9H6DPR");
526522
let p2 = execute("'SM2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQVX8X0G");
527523

528-
let p1_std_principal_data = match p1 {
529-
Value::Principal(PrincipalData::Standard(ref data)) => data.clone(),
530-
_ => panic!(),
524+
let Value::Principal(PrincipalData::Standard(p1_std_principal_data)) = p1.clone() else {
525+
panic!("Expected standard pincipal data");
531526
};
532527

533-
let p1_principal = match p1 {
534-
Value::Principal(ref data) => data.clone(),
535-
_ => panic!(),
528+
let Value::Principal(p1_principal) = p1.clone() else {
529+
panic!("Expected principal data");
536530
};
537531

538-
let p2_principal = match p2 {
539-
Value::Principal(ref data) => data.clone(),
540-
_ => panic!(),
532+
let Value::Principal(p2_principal) = p2.clone() else {
533+
panic!("Expected principal data");
541534
};
542535

543536
let token_contract_id =
@@ -836,14 +829,12 @@ fn test_total_supply(epoch: StacksEpochId, mut env_factory: TopLevelMemoryEnviro
836829

837830
let p1 = execute("'SZ2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQ9H6DPR");
838831

839-
let p1_std_principal_data = match p1 {
840-
Value::Principal(PrincipalData::Standard(ref data)) => data.clone(),
841-
_ => panic!(),
832+
let Value::Principal(PrincipalData::Standard(p1_std_principal_data)) = p1.clone() else {
833+
panic!("Expected standard principal data");
842834
};
843835

844-
let p1_principal = match p1 {
845-
Value::Principal(ref data) => data.clone(),
846-
_ => panic!(),
836+
let Value::Principal(p1_principal) = p1.clone() else {
837+
panic!("Expected principal data");
847838
};
848839

849840
let token_contract_id =
@@ -939,9 +930,8 @@ fn test_overlapping_nfts(
939930

940931
let p1 = execute("'SZ2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQ9H6DPR");
941932

942-
let p1_std_principal_data = match p1 {
943-
Value::Principal(PrincipalData::Standard(ref data)) => data.clone(),
944-
_ => panic!(),
933+
let Value::Principal(PrincipalData::Standard(p1_std_principal_data)) = p1 else {
934+
panic!("Expected standard principal data");
945935
};
946936

947937
let tokens_contract_id =
@@ -991,19 +981,16 @@ fn test_simple_naming_system(
991981
let p1 = execute("'SZ2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQ9H6DPR");
992982
let p2 = execute("'SM2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQVX8X0G");
993983

994-
let p1_std_principal_data = match p1 {
995-
Value::Principal(PrincipalData::Standard(ref data)) => data.clone(),
996-
_ => panic!(),
984+
let Value::Principal(PrincipalData::Standard(p1_std_principal_data)) = p1.clone() else {
985+
panic!("Expected standard principal data");
997986
};
998987

999-
let p1_principal = match p1 {
1000-
Value::Principal(ref data) => data.clone(),
1001-
_ => panic!(),
988+
let Value::Principal(p1_principal) = p1.clone() else {
989+
panic!("Expected principal data");
1002990
};
1003991

1004-
let p2_principal = match p2 {
1005-
Value::Principal(ref data) => data.clone(),
1006-
_ => panic!(),
992+
let Value::Principal(p2_principal) = p2.clone() else {
993+
panic!("Expected principal data");
1007994
};
1008995

1009996
let placeholder_context =

‎libsigner/src/tests/http.rs

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,10 @@ fn test_decode_http_request_err() {
8787
for (data, expected_err_type) in tests.iter() {
8888
let err = decode_http_request(data.as_bytes()).unwrap_err();
8989
match (err, expected_err_type) {
90-
(EventError::Deserialize(..), EventError::Deserialize(..)) => {}
91-
(EventError::MalformedRequest(..), EventError::MalformedRequest(..)) => {}
90+
(EventError::Deserialize(..), EventError::Deserialize(..))
91+
| (EventError::MalformedRequest(..), EventError::MalformedRequest(..)) => {}
9292
(x, y) => {
93-
error!("expected error mismatch: {:?} != {:?}", &y, &x);
94-
panic!();
93+
panic!("expected error mismatch: {x:?} != {y:?}");
9594
}
9695
}
9796
}
@@ -138,11 +137,10 @@ fn test_decode_http_response_err() {
138137
let err_type = decode_http_response(data.as_bytes()).unwrap_err();
139138
match (err_type, expected_err_type) {
140139
(RPCError::HttpError(x), RPCError::HttpError(y)) => assert_eq!(x, *y),
141-
(RPCError::Deserialize(_), RPCError::Deserialize(_)) => {}
142-
(RPCError::MalformedResponse(_), RPCError::MalformedResponse(_)) => {}
140+
(RPCError::Deserialize(_), RPCError::Deserialize(_))
141+
| (RPCError::MalformedResponse(_), RPCError::MalformedResponse(_)) => {}
143142
(x, y) => {
144-
error!("expected error mismatch: {:?} != {:?}", &y, &x);
145-
panic!();
143+
panic!("expected error mismatch: {y:?} != {x:?}");
146144
}
147145
}
148146
}

‎stackslib/src/burnchains/bitcoin/spv.rs

Lines changed: 19 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1517,20 +1517,16 @@ mod test {
15171517
assert_eq!(spv_client.read_block_headers(1, 10).unwrap(), headers);
15181518

15191519
// should fail
1520-
if let Err(btc_error::NoncontiguousHeader) =
1521-
spv_client.insert_block_headers_after(1, headers.clone())
1522-
{
1523-
} else {
1524-
panic!();
1525-
}
1520+
assert!(matches!(
1521+
spv_client.insert_block_headers_after(1, headers.clone()),
1522+
Err(btc_error::NoncontiguousHeader)
1523+
));
15261524

15271525
// should fail
1528-
if let Err(btc_error::NoncontiguousHeader) =
1529-
spv_client.insert_block_headers_after(9, headers.clone())
1530-
{
1531-
} else {
1532-
panic!();
1533-
}
1526+
assert!(matches!(
1527+
spv_client.insert_block_headers_after(9, headers.clone()),
1528+
Err(btc_error::NoncontiguousHeader)
1529+
));
15341530

15351531
spv_client.drop_headers(1).unwrap();
15361532
assert_eq!(
@@ -1642,12 +1638,10 @@ mod test {
16421638
.unwrap();
16431639

16441640
// should fail now, since there's a child to check
1645-
if let Err(btc_error::NoncontiguousHeader) =
1646-
spv_client.insert_block_headers_before(1, headers.clone())
1647-
{
1648-
} else {
1649-
panic!();
1650-
}
1641+
assert!(matches!(
1642+
spv_client.insert_block_headers_before(1, headers.clone()),
1643+
Err(btc_error::NoncontiguousHeader)
1644+
));
16511645

16521646
// should succeed
16531647
spv_client.insert_block_headers_before(9, headers).unwrap();
@@ -1769,10 +1763,13 @@ mod test {
17691763
];
17701764

17711765
// should fail
1772-
if let btc_error::InvalidPoW = spv_client.handle_headers(40317, bad_headers).unwrap_err() {
1773-
} else {
1774-
panic!("Bad PoW headers accepted");
1775-
}
1766+
assert!(
1767+
matches!(
1768+
spv_client.handle_headers(40317, bad_headers),
1769+
Err(btc_error::InvalidPoW)
1770+
),
1771+
"Bad PoW headers accepted"
1772+
);
17761773
}
17771774

17781775
#[test]

‎stackslib/src/burnchains/tests/mod.rs

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -897,11 +897,10 @@ fn verify_keys_accepted(node: &mut TestBurnchainNode, prev_keys: &[LeaderKeyRegi
897897
assert!(tx_opt.is_some());
898898

899899
let tx = tx_opt.unwrap();
900-
if let BlockstackOperationType::LeaderKeyRegister(op) = tx {
901-
assert_eq!(op, *key);
902-
} else {
903-
panic!();
904-
}
900+
let BlockstackOperationType::LeaderKeyRegister(op) = tx else {
901+
panic!("Expected key registration tx");
902+
};
903+
assert_eq!(op, *key);
905904
}
906905
}
907906

@@ -913,11 +912,10 @@ fn verify_commits_accepted(node: &TestBurnchainNode, next_block_commits: &[Leade
913912
assert!(tx_opt.is_some());
914913

915914
let tx = tx_opt.unwrap();
916-
if let BlockstackOperationType::LeaderBlockCommit(op) = tx {
917-
assert_eq!(op, *commit);
918-
} else {
919-
panic!();
920-
}
915+
let BlockstackOperationType::LeaderBlockCommit(op) = tx else {
916+
panic!("Expected leader block commit tx");
917+
};
918+
assert_eq!(op, *commit);
921919
}
922920
}
923921

‎stackslib/src/chainstate/nakamoto/coordinator/tests.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -977,7 +977,7 @@ fn block_info_tests(use_primary_testnet: bool) {
977977
tip_block_id: &StacksBlockId| {
978978
let contract_id = match version {
979979
ClarityVersion::Clarity1 => &clar1_contract_id,
980-
ClarityVersion::Clarity2 => panic!(),
980+
ClarityVersion::Clarity2 => panic!("Clarity2 not supported in this test"),
981981
ClarityVersion::Clarity3 => &clar3_contract_id,
982982
};
983983
peer.with_db_state(|sortdb, chainstate, _, _| {

‎stackslib/src/chainstate/nakamoto/tests/node.rs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -932,11 +932,7 @@ impl TestStacksNode {
932932
) {
933933
Ok(accepted) => accepted,
934934
Err(e) => {
935-
error!(
936-
"Failed to process nakamoto block: {:?}\n{:?}",
937-
&e, &nakamoto_block
938-
);
939-
panic!();
935+
panic!("Failed to process nakamoto block: {e:?}\n{nakamoto_block:?}");
940936
}
941937
}
942938
} else {

‎stackslib/src/chainstate/stacks/boot/pox_2_tests.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3872,7 +3872,13 @@ fn test_get_pox_addrs() {
38723872
assert!(reward_addrs.is_empty());
38733873
}
38743874

3875-
eprintln!("\ntenure: {tenure_id}\nreward cycle: {cur_reward_cycle}\nlockup_reward_cycle: {lockup_reward_cycle}\nmin-uSTX: {min_ustx}\naddrs: {reward_addrs:?}\ntotal_liquid_ustx: {total_liquid_ustx}\ntotal-stacked: {total_stacked}\n");
3875+
eprintln!("tenure: {tenure_id})");
3876+
eprintln!("reward cycle: {cur_reward_cycle}");
3877+
eprintln!("lockup_reward_cycle: {lockup_reward_cycle}");
3878+
eprintln!("min-uSTX: {min_ustx}");
3879+
eprintln!("addrs: {reward_addrs:?}");
3880+
eprintln!("ntotal_liquid_ustx: {total_liquid_ustx}");
3881+
eprintln!("total-stacked: {total_stacked}");
38763882

38773883
if cur_reward_cycle == lockup_reward_cycle.into() {
38783884
assert_eq!(reward_addrs.len(), 4);
@@ -4164,7 +4170,13 @@ fn test_stack_with_segwit() {
41644170
assert!(reward_addrs.is_empty());
41654171
}
41664172

4167-
eprintln!("\ntenure: {tenure_id}\nreward cycle: {cur_reward_cycle}\nlockup_reward_cycle: {lockup_reward_cycle}\nmin-uSTX: {min_ustx}\naddrs: {reward_addrs:?}\ntotal_liquid_ustx: {total_liquid_ustx}\ntotal-stacked: {total_stacked}\n");
4173+
eprintln!("tenure: {tenure_id}");
4174+
eprintln!("reward cycle: {cur_reward_cycle}");
4175+
eprintln!("lockup_reward_cycle: {lockup_reward_cycle}");
4176+
eprintln!("min-uSTX: {min_ustx}");
4177+
eprintln!("addrs: {reward_addrs:?}");
4178+
eprintln!("total_liquid_ustx: {total_liquid_ustx}");
4179+
eprintln!("total-stacked: {total_stacked}");
41684180

41694181
if cur_reward_cycle == lockup_reward_cycle.into() {
41704182
assert_eq!(reward_addrs.len(), 4);

‎stackslib/src/chainstate/stacks/db/blocks.rs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8866,12 +8866,11 @@ pub mod test {
88668866
assert!(poison_opt.is_some());
88678867

88688868
let poison = poison_opt.unwrap();
8869-
if let TransactionPayload::PoisonMicroblock(h1, h2) = poison {
8870-
assert_eq!(h2, forked_microblocks[num_mblocks / 2].header);
8871-
assert_eq!(h1, conflicting_microblock.header);
8872-
} else {
8873-
panic!();
8874-
}
8869+
let TransactionPayload::PoisonMicroblock(h1, h2) = poison else {
8870+
panic!("Unexpected poison type");
8871+
};
8872+
assert_eq!(h2, forked_microblocks[num_mblocks / 2].header);
8873+
assert_eq!(h1, conflicting_microblock.header);
88758874
}
88768875
}
88778876

‎stackslib/src/chainstate/stacks/db/transactions.rs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8365,11 +8365,10 @@ pub mod test {
83658365
ASTRules::PrecheckSize,
83668366
)
83678367
.unwrap_err();
8368-
if let Error::ClarityError(clarity_error::BadTransaction(msg)) = err {
8369-
assert!(msg.find("never seen in this fork").is_some());
8370-
} else {
8371-
panic!();
8372-
}
8368+
let Error::ClarityError(clarity_error::BadTransaction(msg)) = &err else {
8369+
panic!("Unexpected error type");
8370+
};
8371+
assert!(msg.find("never seen in this fork").is_some());
83738372
conn.commit_block();
83748373
}
83758374
}

‎stackslib/src/chainstate/stacks/index/marf.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1038,15 +1038,13 @@ impl<T: MarfTrieId> MARF<T> {
10381038
trace!("MARF::get_path({block_hash:?}) {path:?}");
10391039

10401040
// a NotFoundError _here_ means that a block didn't exist
1041-
storage.open_block(block_hash).map_err(|e| {
1042-
test_debug!("Failed to open block {block_hash:?}: {e:?}");
1043-
e
1041+
storage.open_block(block_hash).inspect_err(|_e| {
1042+
test_debug!("Failed to open block {block_hash:?}: {_e:?}");
10441043
})?;
10451044

10461045
// a NotFoundError _here_ means that the key doesn't exist in this view
1047-
let (cursor, node) = MARF::walk(storage, block_hash, path).map_err(|e| {
1046+
let (cursor, node) = MARF::walk(storage, block_hash, path).inspect_err(|e| {
10481047
trace!("Failed to look up key {block_hash:?} {path:?}: {e:?}");
1049-
e
10501048
})?;
10511049

10521050
// both of these get caught by get_by_key and turned into Ok(None)

0 commit comments

Comments
 (0)