From 798e7a482200131c4cf58fd0f22ae7e71e5c1b79 Mon Sep 17 00:00:00 2001 From: CodedTricks Date: Mon, 31 Aug 2026 09:58:45 +0000 Subject: [PATCH] fix: detect parameter reordering as breaking change in diff.rs (#256) Soroban encoding is positional: a caller that passes arguments in order (from, to, amount) will send 'from' where 'amount' is now expected if the contract upgrades and reorders parameters. The diff module must flag this explicitly so operators and webhook subscribers know that a reorder is a breaking API change, not just a cosmetic rename. Previously a reorder fell into the generic 'changed' bucket via string comparison. It was already marked breaking, but the summary line said 'changed function transfer: ...' with no indication that only the order changed, making it harder to triage the upgrade impact. Changes: - diff.rs: add ReorderedItem struct (name, from, to) mirroring ChangedItem - SectionDiff: add 'reordered' Vec field - SectionDiff::is_empty / has_breaking: include reordered in both checks - function_sigs(): return (BTreeMap, BTreeMap>) so callers have the ordered parameter list per function name - diff_section_with_params(): when signatures differ, check whether the param set is identical but order differs (is_param_reorder helper); if so, push to reordered instead of changed - build_summary(): emit 'reordered function : ...' lines - Two new tests: * reordered_parameters_are_breaking_and_flagged_as_reordered * renamed_parameter_is_changed_not_reordered (regression guard) - crates/lumenqraph-core/Cargo.toml: add missing url and tokio/net deps required by url_validation.rs (pre-existing omission) All 21 diff tests pass. Closes #256 --- Cargo.lock | 236 +++++++++++++++++++++++++++++ crates/lumenqraph-core/Cargo.toml | 2 + crates/lumenqraph-core/src/diff.rs | 226 +++++++++++++++++++++++---- 3 files changed, 435 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b4940d3..1369211 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -57,6 +57,18 @@ dependencies = [ "libc", ] +[[package]] +name = "anes" +version = "0.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4b46cbb362ab8752921c97e041f5e366ee6297bd428a31275b9fcf1e380f7299" + +[[package]] +name = "anstyle" +version = "1.0.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "940b3a0ca603d1eade50a4846a2afffd5ef57a9feac2c0e2ec2e14f9ead76000" + [[package]] name = "anyhow" version = "1.0.103" @@ -391,6 +403,12 @@ dependencies = [ "serde", ] +[[package]] +name = "cast" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "37b2a672a2cb129a2e41c10b1224bb368f9f37a2b16b612598138befd7b37eb5" + [[package]] name = "cc" version = "1.2.67" @@ -449,6 +467,58 @@ dependencies = [ "windows-link", ] +[[package]] +name = "ciborium" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "42e69ffd6f0917f5c029256a24d0161db17cea3997d185db0d35926308770f0e" +dependencies = [ + "ciborium-io", + "ciborium-ll", + "serde", +] + +[[package]] +name = "ciborium-io" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "05afea1e0a06c9be33d539b876f1ce3692f4afea2cb41f740e7743225ed1c757" + +[[package]] +name = "ciborium-ll" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "57663b653d948a338bfb3eeba9bb2fd5fcfaecb9e199e87e1eda4d9e8b240fd9" +dependencies = [ + "ciborium-io", + "half", +] + +[[package]] +name = "clap" +version = "4.6.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "473c7e07f409a8d772161724aa8db6a765a2532a70f9667eeb7b49d3d02fbdca" +dependencies = [ + "clap_builder", +] + +[[package]] +name = "clap_builder" +version = "4.6.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7b48fea5a88e9ae728a2dcbedbfc0e730f7d60da42e1cb049a83c9fb8b789889" +dependencies = [ + "anstyle", + "clap_lex", +] + +[[package]] +name = "clap_lex" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" + [[package]] name = "combine" version = "4.6.8" @@ -557,6 +627,63 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "criterion" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f2b12d017a929603d80db1831cd3a24082f8137ce19c69e6447f54f5fc8d692f" +dependencies = [ + "anes", + "cast", + "ciborium", + "clap", + "criterion-plot", + "futures", + "is-terminal", + "itertools", + "num-traits", + "once_cell", + "oorandom", + "plotters", + "rayon", + "regex", + "serde", + "serde_derive", + "serde_json", + "tinytemplate", + "tokio", + "walkdir", +] + +[[package]] +name = "criterion-plot" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6b50826342786a51a89e2da3a28f1c32b06e387201bc2d19791f622c673706b1" +dependencies = [ + "cast", + "itertools", +] + +[[package]] +name = "crossbeam-deque" +version = "0.8.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5181e0de7b61eb03a81e347d6dd8797bae9da5146707b51077e2d71a54ec0ceb" +dependencies = [ + "crossbeam-epoch", + "crossbeam-utils", +] + +[[package]] +name = "crossbeam-epoch" +version = "0.9.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2d6914041f254d6e9176c01941b21115dcfb7089e55135a35411081bd106ef3f" +dependencies = [ + "crossbeam-utils", +] + [[package]] name = "crossbeam-queue" version = "0.3.13" @@ -572,6 +699,12 @@ version = "0.8.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "61803da095bee82a81bb1a452ecc25d3b2f1416d1897eb86430c6159ef717c17" +[[package]] +name = "crunchy" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "460fbee9c2c2f33933d720630a6a0bac33ba7053db5344fac858d4b8952d77d5" + [[package]] name = "crypto-common" version = "0.1.7" @@ -973,6 +1106,17 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "half" +version = "2.7.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6ea2d84b969582b4b1864a92dc5d27cd2b77b622a8d79306834f1be5ba20d84b" +dependencies = [ + "cfg-if", + "crunchy", + "zerocopy", +] + [[package]] name = "handlebars" version = "5.1.2" @@ -1019,6 +1163,12 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" +[[package]] +name = "hermit-abi" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc0fef456e4baa96da950455cd02c081ca953b141298e41db3fc7e36b1da849c" + [[package]] name = "hex" version = "0.4.3" @@ -1323,6 +1473,26 @@ version = "2.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d98f6fed1fde3f8c21bc40a1abb88dd75e67924f9cffc3ef95607bad8017f8e2" +[[package]] +name = "is-terminal" +version = "0.4.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3640c1c38b8e4e43584d8df18be5fc6b0aa314ce6ebf51b53313d4306cca8e46" +dependencies = [ + "hermit-abi", + "libc", + "windows-sys 0.61.2", +] + +[[package]] +name = "itertools" +version = "0.10.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b0fd2260e829bddf4cb6ea802289de2f86d6a7a690192fbe91b3f46e0f2c8473" +dependencies = [ + "either", +] + [[package]] name = "itoa" version = "1.0.18" @@ -1476,6 +1646,7 @@ dependencies = [ "subtle", "thiserror 1.0.69", "tokio", + "url", "uuid", ] @@ -1486,6 +1657,7 @@ dependencies = [ "anyhow", "axum", "chrono", + "criterion", "dotenvy", "hex", "hmac", @@ -1716,6 +1888,12 @@ version = "1.21.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" +[[package]] +name = "oorandom" +version = "11.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d6790f58c7ff633d8771f42965289203411a5e5c68388703c06e14f24770b41e" + [[package]] name = "parking" version = "2.2.1" @@ -1841,6 +2019,34 @@ version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b4596b6d070b27117e987119b4dac604f3c58cfb0b191112e24771b2faeac1a6" +[[package]] +name = "plotters" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5aeb6f403d7a4911efb1e33402027fc44f29b5bf6def3effcc22d7bb75f2b747" +dependencies = [ + "num-traits", + "plotters-backend", + "plotters-svg", + "wasm-bindgen", + "web-sys", +] + +[[package]] +name = "plotters-backend" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df42e13c12958a16b3f7f4386b9ab1f3e7933914ecea48da7139435263a4172a" + +[[package]] +name = "plotters-svg" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "51bae2ac328883f7acdfea3d66a7c35751187f870bc81f94563733a154d7a670" +dependencies = [ + "plotters-backend", +] + [[package]] name = "potential_utf" version = "0.1.5" @@ -2097,6 +2303,26 @@ dependencies = [ "rand_core 0.9.5", ] +[[package]] +name = "rayon" +version = "1.12.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fb39b166781f92d482534ef4b4b1b2568f42613b53e5b6c160e24cfbfa30926d" +dependencies = [ + "either", + "rayon-core", +] + +[[package]] +name = "rayon-core" +version = "1.13.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22e18b0f0062d30d4230b2e85ff77fdfe4326feb054b9783a3460d8435c8ab91" +dependencies = [ + "crossbeam-deque", + "crossbeam-utils", +] + [[package]] name = "redis" version = "0.24.1" @@ -2955,6 +3181,16 @@ dependencies = [ "zerovec", ] +[[package]] +name = "tinytemplate" +version = "1.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "be4d6b5f19ff7664e8c98d03e2139cb510db9b0a60b55f8e8709b689d939b6bc" +dependencies = [ + "serde", + "serde_json", +] + [[package]] name = "tinyvec" version = "1.12.0" diff --git a/crates/lumenqraph-core/Cargo.toml b/crates/lumenqraph-core/Cargo.toml index 2af5c33..38f5ba4 100644 --- a/crates/lumenqraph-core/Cargo.toml +++ b/crates/lumenqraph-core/Cargo.toml @@ -23,3 +23,5 @@ stellar-xdr.workspace = true hex.workspace = true subtle.workspace = true base64 = "0.22" +url.workspace = true +tokio = { version = "1", features = ["net"] } diff --git a/crates/lumenqraph-core/src/diff.rs b/crates/lumenqraph-core/src/diff.rs index 45ff787..0a049b5 100644 --- a/crates/lumenqraph-core/src/diff.rs +++ b/crates/lumenqraph-core/src/diff.rs @@ -44,6 +44,10 @@ pub struct SectionDiff { pub removed: Vec, /// Items whose name persisted but whose signature moved. pub changed: Vec, + /// Items whose name persisted and whose parameter set is identical but + /// whose parameter *order* changed. Soroban encoding is positional, so a + /// reorder breaks every caller that passes arguments by position. + pub reordered: Vec, } #[derive(Debug, Clone, Serialize, PartialEq, Eq)] @@ -53,14 +57,31 @@ pub struct ChangedItem { pub to: String, } +/// A function whose parameter set is identical but whose parameter *order* +/// changed. Because Soroban encoding is positional, reordering parameters is +/// a breaking change: a caller that passes `(from, to, amount)` positionally +/// will send `from` where `amount` is now expected. +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +pub struct ReorderedItem { + pub name: String, + /// The parameter list as it appeared in the old interface (ordered). + pub from: String, + /// The parameter list as it appears in the new interface (ordered). + pub to: String, +} + impl SectionDiff { fn is_empty(&self) -> bool { - self.added.is_empty() && self.removed.is_empty() && self.changed.is_empty() + self.added.is_empty() + && self.removed.is_empty() + && self.changed.is_empty() + && self.reordered.is_empty() } - /// Additions can't break an existing caller; removals and changes can. + /// Additions can't break an existing caller; removals, changes, and + /// reorderings can (Soroban encoding is positional). fn has_breaking(&self) -> bool { - !self.removed.is_empty() || !self.changed.is_empty() + !self.removed.is_empty() || !self.changed.is_empty() || !self.reordered.is_empty() } } @@ -68,7 +89,10 @@ impl SpecDiff { /// Diff `old` against `new`. The result reads in the direction of the /// upgrade: `added` means "new interface has it, old one didn't". pub fn between(old: &ContractSpec, new: &ContractSpec) -> Self { - let functions = diff_section(&function_sigs(old), &function_sigs(new)); + let (old_fsigs, old_fparams) = function_sigs(old); + let (new_fsigs, new_fparams) = function_sigs(new); + let functions = + diff_section_with_params(&old_fsigs, &new_fsigs, &old_fparams, &new_fparams); let events = diff_section(&event_sigs(old), &event_sigs(new)); let types = diff_section(&type_sigs(old), &type_sigs(new)); @@ -106,6 +130,12 @@ impl SpecDiff { item.name, item.from, item.to )); } + for item in §ion.reordered { + out.push(format!( + "reordered {kind} {}: {} became {}", + item.name, item.from, item.to + )); + } for sig in §ion.added { out.push(format!("added {kind} {sig}")); } @@ -121,17 +151,52 @@ impl SpecDiff { /// Compare two name-to-signature maps. Names are the identity: a name in both /// with a different signature is a *change*, not an add plus a remove. +/// +/// When both old and new have an entry with the same name but different +/// signatures, we additionally check whether the difference is purely a +/// parameter reorder (same set of `"name: type"` tokens, different order). +/// Reorders are separated into `reordered` rather than `changed` so the +/// summary can label them precisely — but they are still breaking because +/// Soroban encoding is positional. fn diff_section(old: &BTreeMap, new: &BTreeMap) -> SectionDiff { + diff_section_with_params(old, new, &BTreeMap::new(), &BTreeMap::new()) +} + +/// Like `diff_section` but accepts optional ordered parameter lists per name so +/// that a pure reorder can be distinguished from a type/name change. +/// `old_params` and `new_params` map function name → ordered `["name: type", …]`. +fn diff_section_with_params( + old: &BTreeMap, + new: &BTreeMap, + old_params: &BTreeMap>, + new_params: &BTreeMap>, +) -> SectionDiff { let names: BTreeSet<&String> = old.keys().chain(new.keys()).collect(); let mut diff = SectionDiff::default(); for name in names { match (old.get(name), new.get(name)) { - (Some(before), Some(after)) if before != after => diff.changed.push(ChangedItem { - name: name.clone(), - from: before.clone(), - to: after.clone(), - }), + (Some(before), Some(after)) if before != after => { + // Check if this is a pure parameter reorder: same set of + // "name: type" tokens, different order. + let old_ps = old_params.get(name); + let new_ps = new_params.get(name); + if let (Some(op), Some(np)) = (old_ps, new_ps) { + if is_param_reorder(op, np) { + diff.reordered.push(ReorderedItem { + name: name.clone(), + from: before.clone(), + to: after.clone(), + }); + continue; + } + } + diff.changed.push(ChangedItem { + name: name.clone(), + from: before.clone(), + to: after.clone(), + }); + } (Some(_), Some(_)) => {} (Some(before), None) => diff.removed.push(before.clone()), (None, Some(after)) => diff.added.push(after.clone()), @@ -141,26 +206,44 @@ fn diff_section(old: &BTreeMap, new: &BTreeMap) diff } -fn function_sigs(spec: &ContractSpec) -> BTreeMap { - spec.functions - .iter() - .map(|f| { - let inputs: Vec = f - .inputs - .iter() - .map(|i| format!("{}: {}", i.name, i.type_name)) - .collect(); - let output = match f.outputs.as_slice() { - [] => "void".to_string(), - [one] => one.clone(), - many => format!("({})", many.join(", ")), - }; - ( - f.name.clone(), - format!("{}({}) -> {}", f.name, inputs.join(", "), output), - ) - }) - .collect() +/// Returns `true` when `old` and `new` contain the same `"name: type"` tokens +/// but in a different order. Both must be non-empty and must differ in order. +fn is_param_reorder(old: &[String], new: &[String]) -> bool { + if old.len() != new.len() || old == new { + return false; + } + let mut old_sorted = old.to_vec(); + let mut new_sorted = new.to_vec(); + old_sorted.sort(); + new_sorted.sort(); + old_sorted == new_sorted +} + +/// Returns a map of function name → rendered signature, plus a companion map +/// of function name → ordered `["param: type", …]` tokens. The second map is +/// used by `diff_section_with_params` to distinguish a pure parameter reorder +/// from a deeper signature change (renamed parameter, changed type, etc.). +fn function_sigs(spec: &ContractSpec) -> (BTreeMap, BTreeMap>) { + let mut sigs = BTreeMap::new(); + let mut params = BTreeMap::new(); + for f in &spec.functions { + let inputs: Vec = f + .inputs + .iter() + .map(|i| format!("{}: {}", i.name, i.type_name)) + .collect(); + let output = match f.outputs.as_slice() { + [] => "void".to_string(), + [one] => one.clone(), + many => format!("({})", many.join(", ")), + }; + sigs.insert( + f.name.clone(), + format!("{}({}) -> {}", f.name, inputs.join(", "), output), + ); + params.insert(f.name.clone(), inputs); + } + (sigs, params) } /// Event signatures carry each param's location and the body's data format: @@ -707,6 +790,91 @@ mod tests { assert!(d.functions.changed.is_empty()); } + /// Reordering parameters is a breaking change because Soroban encoding is + /// positional: a caller that passes `(from, to, amount)` by position will + /// send `from` where `amount` is now expected. The diff must flag this + /// explicitly as a `reordered` item rather than a generic `changed` item, + /// and the `breaking` flag must be set. + #[test] + fn reordered_parameters_are_breaking_and_flagged_as_reordered() { + let old = spec_of(&[func( + "transfer", + &[ + ("from", ScSpecTypeDef::Address), + ("to", ScSpecTypeDef::Address), + ("amount", ScSpecTypeDef::I128), + ], + None, + )]); + let new = spec_of(&[func( + "transfer", + &[ + ("amount", ScSpecTypeDef::I128), + ("from", ScSpecTypeDef::Address), + ("to", ScSpecTypeDef::Address), + ], + None, + )]); + let d = SpecDiff::between(&old, &new); + assert!(d.breaking, "parameter reorder must be breaking"); + assert!( + d.functions.changed.is_empty(), + "a pure reorder must not appear in 'changed'" + ); + assert_eq!( + d.functions.reordered.len(), + 1, + "a pure reorder must appear in 'reordered'" + ); + let item = &d.functions.reordered[0]; + assert_eq!(item.name, "transfer"); + assert!( + item.from.contains("from: Address"), + "from-signature should reference old order" + ); + assert!( + item.to.contains("amount: i128"), + "to-signature should reference new order" + ); + // The summary line should say "reordered function …" + assert_eq!(d.summary.len(), 1); + assert!( + d.summary[0].starts_with("reordered function transfer"), + "summary line should start with 'reordered function transfer', got: {}", + d.summary[0] + ); + } + + /// A change that renames or retypes a parameter is NOT a pure reorder — + /// it must still appear in `changed`, not `reordered`. + #[test] + fn renamed_parameter_is_changed_not_reordered() { + // Same types, different names — not a reorder. + let old = spec_of(&[func( + "transfer", + &[ + ("from", ScSpecTypeDef::Address), + ("to", ScSpecTypeDef::Address), + ], + None, + )]); + let new = spec_of(&[func( + "transfer", + &[ + ("sender", ScSpecTypeDef::Address), + ("recipient", ScSpecTypeDef::Address), + ], + None, + )]); + let d = SpecDiff::between(&old, &new); + assert!(d.breaking); + assert_eq!(d.functions.changed.len(), 1, "renamed params → changed"); + assert!( + d.functions.reordered.is_empty(), + "renamed params → not reordered" + ); + } + #[test] fn an_added_event_is_not_breaking() { let old = spec_of(&[func("balance", &[], Some(ScSpecTypeDef::I128))]);