Skip to content

feat: Denylist#308

Open
jonas089 wants to merge 15 commits intoopenintentsframework:mainfrom
celestiaorg:jonas/denylist
Open

feat: Denylist#308
jonas089 wants to merge 15 commits intoopenintentsframework:mainfrom
celestiaorg:jonas/denylist

Conversation

@jonas089
Copy link
Contributor

@jonas089 jonas089 commented Mar 9, 2026

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

    • Deny-list support: optional address list to reject matching intents at runtime with startup warnings.
    • AWS KMS signer support for signing operations (optional integration).
  • Chores

    • Consolidated and reordered workspace manifests and dev-dependencies.
    • Standardized and deduplicated testing feature flags across crates.

jonas089 and others added 6 commits March 4, 2026 00:23
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>
@coderabbitai
Copy link

coderabbitai bot commented Mar 9, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s) Summary
Workspace & AWS deps
Cargo.toml
Reorders workspace members and consolidates AWS KMS deps into [workspace.dependencies] (adds aws-config, aws-sdk-kms) and removes a duplicated AWS block.
Crate Cargo feature cleanup
crates/solver-account/Cargo.toml, crates/solver-delivery/Cargo.toml, crates/solver-order/Cargo.toml, crates/solver-pricing/Cargo.toml, crates/solver-settlement/Cargo.toml, crates/solver-storage/Cargo.toml, crates/solver-service/Cargo.toml
Adds/normalizes top-level [features] blocks (e.g., testing = ["mockall"]), removes duplicate feature declarations, and exposes/relocates kms feature entries where applicable.
Dev-dependencies adjustments
crates/solver-core/Cargo.toml, crates/solver-storage/Cargo.toml, crates/solver-demo/Cargo.toml, crates/solver-service/Cargo.toml
Reorders and adjusts dev-dependencies (adds testing-featureed workspace crates, moves rust_decimal/tempfile to dev/workspace entries, minor reordering of wiremock/serial_test).
Config schema & builder
crates/solver-config/src/lib.rs, crates/solver-config/src/builders/config.rs
Adds pub deny_list: Option<String> (with #[serde(default)]) to SolverConfig; adds a private deny_list: Option<String> to ConfigBuilder and a public deny_list(path: String) -> Self setter; builder propagates value into SolverConfig.
Runtime/operator config propagation
crates/solver-service/src/config_merge.rs, crates/solver-types/src/operator_config.rs, crates/solver-service/src/apis/admin.rs
Propagates new deny_list field into runtime SolverConfig, OperatorSolverConfig and admin API types; tests updated to initialize deny_list: None.
Intent handling / deny-list enforcement
crates/solver-core/src/handlers/intent.rs, crates/solver-core/src/engine/mod.rs
Adds denied_addresses: HashSet<String> to IntentHandler, implements load_deny_list(path) to read JSON array into the set, updates IntentHandler::new signature to accept &Config, and enforces deny-list in handle (publishes IntentRejected and skips storage for matches).

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • shahnami
  • pepebndc
  • NicoMolinaOZ

Poem

🐰 I nibble addresses in a neat little row,
I hop through configs where the KMS winds blow.
I hush the naughty intents that try to sneak,
I stamp them "rejected" with a clever squeak.
Carrots, crates, and tidy code — what a show! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description explains the purpose and use case but is missing key template sections including Testing Process and a completed Checklist with issue references and unit test confirmation. Add a Testing Process section and complete the Checklist by referencing related issues and confirming unit tests were added for the deny list functionality.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: Denylist' is concise and clearly describes the main feature addition, aligning with the primary objective of introducing a generalized deny list for EVM addresses.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can generate a title for your PR based on the changes.

Add @coderabbitai placeholder anywhere in the title of your PR and CodeRabbit will replace it with a title based on the changes in the PR. You can change the placeholder by changing the reviews.auto_title_placeholder setting.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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_list could return a Result or 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 0x prefix). 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

📥 Commits

Reviewing files that changed from the base of the PR and between f8543a1 and 12e4dbe.

📒 Files selected for processing (14)
  • Cargo.toml
  • crates/solver-account/Cargo.toml
  • crates/solver-config/src/builders/config.rs
  • crates/solver-config/src/lib.rs
  • crates/solver-core/Cargo.toml
  • crates/solver-core/src/handlers/intent.rs
  • crates/solver-delivery/Cargo.toml
  • crates/solver-demo/Cargo.toml
  • crates/solver-order/Cargo.toml
  • crates/solver-pricing/Cargo.toml
  • crates/solver-service/Cargo.toml
  • crates/solver-service/src/config_merge.rs
  • crates/solver-settlement/Cargo.toml
  • crates/solver-storage/Cargo.toml

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🔴 Critical

Tests are missing the new static_config parameter.

The IntentHandler::new() signature requires both dynamic_config: Arc<RwLock<Config>> and static_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_list is None). 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

📥 Commits

Reviewing files that changed from the base of the PR and between 12e4dbe and 7a643e8.

📒 Files selected for processing (14)
  • Cargo.toml
  • crates/solver-account/Cargo.toml
  • crates/solver-config/src/lib.rs
  • crates/solver-core/Cargo.toml
  • crates/solver-core/src/engine/mod.rs
  • crates/solver-core/src/handlers/intent.rs
  • crates/solver-delivery/Cargo.toml
  • crates/solver-demo/Cargo.toml
  • crates/solver-order/Cargo.toml
  • crates/solver-pricing/Cargo.toml
  • crates/solver-service/Cargo.toml
  • crates/solver-service/src/config_merge.rs
  • crates/solver-settlement/Cargo.toml
  • crates/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
Copy link
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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🔴 Critical

Add missing toml dependency to workspace.

The CI pipeline failure is caused by crates/solver-account/Cargo.toml referencing toml = { workspace = true } without a corresponding entry in the root Cargo.toml's [workspace.dependencies] section. Add toml to 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 using dep:mockall to limit the public feature surface.

testing = ["mockall"] exposes both the testing feature and an implicit mockall feature publicly. If testing should be the only public feature for opting into mockall, use dep:mockall instead 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:

  1. load_deny_list with valid file, missing file, malformed JSON
  2. Intent rejection when sender is on deny list
  3. Intent rejection when recipient is on deny list
  4. Behavior when intent.data cannot be parsed

Do 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_addresses is empty, but this includes the intentional case where no deny_list path 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 0x prefix, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7a643e8 and 7b147cf.

📒 Files selected for processing (14)
  • Cargo.toml
  • crates/solver-account/Cargo.toml
  • crates/solver-config/src/lib.rs
  • crates/solver-core/Cargo.toml
  • crates/solver-core/src/engine/mod.rs
  • crates/solver-core/src/handlers/intent.rs
  • crates/solver-delivery/Cargo.toml
  • crates/solver-demo/Cargo.toml
  • crates/solver-order/Cargo.toml
  • crates/solver-pricing/Cargo.toml
  • crates/solver-service/Cargo.toml
  • crates/solver-service/src/config_merge.rs
  • crates/solver-settlement/Cargo.toml
  • crates/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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🔴 Critical

Missing static_config parameter causes compilation failure.

Same issue as the previous test—IntentHandler::new requires static_config: &Config but 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 | 🔴 Critical

Missing static_config parameter causes compilation failure.

The IntentHandler::new signature now requires static_config: &Config as the last parameter (line 79), but this test passes only config without &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 like create_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 | 🟠 Major

User address may lack 0x prefix, causing match failure.

The deny list stores addresses with 0x prefix (line 115 preserves existing prefixes, line 203 explicitly adds 0x for recipients), but order_data.user is only lowercased without ensuring the 0x prefix. If the user field doesn't include the prefix, it will never match a deny list entry. The with_0x_prefix utility 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 | 🟡 Minor

Silent skip on parse failure may bypass sanctions enforcement.

If intent.data cannot be parsed as Eip7683OrderData, 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_addresses is empty, but this conflates three different scenarios:

  1. No deny_list path configured (intentional, no warning needed)
  2. Path configured but file missing/parse failed (warning already emitted by load_deny_list)
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7b147cf and b35f614.

📒 Files selected for processing (7)
  • crates/solver-config/src/builders/config.rs
  • crates/solver-config/src/lib.rs
  • crates/solver-core/Cargo.toml
  • crates/solver-core/src/engine/mod.rs
  • crates/solver-core/src/handlers/intent.rs
  • crates/solver-service/src/config_merge.rs
  • crates/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

Copy link
Collaborator

@nahimterrazas nahimterrazas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jonas089
Copy link
Contributor Author

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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (3)
crates/solver-core/src/handlers/intent.rs (2)

127-127: ⚠️ Potential issue | 🟠 Major

Address canonicalization is still inconsistent (0x prefix 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 | 🟡 Minor

Parse 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 | 🔴 Critical

Deny-list value is still dropped during initial merge (compliance/security risk).

At Line 455, deny_list is hardcoded to None, 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 SeedOverrides does not yet expose deny_list, add it there and thread it through JSON parsing as Option<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

📥 Commits

Reviewing files that changed from the base of the PR and between b35f614 and a641541.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • crates/solver-account/Cargo.toml
  • crates/solver-config/src/builders/config.rs
  • crates/solver-core/Cargo.toml
  • crates/solver-core/src/handlers/intent.rs
  • crates/solver-service/src/apis/admin.rs
  • crates/solver-service/src/config_merge.rs
  • crates/solver-settlement/Cargo.toml
  • crates/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
Copy link

codecov bot commented Mar 14, 2026

Codecov Report

❌ Patch coverage is 91.52542% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/solver-core/src/handlers/intent.rs 94.2% 6 Missing ⚠️
crates/solver-config/src/builders/config.rs 33.3% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@jonas089
Copy link
Contributor Author

@nahimterrazas

Hey, thank you for your thorough review and comments.
I think that I have addressed everything that you mentioned in your message.:

  • The denylist is now part of the solver config and does not default to None
  • If a denylist is configured but can't be loaded we error out
  • If no denylist is configured we log a warning (because it should still be possible to use the solver without this feature)
  • The build is successful and CI is passing
  • Test coverage for loading and enforcing the denylist is improved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants