Open
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>
jonas089
requested review from
NicoMolinaOZ,
nahimterrazas,
pepebndc and
shahnami
as code owners
📝 WalkthroughWalkthroughAdds an optional deny_list field to SolverConfig and wires deny-list loading/enforcement into the intent handling flow; reorders workspace members, consolidates duplicate feature blocks, and promotes AWS KMS deps to workspace-level dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IntentHandler
participant Config
participant Storage
participant EventBus
Note over Config,IntentHandler: on startup
Config->>IntentHandler: provide deny_list path / data
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: reject 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 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
jonas089
force-pushed
the
jonas/denylist
branch
from
7a643e8 to
94cd7cf
Compare
Contributor
Author
|
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
Comment on lines
+179
to
+180
| if let Ok(order_data) = serde_json::from_value::<Eip7683OrderData>(intent.data.clone()) | ||
| { |
There was a problem hiding this comment.
Silent skip on parse failure may bypass sanctions enforcement.
If intent.data cannot be parsed as Eip7683OrderData (malformed data, different standard), the deny list check is silently skipped. For compliance-critical functionality, consider logging a warning when parsing fails so operators have 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 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.
Comment on lines
+181
to
+183
| // Check the order sender (user field). | ||
| let user_addr = order_data.user.to_lowercase(); | ||
| if self.denied_addresses.contains(&user_addr) { |
There was a problem hiding this comment.
User address may lack 0x prefix, causing match failure.
The deny list stores addresses with 0x prefix (per line 61-62 documentation), but order_data.user is lowercased without normalization. If the user field doesn't include the 0x prefix, it will never match a deny list entry. The with_0x_prefix utility is already used elsewhere in this file (line 226).
🐛 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) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Check the order sender (user field). | |
| let user_addr = order_data.user.to_lowercase(); | |
| if self.denied_addresses.contains(&user_addr) { | |
| // Check the order sender (user field). | |
| 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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