Conversation
Add [lib] target to solver-service so it can be used as a library dependency. lib.rs already exists — this just declares it in Cargo.toml. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Load a JSON list of sanctioned Ethereum addresses at startup via solver.ofac_list in solver.toml. Intents whose sender or any recipient appears on the list are silently dropped before storage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…time Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional deny_list configuration and loads/enforces it in intent handling; updates config/operator types and admin API; reorders workspace members and consolidates AWS KMS dependencies and crate feature blocks across Cargo manifests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IntentHandler
participant Config
participant Storage
participant EventBus
Note over Config,IntentHandler: startup
Config->>IntentHandler: provide deny_list path
IntentHandler->>IntentHandler: load_deny_list -> HashSet
Client->>IntentHandler: Submit Intent
IntentHandler->>IntentHandler: check sender/recipients vs denied_addresses
alt address denied
IntentHandler->>EventBus: publish IntentRejected
IntentHandler-->>Client: rejection response
else address allowed
IntentHandler->>Storage: store intent
Storage-->>EventBus: publish IntentStored
EventBus-->>Client: confirmation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/solver-core/src/handlers/intent.rs (3)
79-83: Warning message may be misleading for intentionally empty deny lists.Line 82 logs "Deny List could not be loaded" when
denied_addresses.is_empty(), but this also triggers when the file exists and contains an empty array[]. Consider distinguishing between "failed to load" vs "loaded but empty".🔧 Suggested improvement
- if denied_addresses.is_empty() { - tracing::warn!("Deny List could not be loaded. Enforcement is DISABLED!"); - } + if denied_addresses.is_empty() { + tracing::warn!("Deny list is empty or could not be loaded. Enforcement is DISABLED!"); + }Alternatively,
load_deny_listcould return aResultor tuple to distinguish load failures from empty lists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 79 - 83, The warning is triggered whenever denied_addresses.is_empty(), which conflates a real loading failure with a legitimately empty deny list; change Self::load_deny_list to return a Result<Vec<String>, Error> (or a (Vec, bool) tuple) so callers can distinguish load failures from an empty vector, then update the call site that reads dynamic_config/blocking_read() and uses denied_addresses to match on the Result: on Err(...) emit the existing tracing::warn!("Deny List could not be loaded...") including error details, and on Ok(vec) only warn if vec is non-empty or log a different message like "Deny list loaded but empty" when appropriate.
101-131: Consider validating Ethereum address format when loading deny list.The function lowercases addresses but doesn't validate they're well-formed Ethereum addresses (e.g., 42-character hex strings with
0xprefix). Malformed entries would silently never match, which could be confusing for operators.🛡️ Optional validation
Ok(addrs) => { - let set: HashSet<String> = - addrs.into_iter().map(|a| a.to_lowercase()).collect(); + let set: HashSet<String> = addrs + .into_iter() + .filter_map(|a| { + let addr = a.to_lowercase(); + // Basic validation: 0x prefix + 40 hex chars + if addr.len() == 42 && addr.starts_with("0x") && addr[2..].chars().all(|c| c.is_ascii_hexdigit()) { + Some(addr) + } else { + tracing::warn!(address = %a, "Invalid address format in deny list, skipping"); + None + } + }) + .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 101 - 131, In load_deny_list, validate each parsed address is a well-formed Ethereum address before inserting into the HashSet (e.g., check it starts with "0x", is 42 chars long, and the hex portion parses, or use an Address::from_str helper) instead of silently accepting any string; skip and emit a tracing::warn for malformed entries (including the offending value and path) and only insert validated, lowercased addresses into the set so operators see which entries were dropped and the deny list contains only valid addresses.
176-219: Missing test coverage for deny list functionality.The deny list feature introduces significant logic (sender/recipient checking, event publishing, early return), but no tests cover these scenarios. Consider adding tests for:
- Intent rejected when sender is on deny list
- Intent rejected when recipient is on deny list
- Intent processed normally when addresses are not on deny list
- Behavior with empty deny list
Would you like me to help generate unit tests for the deny list functionality?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 176 - 219, Add unit tests covering deny-list behavior in the intent handler: create tests that (1) construct an Eip7683OrderData with user equal to a denied address and assert the handler (method that processes intents in handlers::intent.rs) returns Ok(()) and that event_bus.publish was called with SolverEvent::Discovery(DiscoveryEvent::IntentRejected { intent_id: ..., reason: ... }) for the sender case; (2) construct Eip7683OrderData with an output recipient equal to a denied address and assert the same publish/return behavior for the recipient case; (3) test that when denied_addresses is empty or contains different addresses the intent is processed normally (no IntentRejected publish); and (4) test the normal flow when denied_addresses does not match any involved addresses. In each test, set up the handler instance with self.denied_addresses, supply intent.data as serde_json::to_value(order) to match the code path that deserializes into Eip7683OrderData, and replace or instrument event_bus to capture published events so you can assert the correct SolverEvent::Discovery payload and that the handler returns early (Ok(())) where expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/solver-core/src/handlers/intent.rs`:
- Around line 79-83: The warning is triggered whenever
denied_addresses.is_empty(), which conflates a real loading failure with a
legitimately empty deny list; change Self::load_deny_list to return a
Result<Vec<String>, Error> (or a (Vec, bool) tuple) so callers can distinguish
load failures from an empty vector, then update the call site that reads
dynamic_config/blocking_read() and uses denied_addresses to match on the Result:
on Err(...) emit the existing tracing::warn!("Deny List could not be loaded...")
including error details, and on Ok(vec) only warn if vec is non-empty or log a
different message like "Deny list loaded but empty" when appropriate.
- Around line 101-131: In load_deny_list, validate each parsed address is a
well-formed Ethereum address before inserting into the HashSet (e.g., check it
starts with "0x", is 42 chars long, and the hex portion parses, or use an
Address::from_str helper) instead of silently accepting any string; skip and
emit a tracing::warn for malformed entries (including the offending value and
path) and only insert validated, lowercased addresses into the set so operators
see which entries were dropped and the deny list contains only valid addresses.
- Around line 176-219: Add unit tests covering deny-list behavior in the intent
handler: create tests that (1) construct an Eip7683OrderData with user equal to
a denied address and assert the handler (method that processes intents in
handlers::intent.rs) returns Ok(()) and that event_bus.publish was called with
SolverEvent::Discovery(DiscoveryEvent::IntentRejected { intent_id: ..., reason:
... }) for the sender case; (2) construct Eip7683OrderData with an output
recipient equal to a denied address and assert the same publish/return behavior
for the recipient case; (3) test that when denied_addresses is empty or contains
different addresses the intent is processed normally (no IntentRejected
publish); and (4) test the normal flow when denied_addresses does not match any
involved addresses. In each test, set up the handler instance with
self.denied_addresses, supply intent.data as serde_json::to_value(order) to
match the code path that deserializes into Eip7683OrderData, and replace or
instrument event_bus to capture published events so you can assert the correct
SolverEvent::Discovery payload and that the handler returns early (Ok(())) where
expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 295da53a-410c-4733-9194-5a198e7e1e9e
📒 Files selected for processing (14)
Cargo.tomlcrates/solver-account/Cargo.tomlcrates/solver-config/src/builders/config.rscrates/solver-config/src/lib.rscrates/solver-core/Cargo.tomlcrates/solver-core/src/handlers/intent.rscrates/solver-delivery/Cargo.tomlcrates/solver-demo/Cargo.tomlcrates/solver-order/Cargo.tomlcrates/solver-pricing/Cargo.tomlcrates/solver-service/Cargo.tomlcrates/solver-service/src/config_merge.rscrates/solver-settlement/Cargo.tomlcrates/solver-storage/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/solver-core/src/handlers/intent.rs (1)
712-722:⚠️ Potential issue | 🔴 CriticalTests are missing the new
static_configparameter.The
IntentHandler::new()signature requires bothdynamic_config: Arc<RwLock<Config>>andstatic_config: &Config, but the test helper at lines 712-722 only passes a single config parameter.🐛 Proposed fix to add missing parameter
+ let static_config = config.read().await.clone(); + let handler = IntentHandler::new( order_service, storage, state_machine, event_bus, delivery, solver_address.clone(), token_manager, cost_profit_service, config, + &static_config, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 712 - 722, The test helper call to IntentHandler::new is missing the required static_config parameter: update the IntentHandler::new invocation to pass both dynamic_config (Arc<RwLock<Config>>) and static_config (&Config) per the new signature; locate the test that constructs the handler (the IntentHandler::new(...) call) and add a reference to the immutable config slice (e.g., &config or &static_config) as the second config argument while keeping the existing dynamic_config/Arc<RwLock<Config>> argument unchanged.
🧹 Nitpick comments (1)
crates/solver-core/src/handlers/intent.rs (1)
80-83: Misleading warning when deny list is intentionally unconfigured.The warning "Deny List could not be loaded. Enforcement is DISABLED!" fires when the deny list is intentionally not configured (i.e.,
deny_listisNone). This creates confusing log noise for operators who don't need the feature.Consider differentiating between "not configured" (info or debug level) vs "configured but failed to load" (warning level):
♻️ Proposed differentiation of log levels
pub fn new( ... static_config: &Config, ) -> Self { - let denied_addresses = Self::load_deny_list(static_config.solver.deny_list.as_deref()); - if denied_addresses.is_empty() { - tracing::warn!("Deny List could not be loaded. Enforcement is DISABLED!"); - } + let deny_list_path = static_config.solver.deny_list.as_deref(); + let denied_addresses = Self::load_deny_list(deny_list_path); + match (deny_list_path, denied_addresses.is_empty()) { + (None, _) | (Some(""), _) => { + tracing::info!("Deny list not configured. Enforcement is disabled."); + } + (Some(_), true) => { + tracing::warn!("Deny list configured but could not be loaded. Enforcement is DISABLED!"); + } + (Some(path), false) => { + tracing::info!(path = %path, count = %denied_addresses.len(), "Deny list loaded successfully"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 80 - 83, The current code logs a warning whenever denied_addresses is empty, which is misleading when deny_list was intentionally left None; change the logic to inspect static_config.solver.deny_list first and only emit a warning if deny_list was Some but load_deny_list returned an empty/failure result, otherwise emit an info/debug message that the deny list is not configured; update messages and log levels accordingly and keep references to load_deny_list, denied_addresses, and static_config.solver.deny_list when locating the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/solver-service/src/config_merge.rs`:
- Line 848: The deny_list is being forced to None when constructing SolverConfig
(deny_list: None) and OperatorSolverConfig also lacks a deny_list field, so the
path is never propagated to IntentHandler; fix by adding a deny_list:
Option<String> (or Option<PathBuf>) field to OperatorSolverConfig, thread it
through the merge logic and SeedOverrides (or alternately add deny_list to
SeedOverrides), then update build_runtime_config to take the operator's
deny_list and set SolverConfig.deny_list from that value (or read from an env
var there if you prefer that approach); ensure references in SolverConfig
construction use the new field instead of hardcoding None so
static_config.solver.deny_list.as_deref() can return the configured path.
---
Outside diff comments:
In `@crates/solver-core/src/handlers/intent.rs`:
- Around line 712-722: The test helper call to IntentHandler::new is missing the
required static_config parameter: update the IntentHandler::new invocation to
pass both dynamic_config (Arc<RwLock<Config>>) and static_config (&Config) per
the new signature; locate the test that constructs the handler (the
IntentHandler::new(...) call) and add a reference to the immutable config slice
(e.g., &config or &static_config) as the second config argument while keeping
the existing dynamic_config/Arc<RwLock<Config>> argument unchanged.
---
Nitpick comments:
In `@crates/solver-core/src/handlers/intent.rs`:
- Around line 80-83: The current code logs a warning whenever denied_addresses
is empty, which is misleading when deny_list was intentionally left None; change
the logic to inspect static_config.solver.deny_list first and only emit a
warning if deny_list was Some but load_deny_list returned an empty/failure
result, otherwise emit an info/debug message that the deny list is not
configured; update messages and log levels accordingly and keep references to
load_deny_list, denied_addresses, and static_config.solver.deny_list when
locating the code to modify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a876b20-aafd-42ca-a5b0-e6fa45806806
📒 Files selected for processing (14)
Cargo.tomlcrates/solver-account/Cargo.tomlcrates/solver-config/src/lib.rscrates/solver-core/Cargo.tomlcrates/solver-core/src/engine/mod.rscrates/solver-core/src/handlers/intent.rscrates/solver-delivery/Cargo.tomlcrates/solver-demo/Cargo.tomlcrates/solver-order/Cargo.tomlcrates/solver-pricing/Cargo.tomlcrates/solver-service/Cargo.tomlcrates/solver-service/src/config_merge.rscrates/solver-settlement/Cargo.tomlcrates/solver-storage/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- Cargo.toml
- crates/solver-order/Cargo.toml
- crates/solver-settlement/Cargo.toml
- crates/solver-pricing/Cargo.toml
- crates/solver-account/Cargo.toml
- crates/solver-config/src/lib.rs
7a643e8 to
94cd7cf
Compare
|
Do you guys think we can get this PR merged soon? I think the denylist is a good addition for most teams who intent to use the system in production. I am unsure at the moment why the CI is failing but working on it. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/solver-service/Cargo.toml (1)
1-70:⚠️ Potential issue | 🔴 CriticalAdd missing
tomldependency to workspace.The CI pipeline failure is caused by
crates/solver-account/Cargo.tomlreferencingtoml = { workspace = true }without a corresponding entry in the rootCargo.toml's[workspace.dependencies]section. Addtomlto the workspace dependencies in the root Cargo.toml to resolve the workspace manifest parsing failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-service/Cargo.toml` around lines 1 - 70, The workspace is missing a declaration for the toml crate referenced as toml = { workspace = true } in crates/solver-account/Cargo.toml; open the root Cargo.toml and add toml to the [workspace.dependencies] section (e.g., include an entry for the toml crate so the workspace can resolve toml), ensuring the dependency key is "toml" and that it is declared in [workspace.dependencies] to match the per-crate workspace = true reference.
🧹 Nitpick comments (4)
crates/solver-order/Cargo.toml (1)
7-8: Consider usingdep:mockallto limit the public feature surface.
testing = ["mockall"]exposes both thetestingfeature and an implicitmockallfeature publicly. Iftestingshould be the only public feature for opting into mockall, usedep:mockallinstead to suppress the implicit feature.Suggested change
[features] -testing = ["mockall"] +testing = ["dep:mockall"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-order/Cargo.toml` around lines 7 - 8, The features entry currently exposes the public "mockall" feature via testing = ["mockall"]; change it to testing = ["dep:mockall"] so the crate only exposes the public "testing" feature while opting into the mockall dependency internally. Locate the features table in Cargo.toml and update the testing feature to reference dep:mockall instead of mockall (leave the dependency declaration for mockall as-is).crates/solver-core/src/handlers/intent.rs (3)
493-496: Missing test coverage for deny list functionality.The test helper and existing tests were updated for the new signature, but there are no tests for the deny list feature itself. For a compliance-critical capability (OFAC sanctions enforcement), consider adding tests for:
load_deny_listwith valid file, missing file, malformed JSON- Intent rejection when sender is on deny list
- Intent rejection when recipient is on deny list
- Behavior when
intent.datacannot be parsedDo you want me to generate unit tests for the deny list functionality?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 493 - 496, The test suite lacks coverage for the deny list feature introduced alongside the updated create_test_config helper; add unit tests targeting the deny-list behavior by exercising the load_deny_list function (valid file, missing file, malformed JSON) and the intent handling path that consults the deny list: create tests that assert intents are rejected when the sender is on the deny list, when the recipient is on the deny list, and when intent.data cannot be parsed; use the create_test_config helper to build Config instances, call load_deny_list to populate the Config (or inject a mock deny list), and invoke the intent evaluation function in intent.rs to assert accept/reject outcomes and appropriate error cases.
80-83: Warning message conflates intentional disablement with load failure.The current warning fires whenever
denied_addressesis empty, but this includes the intentional case where nodeny_listpath is configured. This makes it hard for operators to distinguish between "feature is off by design" and "feature failed to load due to config error."Consider emitting distinct log messages based on the actual state:
♻️ Suggested improvement
- let denied_addresses = Self::load_deny_list(static_config.solver.deny_list.as_deref()); - if denied_addresses.is_empty() { - tracing::warn!("Deny List could not be loaded. Enforcement is DISABLED!"); - } + let deny_list_path = static_config.solver.deny_list.as_deref(); + let denied_addresses = Self::load_deny_list(deny_list_path); + match (deny_list_path, denied_addresses.is_empty()) { + (None, _) | (Some(""), _) => { + tracing::info!("No deny list configured. Enforcement is disabled."); + } + (Some(_), true) => { + tracing::warn!("Deny list configured but loaded 0 addresses. Check file contents."); + } + (Some(_), false) => { + // Success case already logged in load_deny_list + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 80 - 83, The warning currently triggers whenever denied_addresses is empty, which mixes the intentional "no deny_list configured" case with an actual load failure; update the logic in intent handler around Self::load_deny_list, denied_addresses and static_config.solver.deny_list to distinguish cases: if static_config.solver.deny_list is None, log an informational message that deny-list enforcement is intentionally disabled; if Some(path) but load_deny_list returned an error or empty list, log a warning (include the path and any error details) indicating the deny list failed to load and enforcement is disabled; keep existing behavior when a valid non-empty deny list is present.
111-121: Consider validating address format to catch configuration errors.Invalid entries (typos, missing
0xprefix, wrong length) will be silently stored but never match. For a compliance-critical feature like sanctions enforcement, failing loudly on malformed entries helps operators catch config errors.♻️ Example validation
Ok(addrs) => { - let set: HashSet<String> = - addrs.into_iter().map(|a| a.to_lowercase()).collect(); + let mut set = HashSet::new(); + for addr in addrs { + let normalized = addr.to_lowercase(); + // Basic Ethereum address validation: 0x + 40 hex chars + if normalized.len() == 42 + && normalized.starts_with("0x") + && normalized[2..].chars().all(|c| c.is_ascii_hexdigit()) + { + set.insert(normalized); + } else { + tracing::warn!( + address = %addr, + "Invalid address format in deny list, skipping" + ); + } + } tracing::info!(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 111 - 121, The parsed deny-list entries (variable addrs -> set) are not validated and malformed addresses will be silently stored; update the Ok(content) match branch to validate each address string (e.g., normalize with to_lowercase, ensure 0x prefix, correct length and hex characters) using a helper like is_valid_address(&str), collect any invalid entries, and if any are found fail loudly by returning an Err or logging a clear tracing::error and exiting (or propagating the error) with the list of bad entries; replace the current silent set creation (addrs.into_iter().map(|a| a.to_lowercase()).collect()) with this validated iteration so only valid addresses end up in the HashSet and operators get clear feedback on malformed config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/solver-core/src/handlers/intent.rs`:
- Around line 181-183: The deny-list check is comparing self.denied_addresses
(which store addresses with "0x" prefix) against order_data.user after only
lowercasing it, so addresses missing the "0x" prefix won't match; update the
check to normalize the user address the same way used elsewhere by calling
with_0x_prefix(order_data.user.to_lowercase()) (or equivalent helper) before
testing contains on self.denied_addresses so the comparison uses the canonical
"0x"-prefixed, lowercased form.
- Around line 179-180: The code currently uses
serde_json::from_value::<Eip7683OrderData>(intent.data.clone()) with an if let
Ok(...) so parsing failures are silently skipped; change this to handle the Err
case by logging a warning that includes the parse error and identifying info
(e.g., intent id or intent.raw data) before skipping the deny-list check. Locate
the parsing site (the from_value::<Eip7683OrderData>(intent.data.clone()) call)
and replace the if let with a match or if let Err arm that emits a warning via
the existing logger (e.g., process_logger, tracing, or the module's logger)
including the error and context so operators can observe malformed or
non-EIP-7683 data. Ensure the normal deny-list logic remains unchanged for the
Ok branch.
---
Outside diff comments:
In `@crates/solver-service/Cargo.toml`:
- Around line 1-70: The workspace is missing a declaration for the toml crate
referenced as toml = { workspace = true } in crates/solver-account/Cargo.toml;
open the root Cargo.toml and add toml to the [workspace.dependencies] section
(e.g., include an entry for the toml crate so the workspace can resolve toml),
ensuring the dependency key is "toml" and that it is declared in
[workspace.dependencies] to match the per-crate workspace = true reference.
---
Nitpick comments:
In `@crates/solver-core/src/handlers/intent.rs`:
- Around line 493-496: The test suite lacks coverage for the deny list feature
introduced alongside the updated create_test_config helper; add unit tests
targeting the deny-list behavior by exercising the load_deny_list function
(valid file, missing file, malformed JSON) and the intent handling path that
consults the deny list: create tests that assert intents are rejected when the
sender is on the deny list, when the recipient is on the deny list, and when
intent.data cannot be parsed; use the create_test_config helper to build Config
instances, call load_deny_list to populate the Config (or inject a mock deny
list), and invoke the intent evaluation function in intent.rs to assert
accept/reject outcomes and appropriate error cases.
- Around line 80-83: The warning currently triggers whenever denied_addresses is
empty, which mixes the intentional "no deny_list configured" case with an actual
load failure; update the logic in intent handler around Self::load_deny_list,
denied_addresses and static_config.solver.deny_list to distinguish cases: if
static_config.solver.deny_list is None, log an informational message that
deny-list enforcement is intentionally disabled; if Some(path) but
load_deny_list returned an error or empty list, log a warning (include the path
and any error details) indicating the deny list failed to load and enforcement
is disabled; keep existing behavior when a valid non-empty deny list is present.
- Around line 111-121: The parsed deny-list entries (variable addrs -> set) are
not validated and malformed addresses will be silently stored; update the
Ok(content) match branch to validate each address string (e.g., normalize with
to_lowercase, ensure 0x prefix, correct length and hex characters) using a
helper like is_valid_address(&str), collect any invalid entries, and if any are
found fail loudly by returning an Err or logging a clear tracing::error and
exiting (or propagating the error) with the list of bad entries; replace the
current silent set creation (addrs.into_iter().map(|a|
a.to_lowercase()).collect()) with this validated iteration so only valid
addresses end up in the HashSet and operators get clear feedback on malformed
config.
In `@crates/solver-order/Cargo.toml`:
- Around line 7-8: The features entry currently exposes the public "mockall"
feature via testing = ["mockall"]; change it to testing = ["dep:mockall"] so the
crate only exposes the public "testing" feature while opting into the mockall
dependency internally. Locate the features table in Cargo.toml and update the
testing feature to reference dep:mockall instead of mockall (leave the
dependency declaration for mockall as-is).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 428a5bd9-c8ea-4ea4-9b0a-cb6b33ef7c54
📒 Files selected for processing (14)
Cargo.tomlcrates/solver-account/Cargo.tomlcrates/solver-config/src/lib.rscrates/solver-core/Cargo.tomlcrates/solver-core/src/engine/mod.rscrates/solver-core/src/handlers/intent.rscrates/solver-delivery/Cargo.tomlcrates/solver-demo/Cargo.tomlcrates/solver-order/Cargo.tomlcrates/solver-pricing/Cargo.tomlcrates/solver-service/Cargo.tomlcrates/solver-service/src/config_merge.rscrates/solver-settlement/Cargo.tomlcrates/solver-storage/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- crates/solver-config/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/solver-delivery/Cargo.toml
- crates/solver-core/src/engine/mod.rs
- crates/solver-settlement/Cargo.toml
- crates/solver-account/Cargo.toml
- crates/solver-core/Cargo.toml
- crates/solver-pricing/Cargo.toml
- crates/solver-demo/Cargo.toml
- Cargo.toml
- crates/solver-storage/Cargo.toml
- crates/solver-service/src/config_merge.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/solver-core/src/handlers/intent.rs (2)
1211-1221:⚠️ Potential issue | 🔴 CriticalMissing
static_configparameter causes compilation failure.Same issue as the previous test—
IntentHandler::newrequiresstatic_config: &Configbut it's not passed here.🐛 Proposed fix
+ let static_config = config.read().await.clone(); + let handler = IntentHandler::new( order_service, storage, state_machine, event_bus, delivery, solver_address, token_manager, cost_profit_service, config, + &static_config, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 1211 - 1221, IntentHandler::new is missing the required static_config parameter; update the call that constructs the handler (the IntentHandler::new invocation) to pass the static_config: &Config reference used by the surrounding test/setup (or create/borrow the existing config variable) as the additional argument so the signature matches IntentHandler::new(order_service, storage, state_machine, event_bus, delivery, solver_address, token_manager, cost_profit_service, config, static_config).
1135-1145:⚠️ Potential issue | 🔴 CriticalMissing
static_configparameter causes compilation failure.The
IntentHandler::newsignature now requiresstatic_config: &Configas the last parameter (line 79), but this test passes onlyconfigwithout&static_config. This will cause a compilation error.🐛 Proposed fix
+ let static_config = config.read().await.clone(); + let handler = IntentHandler::new( order_service, storage, state_machine, event_bus, delivery, solver_address, token_manager, cost_profit_service, config, + &static_config, );Alternatively, update
create_test_config_with_broadcaster()to return a tuple likecreate_test_config()does.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 1135 - 1145, The test calls IntentHandler::new without the required static_config parameter, causing a compile error; update the call in the test (where IntentHandler::new(...) is constructed) to pass the missing static_config as the final argument (e.g., pass &static_config) or change create_test_config_with_broadcaster() to return the same tuple (config, static_config) like create_test_config() so you can supply &static_config to IntentHandler::new; ensure the argument order matches the new signature used by IntentHandler::new.
♻️ Duplicate comments (2)
crates/solver-core/src/handlers/intent.rs (2)
182-184:⚠️ Potential issue | 🟠 MajorUser address may lack
0xprefix, causing match failure.The deny list stores addresses with
0xprefix (line 115 preserves existing prefixes, line 203 explicitly adds0xfor recipients), butorder_data.useris only lowercased without ensuring the0xprefix. If theuserfield doesn't include the prefix, it will never match a deny list entry. Thewith_0x_prefixutility is already imported and used elsewhere in this file (line 227).,
🐛 Proposed fix
// Check the order sender (user field). - let user_addr = order_data.user.to_lowercase(); + let user_addr = with_0x_prefix(&order_data.user).to_lowercase(); if self.denied_addresses.contains(&user_addr) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 182 - 184, The deny-list check uses a lowercased user address but doesn't ensure a leading "0x", so matches against self.denied_addresses fail; update the check in the intent handler to normalize the user address with the existing with_0x_prefix utility (apply it to the lowercased order_data.user) before calling contains, i.e., derive user_addr via with_0x_prefix(&order_data.user.to_lowercase()) and then use self.denied_addresses.contains(&user_addr) so addresses with or without the prefix match correctly.
180-181:⚠️ Potential issue | 🟡 MinorSilent skip on parse failure may bypass sanctions enforcement.
If
intent.datacannot be parsed asEip7683OrderData, the deny list check is silently skipped. For compliance-critical functionality like sanctions enforcement, consider logging when parsing fails to provide operator visibility.,
🛡️ Suggested improvement
- if let Ok(order_data) = serde_json::from_value::<Eip7683OrderData>(intent.data.clone()) - { + match serde_json::from_value::<Eip7683OrderData>(intent.data.clone()) { + Ok(order_data) => { // Check the order sender (user field). // ... existing deny list logic ... + } + Err(e) => { + tracing::debug!( + intent_id = %intent.id, + error = %e, + "Could not parse intent data for deny list check, skipping" + ); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 180 - 181, The current if let Ok(...) = serde_json::from_value::<Eip7683OrderData>(intent.data.clone()) silently skips deny-list checks on parse failure; change this to match the Result so that Err branches log a warning/error including identifying info (e.g., intent.id or intent.meta) and the serde_json error before continuing, and keep the deny-list enforcement path explicit for the Ok(Eip7683OrderData) case; update the block around the Eip7683OrderData parse (the if let in handlers/intent.rs) to use match or handle Err to emit operator-visible logs.
🧹 Nitpick comments (1)
crates/solver-core/src/handlers/intent.rs (1)
81-84: Warning message is misleading when deny list is intentionally unconfigured.The warning "Enforcement is DISABLED!" fires whenever
denied_addressesis empty, but this conflates three different scenarios:
- No
deny_listpath configured (intentional, no warning needed)- Path configured but file missing/parse failed (warning already emitted by
load_deny_list)- Path configured, file valid, but contains zero addresses (rare edge case)
Consider adjusting the logic to only warn when a path was configured but loading yielded an empty set:
♻️ Suggested improvement
pub fn new( // ... parameters ... static_config: &Config, ) -> Self { - let denied_addresses = Self::load_deny_list(static_config.solver.deny_list.as_deref()); - if denied_addresses.is_empty() { - tracing::warn!("Deny List could not be loaded. Enforcement is DISABLED!"); - } + let deny_list_path = static_config.solver.deny_list.as_deref(); + let denied_addresses = Self::load_deny_list(deny_list_path); + if deny_list_path.is_some() && denied_addresses.is_empty() { + tracing::warn!("Deny list path was configured but no addresses were loaded. Enforcement is DISABLED!"); + } else if deny_list_path.is_none() { + tracing::info!("No deny list configured"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 81 - 84, The warning is emitted whenever denied_addresses is empty, conflating "no path configured" with an actual load failure or empty file; update the logic in the block around load_deny_list so you only emit the tracing::warn when a deny_list path was actually configured but loading returned an empty set: check static_config.solver.deny_list.is_some() (or .as_deref().is_some()) and then if denied_addresses.is_empty() log the warning; leave load_deny_list to continue emitting its own errors on I/O/parse failures and don't warn when no path was provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/solver-core/src/handlers/intent.rs`:
- Around line 1211-1221: IntentHandler::new is missing the required
static_config parameter; update the call that constructs the handler (the
IntentHandler::new invocation) to pass the static_config: &Config reference used
by the surrounding test/setup (or create/borrow the existing config variable) as
the additional argument so the signature matches
IntentHandler::new(order_service, storage, state_machine, event_bus, delivery,
solver_address, token_manager, cost_profit_service, config, static_config).
- Around line 1135-1145: The test calls IntentHandler::new without the required
static_config parameter, causing a compile error; update the call in the test
(where IntentHandler::new(...) is constructed) to pass the missing static_config
as the final argument (e.g., pass &static_config) or change
create_test_config_with_broadcaster() to return the same tuple (config,
static_config) like create_test_config() so you can supply &static_config to
IntentHandler::new; ensure the argument order matches the new signature used by
IntentHandler::new.
---
Duplicate comments:
In `@crates/solver-core/src/handlers/intent.rs`:
- Around line 182-184: The deny-list check uses a lowercased user address but
doesn't ensure a leading "0x", so matches against self.denied_addresses fail;
update the check in the intent handler to normalize the user address with the
existing with_0x_prefix utility (apply it to the lowercased order_data.user)
before calling contains, i.e., derive user_addr via
with_0x_prefix(&order_data.user.to_lowercase()) and then use
self.denied_addresses.contains(&user_addr) so addresses with or without the
prefix match correctly.
- Around line 180-181: The current if let Ok(...) =
serde_json::from_value::<Eip7683OrderData>(intent.data.clone()) silently skips
deny-list checks on parse failure; change this to match the Result so that Err
branches log a warning/error including identifying info (e.g., intent.id or
intent.meta) and the serde_json error before continuing, and keep the deny-list
enforcement path explicit for the Ok(Eip7683OrderData) case; update the block
around the Eip7683OrderData parse (the if let in handlers/intent.rs) to use
match or handle Err to emit operator-visible logs.
---
Nitpick comments:
In `@crates/solver-core/src/handlers/intent.rs`:
- Around line 81-84: The warning is emitted whenever denied_addresses is empty,
conflating "no path configured" with an actual load failure or empty file;
update the logic in the block around load_deny_list so you only emit the
tracing::warn when a deny_list path was actually configured but loading returned
an empty set: check static_config.solver.deny_list.is_some() (or
.as_deref().is_some()) and then if denied_addresses.is_empty() log the warning;
leave load_deny_list to continue emitting its own errors on I/O/parse failures
and don't warn when no path was provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aba467ea-3754-4644-8dc5-aec811ec898f
📒 Files selected for processing (7)
crates/solver-config/src/builders/config.rscrates/solver-config/src/lib.rscrates/solver-core/Cargo.tomlcrates/solver-core/src/engine/mod.rscrates/solver-core/src/handlers/intent.rscrates/solver-service/src/config_merge.rscrates/solver-settlement/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- crates/solver-settlement/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/solver-service/src/config_merge.rs
- crates/solver-config/src/builders/config.rs
- crates/solver-core/src/engine/mod.rs
nahimterrazas
left a comment
There was a problem hiding this comment.
Thanks for taking the time to contribute this. It’s a useful feature, and I appreciate you opening it up as a general solution.
I reviewed the PR, and I don’t think it’s ready to merge yet. The main blockers I found are:
The branch currently breaks the workspace build. solver-account adds toml = { workspace = true }, but toml is not defined in the root workspace dependencies, so Cargo fails before tests even run.
The denylist is not wired into the project’s actual runtime config flow. The solver boots and reloads from OperatorConfig, but this new field only exists on Config, and build_runtime_config() currently sets deny_list: None. In practice, that means the feature won’t be active in the normal seeded/persisted config path.
The enforcement currently fails open. If the configured file is missing or malformed, the solver logs a warning and continues with enforcement disabled. For a sanctions/compliance control, that feels too risky.
I do think the general direction makes sense: rejecting denied intents early in the intent handler is a reasonable place for this logic. But I think it should be reworked so the denylist is a first-class part of the persisted config model, with clear startup/reload behavior and stricter handling when a configured list cannot be loaded.
If you want to keep working on it, I think the next step would be:
- fix the Cargo manifest issue,
- add denylist support to OperatorConfig and the config conversion pipeline,
- decide on fail-closed vs explicit optional mode,
- add focused tests for sender hit, recipient hit, and invalid/missing file behavior.
Happy to take another look after that.
|
I see... The issue here is that we spawn a solver instance using our own CLI. But I can re-structure the denylist to be a first class citizen of oif-solver. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
crates/solver-core/src/handlers/intent.rs (2)
127-127:⚠️ Potential issue | 🟠 MajorAddress canonicalization is still inconsistent (
0xprefix mismatch risk).Deny-list entries and sender addresses are lowercased, but not normalized with
with_0x_prefix, so equivalent addresses can fail to match.🐛 Proposed fix
- let set: HashSet<String> = addrs.into_iter().map(|a| a.to_lowercase()).collect(); + let set: HashSet<String> = addrs + .into_iter() + .map(|a| with_0x_prefix(&a).to_lowercase()) + .collect(); - let user_addr = order_data.user.to_lowercase(); + let user_addr = with_0x_prefix(&order_data.user).to_lowercase();Also applies to: 184-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` at line 127, The deny-list normalization lowercases addresses but doesn't enforce the 0x prefix, causing mismatch; update the normalization to call the existing with_0x_prefix helper when building the deny HashSet (the let set: HashSet<String> = addrs.into_iter().map(|a| a.to_lowercase()).collect() site) and do the same when normalizing sender addresses before membership checks (the comparisons around lines handling the sender variable at ~184-185). Ensure you apply with_0x_prefix to the string (and then lowercase or vice versa consistently) so both deny entries and senders use the exact same canonical form.
181-182:⚠️ Potential issue | 🟡 MinorParse failures still skip deny-list checks without telemetry.
Only the
Ok(...)path is handled. Malformed/non-EIP7683 payloads bypass checks silently, which hurts compliance observability.🛡️ Proposed fix
- if let Ok(order_data) = serde_json::from_value::<Eip7683OrderData>(intent.data.clone()) - { + match serde_json::from_value::<Eip7683OrderData>(intent.data.clone()) { + Ok(order_data) => { // existing deny-list logic... + } + Err(error) => { + tracing::warn!( + intent_id = %intent.id, + %error, + "Could not parse intent data for deny-list validation; skipping check" + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/handlers/intent.rs` around lines 181 - 182, The current `if let Ok(order_data) = serde_json::from_value::<Eip7683OrderData>(intent.data.clone())` only handles the Ok path and silently skips deny-list/telemetry for parse failures; update this to handle the Err branch: capture the parse error from `serde_json::from_value`, emit appropriate telemetry/logging with the error and the offending `intent.data`, and ensure the deny-list checks that run for parsed orders also run (or run a fallback deny-list flow) when parsing fails so malformed/non-EIP7683 payloads are audited; reference `Eip7683OrderData`, `serde_json::from_value`, and `intent.data` to locate and add the telemetry + deny-list invocation in the else/Err path.crates/solver-service/src/config_merge.rs (1)
449-456:⚠️ Potential issue | 🔴 CriticalDeny-list value is still dropped during initial merge (compliance/security risk).
At Line 455,
deny_listis hardcoded toNone, so initializer-driven first boot cannot carry a configured deny-list into persisted operator config.🔧 Proposed fix
solver: OperatorSolverConfig { min_profitability_pct, gas_buffer_bps, commission_bps, rate_buffer_bps, monitoring_timeout_seconds: seed.defaults.monitoring_timeout_seconds, - deny_list: None, + deny_list: initializer.deny_list.clone(), },If
SeedOverridesdoes not yet exposedeny_list, add it there and thread it through JSON parsing asOption<String>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-service/src/config_merge.rs` around lines 449 - 456, The initializer is dropping the configured deny-list by hardcoding deny_list: None in the OperatorSolverConfig construction; update SeedOverrides (and its JSON parsing) to expose deny_list as Option<String> and thread that value into the merge so the OperatorSolverConfig uses the seed-provided deny_list (e.g., deny_list: seed.defaults.deny_list or deny_list: seed.deny_list depending on how you modeled defaults) instead of None; adjust any JSON (de)serialization for SeedOverrides to accept an optional deny_list and ensure the merge logic in config_merge.rs reads and assigns that Option to OperatorSolverConfig::deny_list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/solver-core/src/handlers/intent.rs`:
- Around line 79-83: The deny-list is being read once from static_config during
construction (using static_config, deny_list_configured, and denied_addresses
via Self::load_deny_list), freezing enforcement to startup values; instead, stop
preloading those values and either store a reference to the live/merged Config
or move the call to Self::load_deny_list into the enforcement path so you
compute deny_list_configured and denied_addresses at runtime (or call a helper
that reads the current merged config when checking an address). Update the
constructor to not cache deny list state and update any enforcement methods to
call Self::load_deny_list or read the live config before making allow/deny
decisions.
- Around line 119-122: The current match on the local variable `path` treats
Some("") as absent and returns Ok(empty set), bypassing fail-closed; change the
fallback arm so that when `path` is Some but empty (i.e., the "_" / empty case)
the function returns an error instead of Ok(HashSet::new()), thereby enforcing
fail-closed. Locate the match block in handlers/intent.rs that binds `path` (the
shown match) and replace the `_ => return Ok(HashSet::new())` behavior with
returning the appropriate Err value used by this function (use the same error
type/constructor used elsewhere in this file to indicate an invalid/empty
deny-list path).
---
Duplicate comments:
In `@crates/solver-core/src/handlers/intent.rs`:
- Line 127: The deny-list normalization lowercases addresses but doesn't enforce
the 0x prefix, causing mismatch; update the normalization to call the existing
with_0x_prefix helper when building the deny HashSet (the let set:
HashSet<String> = addrs.into_iter().map(|a| a.to_lowercase()).collect() site)
and do the same when normalizing sender addresses before membership checks (the
comparisons around lines handling the sender variable at ~184-185). Ensure you
apply with_0x_prefix to the string (and then lowercase or vice versa
consistently) so both deny entries and senders use the exact same canonical
form.
- Around line 181-182: The current `if let Ok(order_data) =
serde_json::from_value::<Eip7683OrderData>(intent.data.clone())` only handles
the Ok path and silently skips deny-list/telemetry for parse failures; update
this to handle the Err branch: capture the parse error from
`serde_json::from_value`, emit appropriate telemetry/logging with the error and
the offending `intent.data`, and ensure the deny-list checks that run for parsed
orders also run (or run a fallback deny-list flow) when parsing fails so
malformed/non-EIP7683 payloads are audited; reference `Eip7683OrderData`,
`serde_json::from_value`, and `intent.data` to locate and add the telemetry +
deny-list invocation in the else/Err path.
In `@crates/solver-service/src/config_merge.rs`:
- Around line 449-456: The initializer is dropping the configured deny-list by
hardcoding deny_list: None in the OperatorSolverConfig construction; update
SeedOverrides (and its JSON parsing) to expose deny_list as Option<String> and
thread that value into the merge so the OperatorSolverConfig uses the
seed-provided deny_list (e.g., deny_list: seed.defaults.deny_list or deny_list:
seed.deny_list depending on how you modeled defaults) instead of None; adjust
any JSON (de)serialization for SeedOverrides to accept an optional deny_list and
ensure the merge logic in config_merge.rs reads and assigns that Option to
OperatorSolverConfig::deny_list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e312962-6a17-456c-8692-dbc61c2a5b3b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/solver-account/Cargo.tomlcrates/solver-config/src/builders/config.rscrates/solver-core/Cargo.tomlcrates/solver-core/src/handlers/intent.rscrates/solver-service/src/apis/admin.rscrates/solver-service/src/config_merge.rscrates/solver-settlement/Cargo.tomlcrates/solver-types/src/operator_config.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/solver-settlement/Cargo.toml
- crates/solver-core/Cargo.toml
- crates/solver-config/src/builders/config.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Hey, thank you for your thorough review and comments.
|
Generalized Deny List for EVM addresses
At Celestia we are currently building on top of the OIF solver and would like to enforce the OFAC sanctions list. Therefore
this PR introduces a generalized deny list to the solver such that other teams can enforce to their own
discretion and we won't need to maintain a fork to enforce OFAC.
Summary by CodeRabbit
New Features
Chores