Skip to content

Commit df515f8

Browse files
author
miller070
committed
refactor: dedup latest-non-deprecated loop, add missing tests, extract ZERO_ADDRESS_STR
a) router-registry: extract fn latest_non_deprecated helper to eliminate the byte-for-byte duplicate reverse-iteration loop that existed in both get_latest and the no-constraint branch of get_latest_with_constraint. Both callers now delegate to the single shared helper. b) router-access: add test_get_role_parent_returns_none_when_not_set to cover the baseline case that get_role_parent returns None for a role that was never passed to set_role_parent. Mirrors the existing test_get_role_admin_returns_none_when_not_set. c) router-access: add test_is_blacklisted_reflects_blacklist_state to directly exercise the is_blacklisted view function (false -> true after blacklist() -> false after unblacklist()), which was previously only covered indirectly through has_role tests. d) router-core: extract ZERO_ADDRESS_STR constant so the Stellar zero address literal is defined in one place and referenced by both register_route and register_route_internal.
1 parent c28197b commit df515f8

3 files changed

Lines changed: 76 additions & 33 deletions

File tree

contracts/router-access/src/lib.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1700,4 +1700,32 @@ mod tests {
17001700
assert_eq!(result, Err(Ok(AccessError::HierarchyCycle)));
17011701
assert_eq!(client.get_role_parent(&role_c), None);
17021702
}
1703+
1704+
#[test]
1705+
fn test_get_role_parent_returns_none_when_not_set() {
1706+
// Mirrors test_get_role_admin_returns_none_when_not_set for get_role_parent.
1707+
// A role that has never been passed to set_role_parent must report no parent.
1708+
let (env, _admin, client) = setup();
1709+
let role = String::from_str(&env, "orphan-role");
1710+
1711+
assert_eq!(client.get_role_parent(&role), None);
1712+
}
1713+
1714+
#[test]
1715+
fn test_is_blacklisted_reflects_blacklist_state() {
1716+
// Directly exercises is_blacklisted: false → true after blacklist() → false after unblacklist().
1717+
let (env, admin, client) = setup();
1718+
let addr = Address::generate(&env);
1719+
1720+
// Fresh address is not blacklisted
1721+
assert!(!client.is_blacklisted(&addr));
1722+
1723+
// After blacklisting it becomes true
1724+
client.blacklist(&admin, &addr);
1725+
assert!(client.is_blacklisted(&addr));
1726+
1727+
// After un-blacklisting it goes back to false
1728+
client.unblacklist(&admin, &addr);
1729+
assert!(!client.is_blacklisted(&addr));
1730+
}
17031731
}

contracts/router-core/src/lib.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,13 @@ pub enum RouterError {
189189
const MAX_RECURSION_DEPTH: u32 = 10;
190190
// ── Constants ─────────────────────────────────────────────────────────────────
191191

192+
/// The Stellar "zero" address — used as a sentinel for "no owner" / invalid address checks.
193+
///
194+
/// A single source-of-truth for the literal so that a typo in one validation
195+
/// point cannot silently diverge from another, and any future change only
196+
/// needs to be made here.
197+
const ZERO_ADDRESS_STR: &str = "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF";
198+
192199
/// Minimum remaining TTL (in ledgers) before instance storage is extended.
193200
/// ~30 days at 5 s/ledger.
194201
const INSTANCE_TTL_THRESHOLD: u32 = 17280 * 30;
@@ -281,7 +288,7 @@ impl RouterCore {
281288
// Validate address is not the zero address
282289
let zero_address = Address::from_string(&String::from_str(
283290
&env,
284-
"GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF",
291+
ZERO_ADDRESS_STR,
285292
));
286293
if address == zero_address {
287294
return Err(RouterError::InvalidAddress);
@@ -2168,7 +2175,7 @@ impl RouterCore {
21682175
// Validate address is not the zero address
21692176
let zero_address = Address::from_string(&String::from_str(
21702177
env,
2171-
"GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF",
2178+
ZERO_ADDRESS_STR,
21722179
));
21732180
if address == zero_address {
21742181
return Err(RouterError::InvalidAddress);

contracts/router-registry/src/lib.rs

Lines changed: 39 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -316,22 +316,7 @@ impl RouterRegistry {
316316
if versions.is_empty() {
317317
return Err(RegistryError::NotFound);
318318
}
319-
// Iterate in reverse to find latest non-deprecated
320-
let len = versions.len();
321-
let mut i = len;
322-
while i > 0 {
323-
i -= 1;
324-
let v = versions.get(i).ok_or(RegistryError::NotFound)?;
325-
let entry: ContractEntry = env
326-
.storage()
327-
.instance()
328-
.get(&DataKey::Entry(name.clone(), v))
329-
.ok_or(RegistryError::NotFound)?;
330-
if !entry.deprecated {
331-
return Ok(entry);
332-
}
333-
}
334-
Err(RegistryError::AllVersionsDeprecated)
319+
Self::latest_non_deprecated(&env, &name, &versions)
335320
}
336321

337322
/// Get the latest non-deprecated entry matching a semver constraint.
@@ -357,23 +342,12 @@ impl RouterRegistry {
357342
) -> Result<ContractEntry, RegistryError> {
358343
let versions = Self::get_versions_list(&env, &name);
359344

360-
// If no constraint, use get_latest logic
345+
// If no constraint, delegate to the shared helper (same semantics as get_latest)
361346
if constraint.is_none() {
362-
let len = versions.len();
363-
let mut i = len;
364-
while i > 0 {
365-
i -= 1;
366-
let v = versions.get(i).ok_or(RegistryError::NotFound)?;
367-
let entry: ContractEntry = env
368-
.storage()
369-
.instance()
370-
.get(&DataKey::Entry(name.clone(), v))
371-
.ok_or(RegistryError::NotFound)?;
372-
if !entry.deprecated {
373-
return Ok(entry);
374-
}
347+
if versions.is_empty() {
348+
return Err(RegistryError::NotFound);
375349
}
376-
return Err(RegistryError::AllVersionsDeprecated);
350+
return Self::latest_non_deprecated(&env, &name, &versions);
377351
}
378352

379353
let constraint_str = constraint.unwrap();
@@ -782,6 +756,40 @@ impl RouterRegistry {
782756

783757

784758

759+
/// Iterates `versions` in descending order and returns the first
760+
/// [`ContractEntry`] for `name` that is not deprecated.
761+
///
762+
/// This is the single shared implementation used by both
763+
/// [`get_latest`](Self::get_latest) and the "no constraint" branch of
764+
/// [`get_latest_with_constraint`](Self::get_latest_with_constraint).
765+
/// Any future change to "how we pick the latest non-deprecated version"
766+
/// only needs to be made here.
767+
///
768+
/// # Errors
769+
/// * [`RegistryError::NotFound`] — if a version index lookup fails.
770+
/// * [`RegistryError::AllVersionsDeprecated`] — if every version is deprecated.
771+
fn latest_non_deprecated(
772+
env: &Env,
773+
name: &String,
774+
versions: &Vec<u32>,
775+
) -> Result<ContractEntry, RegistryError> {
776+
let len = versions.len();
777+
let mut i = len;
778+
while i > 0 {
779+
i -= 1;
780+
let v = versions.get(i).ok_or(RegistryError::NotFound)?;
781+
let entry: ContractEntry = env
782+
.storage()
783+
.instance()
784+
.get(&DataKey::Entry(name.clone(), v))
785+
.ok_or(RegistryError::NotFound)?;
786+
if !entry.deprecated {
787+
return Ok(entry);
788+
}
789+
}
790+
Err(RegistryError::AllVersionsDeprecated)
791+
}
792+
785793
fn get_versions_list(env: &Env, name: &String) -> Vec<u32> {
786794
env.storage()
787795
.instance()

0 commit comments

Comments
 (0)