Skip to content

Commit 45bb837

Browse files
ISTIFANUS-NISTIFANUS-N
authored
Merge pull request #221 from MerlinTheWhiz/fix/validation-management-code-duplication
fix: Code Duplication in Validator Management
2 parents 6f72012 + 87abacf commit 45bb837

File tree

6 files changed

+301
-106
lines changed

6 files changed

+301
-106
lines changed

‎contracts/teachlink/src/bft_consensus.rs‎

Lines changed: 8 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,8 @@ impl BFTConsensus {
6868
slashed_amount: 0,
6969
};
7070

71-
// Store validator data
72-
env.storage().instance().set(
73-
&crate::storage::DataKey::Validator(validator.clone()),
74-
&true,
75-
);
71+
// Store validator data and maintain list/flag via utilities
72+
crate::validator_utils::set_validator_flag(env, &validator, true);
7673
env.storage().instance().set(
7774
&crate::storage::DataKey::ValidatorMetadata(validator.clone()),
7875
&validator_info,
@@ -81,19 +78,7 @@ impl BFTConsensus {
8178
&crate::storage::DataKey::ValidatorStake(validator.clone()),
8279
&stake,
8380
);
84-
85-
// Maintain list
86-
let mut list: Vec<Address> = env
87-
.storage()
88-
.instance()
89-
.get(&crate::storage::VALIDATORS_LIST)
90-
.unwrap_or_else(|| Vec::new(env));
91-
if !list.contains(&validator) {
92-
list.push_back(validator.clone());
93-
env.storage()
94-
.instance()
95-
.set(&crate::storage::VALIDATORS_LIST, &list);
96-
}
81+
crate::validator_utils::add_validator_to_list(env, &validator);
9782

9883
// Update consensus state
9984
Self::update_consensus_state(env)?;
@@ -130,11 +115,8 @@ impl BFTConsensus {
130115
.get::<_, i128>(&crate::storage::DataKey::ValidatorStake(validator.clone()))
131116
.unwrap_or(0);
132117

133-
// Remove validator
134-
env.storage().instance().set(
135-
&crate::storage::DataKey::Validator(validator.clone()),
136-
&false,
137-
);
118+
// Remove validator and update list/flag via utilities
119+
crate::validator_utils::set_validator_flag(env, &validator, false);
138120
env.storage()
139121
.instance()
140122
.remove(&crate::storage::DataKey::ValidatorMetadata(
@@ -143,19 +125,7 @@ impl BFTConsensus {
143125
env.storage()
144126
.instance()
145127
.remove(&crate::storage::DataKey::ValidatorStake(validator.clone()));
146-
147-
// Remove from list
148-
let mut list: Vec<Address> = env
149-
.storage()
150-
.instance()
151-
.get(&crate::storage::VALIDATORS_LIST)
152-
.unwrap_or_else(|| Vec::new(env));
153-
if let Some(pos) = list.iter().position(|v| v == validator) {
154-
list.remove(pos as u32);
155-
env.storage()
156-
.instance()
157-
.set(&crate::storage::VALIDATORS_LIST, &list);
158-
}
128+
crate::validator_utils::remove_validator_from_list(env, &validator);
159129

160130
// Update consensus state
161131
Self::update_consensus_state(env)?;
@@ -345,42 +315,8 @@ impl BFTConsensus {
345315

346316
/// Update the consensus state based on current validators
347317
fn update_consensus_state(env: &Env) -> Result<(), BridgeError> {
348-
let validators: Vec<Address> = env
349-
.storage()
350-
.instance()
351-
.get(&crate::storage::VALIDATORS_LIST)
352-
.unwrap_or_else(|| Vec::new(env));
353-
354-
let mut total_stake: i128 = 0;
355-
let mut active_validators: u32 = 0;
356-
357-
for validator in validators.iter() {
358-
if Self::is_active_validator(env, &validator) {
359-
active_validators += 1;
360-
let stake = env
361-
.storage()
362-
.instance()
363-
.get(&crate::storage::DataKey::ValidatorStake(validator.clone()))
364-
.unwrap_or(0);
365-
total_stake += stake;
366-
}
367-
}
368-
369-
// Byzantine threshold: 2f+1 where n = 3f+1
370-
// For n validators, we need ceil(2n/3) + 1 for BFT
371-
let byzantine_threshold = if active_validators > 0 {
372-
((2 * active_validators) / 3) + 1
373-
} else {
374-
1
375-
};
376-
377-
let consensus_state = ConsensusState {
378-
total_stake,
379-
active_validators,
380-
byzantine_threshold,
381-
last_consensus_round: env.ledger().timestamp(),
382-
};
383-
318+
// Delegate computation to shared utility, then persist
319+
let consensus_state = crate::validator_utils::compute_consensus_state(env);
384320
env.storage()
385321
.instance()
386322
.set(&CONSENSUS_STATE, &consensus_state);

‎contracts/teachlink/src/bridge.rs‎

Lines changed: 6 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -371,23 +371,9 @@ impl Bridge {
371371
let admin: Address = env.storage().instance().get(&ADMIN).unwrap();
372372
admin.require_auth();
373373

374-
env.storage().instance().set(
375-
&crate::storage::DataKey::Validator(validator.clone()),
376-
&true,
377-
);
378-
379-
// Maintain list
380-
let mut list: Vec<Address> = env
381-
.storage()
382-
.instance()
383-
.get(&crate::storage::VALIDATORS_LIST)
384-
.unwrap_or_else(|| Vec::new(env));
385-
if !list.contains(&validator) {
386-
list.push_back(validator);
387-
env.storage()
388-
.instance()
389-
.set(&crate::storage::VALIDATORS_LIST, &list);
390-
}
374+
// Centralize validator flag/list maintenance
375+
crate::validator_utils::set_validator_flag(env, &validator, true);
376+
crate::validator_utils::add_validator_to_list(env, &validator);
391377

392378
Ok(())
393379
}
@@ -398,23 +384,9 @@ impl Bridge {
398384
let admin: Address = env.storage().instance().get(&ADMIN).unwrap();
399385
admin.require_auth();
400386

401-
env.storage().instance().set(
402-
&crate::storage::DataKey::Validator(validator.clone()),
403-
&false,
404-
);
405-
406-
// Remove from list
407-
let mut list: Vec<Address> = env
408-
.storage()
409-
.instance()
410-
.get(&crate::storage::VALIDATORS_LIST)
411-
.unwrap_or_else(|| Vec::new(env));
412-
if let Some(pos) = list.iter().position(|v| v == validator) {
413-
list.remove(pos as u32);
414-
env.storage()
415-
.instance()
416-
.set(&crate::storage::VALIDATORS_LIST, &list);
417-
}
387+
// Centralize validator flag/list maintenance
388+
crate::validator_utils::set_validator_flag(env, &validator, false);
389+
crate::validator_utils::remove_validator_from_list(env, &validator);
418390

419391
Ok(())
420392
}

‎contracts/teachlink/src/lib.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,12 @@ mod social_learning;
136136
mod storage;
137137
mod tokenization;
138138
mod types;
139+
pub mod validator_utils;
139140
pub mod validation;
141+
// Property-based tests are heavy and depend on dev-only crates. Only
142+
// compile them when the `proptest` feature is enabled to avoid pulling
143+
// these test-only dependencies into normal builds.
144+
#[cfg(feature = "proptest")]
140145
pub mod property_based_tests;
141146

142147
pub use crate::types::{

‎contracts/teachlink/src/validator_utils.rs‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
use crate::storage::{VALIDATORS_LIST, DataKey};
2+
use crate::types::ConsensusState;
3+
use soroban_sdk::{Address, Env, Vec};
4+
5+
/// Lightweight utilities for validator management to avoid duplicated logic
6+
/// across modules (bridge, bft_consensus, slashing, etc.).
7+
pub fn set_validator_flag(env: &Env, validator: &Address, active: bool) {
8+
env.storage()
9+
.instance()
10+
.set(&DataKey::Validator(validator.clone()), &active);
11+
}
12+
13+
pub fn add_validator_to_list(env: &Env, validator: &Address) {
14+
let mut list: Vec<Address> = env
15+
.storage()
16+
.instance()
17+
.get(&VALIDATORS_LIST)
18+
.unwrap_or_else(|| Vec::new(env));
19+
if !list.contains(validator) {
20+
list.push_back(validator.clone());
21+
env.storage().instance().set(&VALIDATORS_LIST, &list);
22+
}
23+
}
24+
25+
pub fn remove_validator_from_list(env: &Env, validator: &Address) {
26+
let mut list: Vec<Address> = env
27+
.storage()
28+
.instance()
29+
.get(&VALIDATORS_LIST)
30+
.unwrap_or_else(|| Vec::new(env));
31+
// iterator yields Address by value; compare against the dereferenced
32+
// `validator` to match types
33+
if let Some(pos) = list.iter().position(|v| v == *validator) {
34+
list.remove(pos as u32);
35+
env.storage().instance().set(&VALIDATORS_LIST, &list);
36+
}
37+
}
38+
39+
/// Returns the computed consensus state from current validators and stakes.
40+
/// This function is pure in terms of computation (reads storage) and returns
41+
/// a ConsensusState. Callers may persist it as needed.
42+
pub fn compute_consensus_state(env: &Env) -> ConsensusState {
43+
let validators: Vec<Address> = env
44+
.storage()
45+
.instance()
46+
.get(&VALIDATORS_LIST)
47+
.unwrap_or_else(|| Vec::new(env));
48+
49+
let mut total_stake: i128 = 0;
50+
let mut active_validators: u32 = 0;
51+
52+
for validator in validators.iter() {
53+
let is_active = env
54+
.storage()
55+
.instance()
56+
.get::<_, bool>(&DataKey::Validator(validator.clone()))
57+
.unwrap_or(false);
58+
if is_active {
59+
active_validators += 1;
60+
let stake = env
61+
.storage()
62+
.instance()
63+
.get::<_, i128>(&DataKey::ValidatorStake(validator.clone()))
64+
.unwrap_or(0);
65+
total_stake += stake;
66+
}
67+
}
68+
69+
let byzantine_threshold = if active_validators > 0 {
70+
((2 * active_validators) / 3) + 1
71+
} else {
72+
1
73+
};
74+
75+
ConsensusState {
76+
total_stake,
77+
active_validators,
78+
byzantine_threshold,
79+
last_consensus_round: env.ledger().timestamp(),
80+
}
81+
}
82+
83+
// =========================
84+
// Unit tests for utilities
85+
// =========================
86+
#[cfg(test)]
87+
mod tests {
88+
use super::*;
89+
use crate::types::ConsensusState;
90+
use soroban_sdk::testutils::Address as _;
91+
use soroban_sdk::{Env, Vec};
92+
93+
#[test]
94+
fn set_and_remove_validator_flag_and_list() {
95+
let env = Env::default();
96+
let validator = Address::generate(&env);
97+
98+
// start empty
99+
let list: Vec<Address> = env
100+
.storage()
101+
.instance()
102+
.get(&VALIDATORS_LIST)
103+
.unwrap_or_else(|| Vec::new(&env));
104+
assert_eq!(list.len(), 0);
105+
106+
// set active and add to list
107+
set_validator_flag(&env, &validator, true);
108+
add_validator_to_list(&env, &validator);
109+
110+
let flag: bool = env
111+
.storage()
112+
.instance()
113+
.get(&DataKey::Validator(validator.clone()))
114+
.unwrap_or(false);
115+
assert!(flag, "validator flag should be true");
116+
117+
let list: Vec<Address> = env
118+
.storage()
119+
.instance()
120+
.get(&VALIDATORS_LIST)
121+
.unwrap_or_else(|| Vec::new(&env));
122+
assert_eq!(list.len(), 1);
123+
124+
// remove
125+
set_validator_flag(&env, &validator, false);
126+
remove_validator_from_list(&env, &validator);
127+
128+
let flag2: bool = env
129+
.storage()
130+
.instance()
131+
.get(&DataKey::Validator(validator.clone()))
132+
.unwrap_or(false);
133+
assert!(!flag2, "validator flag should be false");
134+
135+
let list2: Vec<Address> = env
136+
.storage()
137+
.instance()
138+
.get(&VALIDATORS_LIST)
139+
.unwrap_or_else(|| Vec::new(&env));
140+
assert_eq!(list2.len(), 0);
141+
}
142+
143+
#[test]
144+
fn compute_consensus_state_counts_active_validators_and_stake() {
145+
let env = Env::default();
146+
let v1 = Address::generate(&env);
147+
let v2 = Address::generate(&env);
148+
149+
// prepare stakes and flags
150+
env.storage().instance().set(&DataKey::ValidatorStake(v1.clone()), &100i128);
151+
env.storage().instance().set(&DataKey::ValidatorStake(v2.clone()), &50i128);
152+
153+
set_validator_flag(&env, &v1, true);
154+
set_validator_flag(&env, &v2, false);
155+
156+
add_validator_to_list(&env, &v1);
157+
add_validator_to_list(&env, &v2);
158+
159+
let cs: ConsensusState = compute_consensus_state(&env);
160+
assert_eq!(cs.active_validators, 1);
161+
assert_eq!(cs.total_stake, 100i128);
162+
assert!(cs.byzantine_threshold >= 1);
163+
}
164+
}

0 commit comments

Comments
 (0)