268 siwe admin auth module #282
Merged
Conversation
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
nahimterrazas
requested review from
NicoMolinaOZ,
pepebndc and
shahnami
as code owners
📝 WalkthroughWalkthroughAdds a wallet‑based Admin API with EIP‑712/personal_sign verification, nonce management, storage-backed OperatorConfig with optimistic locking, hot‑reloadable runtime Config (Arc<RwLock>), atomic storage primitives across backends, a /health readiness endpoint, and widespread call‑site/test updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as rgba(63,81,181,0.5)
participant Verifier as rgba(3,169,244,0.5)
participant Nonce as rgba(76,175,80,0.5)
participant Store as rgba(255,153,51,0.5)
participant Crypto as rgba(233,30,99,0.5)
Client->>API: GET /api/v1/admin/nonce
API->>Verifier: generate_nonce()
Verifier->>Nonce: generate()
Nonce-->>Verifier: nonce + ttl
Verifier-->>API: NonceResponse
API-->>Client: NonceResponse
Client->>Client: Sign action (EIP‑712 / personal_sign)
Client->>API: POST /api/v1/admin/tokens (SignedAdminRequest)
API->>Verifier: verify(action, signature)
Verifier->>Nonce: exists(action.nonce)
alt nonce valid
Verifier->>Crypto: recover_from_hash(action_hash, signature)
Crypto-->>Verifier: signer address
Verifier->>Verifier: is_admin(address)
Verifier->>Nonce: consume(nonce)
alt admin authorized
Verifier-->>API: verified signer
API->>Store: load -> compare_and_swap update (operator config)
Store-->>API: updated
API-->>Client: AdminActionResponse(success)
else not admin
Verifier-->>API: AdminAuthError(NotAuthorized)
API-->>Client: error
end
else invalid/used/expired nonce
Verifier-->>API: AdminAuthError(nonce issue)
API-->>Client: error
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
…nt with EIP-712 signature verification
…nt with EIP-712 signature verification
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@crates/solver-service/src/apis/admin.rs`:
- Around line 85-94: The nonce handling is inconsistent: handle_get_nonce
returns a String but AddTokenContents expects a u64 (request.contents.nonce), so
change the API to use a consistent nonce type; choose one of the suggested fixes
and implement it across the codebase—either (A) change AddTokenContents.nonce
and all uses (including verifier.verify and nonce_string construction) to String
so the server and client pass the original nonce string unchanged, or (B) add a
separate field (e.g., original_nonce: String) to the request payload to carry
the exact string while keeping a numeric nonce for internal use, or (C) switch
generate_nonce to emit numeric-only nonces that safely convert to/from u64 and
update tests to use the returned nonce instead of hardcoding 1; update
handle_get_nonce, AddTokenContents, any tests that construct AddTokenContents,
and the call site building nonce_string before calling verifier.verify to match
the chosen approach.
In `@crates/solver-service/src/auth/admin/error.rs`:
- Around line 57-76: The IntoResponse implementation for AdminAuthError leaks
internal details by using self.to_string() for all variants; modify
into_response so that for AdminAuthError::Internal the
AdminAuthErrorResponse.error_description is a generic message like "internal
server error" (or similar) instead of the internal error string, and separately
log the detailed error server-side (e.g., using tracing::error! or the existing
logger) when matching AdminAuthError::Internal; keep all other variants'
error_description as self.to_string(); update the match arm or response
construction in the into_response method (referencing AdminAuthError::Internal,
into_response, and AdminAuthErrorResponse).
In `@crates/solver-service/src/auth/admin/types.rs`:
- Around line 219-250: The struct_hash implementation on WithdrawContents
silently maps invalid amount strings to zero via
U256::from_str_radix(...).unwrap_or(U256::ZERO); change
WithdrawContents::struct_hash to return Result<FixedBytes<32>, Error> (or a
crate error type) and propagate any parsing error from U256::from_str_radix
instead of defaulting to zero, updating the AdminAction trait signature for
struct_hash() to Result<FixedBytes<32>, Error> and adjust all callers to handle
the error; ensure you reference WithdrawContents::struct_hash, the
U256::from_str_radix call, and the AdminAction trait when making these changes.
In `@crates/solver-storage/src/nonce_store.rs`:
- Around line 110-115: The debug log currently prints self.redis_url which may
contain credentials; change the logging to redact or omit credentials before
logging (e.g., implement a small helper redact_redis_url(self.redis_url) or
parse with url::Url and replace the password with "<redacted>" or log only
host:port), then pass the redacted string to the debug! call that currently
includes redis_url and solver_id (the block that logs "Nonce store Redis
connection established" and returns Ok(Arc::new(manager))). Also apply the same
redaction to the other debug call mentioned (the similar debug! around the
second occurrence) so no raw redis URI is ever emitted.
- Around line 77-88: In NonceStore::new (pub fn new(redis_url: &str, solver_id:
&str, ttl_seconds: u64)), add a validation that rejects ttl_seconds == 0 and
return an appropriate NonceError::Configuration with a clear message (e.g., "TTL
seconds must be > 0") to prevent immediate nonce expiry; update the constructor
to check ttl_seconds before completing initialization and ensure any tests or
callers handle the new error case.
- Around line 21-27: The OnceCell currently stores Arc<ConnectionManager> which
causes Arc::make_mut to clone the inner ConnectionManager (bypassing caching);
change the OnceCell type to store ConnectionManager directly
(OnceCell<ConnectionManager>), initialize it with the ConnectionManager (no
Arc), remove all Arc::make_mut calls in get_connection and other helpers (e.g.,
places around lines referenced in the review) and instead return a cloned
ConnectionManager from the OnceCell when a caller needs a manager; update
callers of get_connection (and code paths at the noted locations) to
accept/handle a cloned ConnectionManager rather than an Arc-wrapped one so the
ConnectionManager reuse and cheap cloning semantics are preserved.
In `@crates/solver-types/src/auth.rs`:
- Around line 145-152: The is_admin method on AdminConfig currently ignores the
enabled flag and only checks admin_addresses, enabling auth bypass; update
AdminConfig::is_admin(&self, address: &Address) to first return false if
self.enabled is false, otherwise perform the existing
admin_addresses.iter().any(|a| a == address) check so callers of is_admin (e.g.,
signature/verify flows) respect the enabled toggle.
🧹 Nitpick comments (4)
crates/solver-storage/Cargo.toml (1)
27-27: Redundantuuidin dev-dependencies.Since
uuidis now listed under[dependencies](line 27), it's automatically available for tests. The entry in[dev-dependencies](line 35) is redundant and can be removed.🧹 Suggested cleanup
[dev-dependencies] tempfile = { workspace = true } futures = { workspace = true } -uuid = { workspace = true } rust_decimal = { workspace = true }Also applies to: 35-35
crates/solver-service/src/auth/admin/types.rs (1)
324-344: Prefer&[u8]over&Vec<u8>for function parameters.Using
&[u8]is more idiomatic and flexible as it accepts any slice-like type.Proposed fix
- pub fn serialize<S>(bytes: &Vec<u8>, serializer: S) -> Result<S::Ok, S::Error> + pub fn serialize<S>(bytes: &[u8], serializer: S) -> Result<S::Ok, S::Error> where S: Serializer, {crates/solver-service/src/apis/admin.rs (2)
96-104: Reminder: Address the TODO comment for token persistence.The endpoint currently only logs the action without persisting the token configuration. This should be tracked as a follow-up task.
Do you want me to open an issue to track implementing the actual token persistence to the config store?
106-114: UseDisplayinstead ofDebugfor the admin address.
alloy_primitives::AddressimplementsDisplayto produce EIP-55 checksummed hex format (e.g.,0xd8dA6bf...), whereasformat!("{:?}", admin)usesDebugwhich produces a different representation. Useformat!("{}", admin)oradmin.to_string()for consistency with line 99's tracing macro and to produce the expected checksummed hex output.Proposed fix
Ok(Json(AdminActionResponse { success: true, message: format!( "Token {} added to chain {}", request.contents.symbol, request.contents.chain_id ), - admin: format!("{:?}", admin), + admin: format!("{}", admin), }))
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ror for amount validation
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/solver-storage/src/nonce_store.rs`:
- Around line 8-18: The doc example currently passes &nonce to exists/consume
but the NonceStore API uses u64 nonces; update the example so the value returned
by NonceStore::generate (referenced as generate) is treated as a u64 and call
exists and consume with that u64 (e.g., exists(nonce) and consume(nonce)) rather
than &nonce, keeping NonceStore::new and the async flow unchanged.
…zation logic to ensure disabled state correctly rejects unauthorized access
…entication features
…entication features
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/solver-service/src/server.rs`:
- Around line 118-122: The chain_id selection is non-deterministic due to
HashMap iteration; change the assignment of chain_id (currently using
config.networks.keys().next().copied().unwrap_or(1)) to a deterministic choice
such as config.networks.keys().copied().min().unwrap_or(1) (or read an explicit
chain_id from config) so AdminActionVerifier receives a stable chain_id when
computing the EIP-712 domain separator and signature verification succeeds.
shahnami
reviewed
Jan 27, 2026
shahnami
reviewed
Jan 27, 2026
…ge backends and improve error handling
…ge backends and improve error handling
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/solver-core/src/builder/mod.rs`:
- Around line 60-64: The builder doc comment incorrectly suggests TokenManager
reads dynamic config; update the comment to clarify that the builder takes a
static snapshot of config and services created from it (including TokenManager,
which is constructed from static_config.networks) will NOT see hot-reload
changes, and only services explicitly implemented to read dynamic config at
runtime will observe hot-reloads; reference TokenManager and
static_config.networks in the reworded note so it’s clear TokenManager uses the
static snapshot rather than dynamic config.
In `@crates/solver-service/src/server.rs`:
- Around line 195-226: The code currently swallows create_nonce_store errors and
returns None, silently disabling admin routes; change the control flow so admin
initialization fails fast by returning an error when create_nonce_store returns
Err instead of logging and returning None. Update the caller/signature that
produces AdminApiState (currently returning Option<AdminApiState>) to return
Result<AdminApiState, E> (or propagate the error with ?), and in the Err(e)
branch of create_nonce_store return Err(e). Ensure the AdminApiState
construction (verifier, nonce_store, config_store, dynamic_config) remains
unchanged and include contextual error text when converting/propagating the
error.
🧹 Nitpick comments (1)
crates/solver-config/src/builders/config.rs (1)
41-47: Centralize the gas buffer default to avoid magic numbers.💡 Suggested refactor
use std::collections::HashMap; +const DEFAULT_GAS_BUFFER_BPS: u32 = 1_000; + @@ pub fn new() -> Self { Self { solver_id: "test-solver".to_string(), monitoring_timeout_seconds: 60, min_profitability_pct: Decimal::ZERO, - gas_buffer_bps: 1000, // 10% default + gas_buffer_bps: DEFAULT_GAS_BUFFER_BPS, // 10% default storage_primary: "memory".to_string(),
… multiple backends
… multiple backends
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/solver-service/src/main.rs`:
- Around line 9-12: Remove the duplicated sentence in the crate-level
documentation comment at the top of main.rs (the module-level doc comment
starting with "The solver uses a storage backend..."); delete the extra repeated
line ("on subsequent startups.") so the documentation only contains the sentence
once and keep the surrounding paragraphs intact.
🧹 Nitpick comments (3)
crates/solver-storage/src/readiness/mod.rs (1)
233-247: Consider removing empty log statement.Line 237 logs an empty string which adds visual noise without value. This is a minor cosmetic issue.
♻️ Suggested fix
tracing::error!(" STARTUP BLOCKED: Storage not ready"); tracing::error!("════════════════════════════════════════════════════════════"); - tracing::error!(""); tracing::error!(" To fix:"); tracing::error!(" 1. Address the failed checks above OR"); tracing::error!(" 2. Set REQUIRE_PERSISTENCE=false (not recommended)"); - tracing::error!(""); tracing::error!(" See docs/redis-persistence.md for instructions.");crates/solver-service/src/apis/health.rs (2)
88-107: Consider cachingStoreConfiginAppStateinstead of reading environment on each request.
StoreConfig::from_env()is called on every health check invocation, reading environment variables repeatedly. While environment variables are typically stable, this adds unnecessary overhead and could cause inconsistency if env vars change during runtime.Consider storing
StoreConfiginAppStateduring server initialization:// In AppState (hypothetical): pub struct AppState { pub config: Arc<RwLock<Config>>, pub store_config: StoreConfig, // Add this // ... } // In handle_health: let store_config = state.store_config.clone();
119-126: Consider using constants for check names to prevent silent failures from typos.The string matching against
"connectivity","rdb_persistence", and"aof_persistence"is fragile. If the readiness checker changes these names, the health endpoint would silently report false values.♻️ Extract constants (or import from readiness module)
// In readiness module or shared constants: pub const CHECK_CONNECTIVITY: &str = "connectivity"; pub const CHECK_RDB_PERSISTENCE: &str = "rdb_persistence"; pub const CHECK_AOF_PERSISTENCE: &str = "aof_persistence"; // In health.rs: match check.name.as_str() { CHECK_CONNECTIVITY => connected = check.passed, CHECK_RDB_PERSISTENCE => rdb_enabled = check.passed, CHECK_AOF_PERSISTENCE => aof_enabled = check.passed, _ => {}, }
…ix for instance isolation
…ix for instance isolation
shahnami
reviewed
Feb 2, 2026
shahnami
reviewed
Feb 2, 2026
shahnami
reviewed
Feb 2, 2026
shahnami
reviewed
Feb 2, 2026
shahnami
reviewed
Feb 2, 2026
Collaborator
|
Changes look good! Just a few more questions |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@crates/solver-service/src/apis/admin.rs`:
- Around line 112-129: The JSON examples in the doc comments that show the
request body with "nonce" as a number must be changed to a string because the
API now returns/uses nonce as a string to preserve precision; update the "nonce"
field in the illustrated request body (the doc comment describing the
signature/contents block) and the other occurrence later in the same file so
they show e.g. "nonce": "12345678901234" (retain quotes) everywhere the request
example appears.
In `@crates/solver-service/src/apis/health.rs`:
- Around line 88-106: The health endpoint is returning raw error strings (e.g.,
the Err from StoreConfig::from_env serialized into HealthResponse.storage.error
and similarly in the Redis/other health branches), which can leak sensitive
internals; replace those direct e.to_string() usages with a sanitized public
message like "internal error" or "unavailable", move the full e (or context)
into a server-side log (use your existing logger or tracing::error) and
optionally add a helper sanitize_error_for_public(err: &anyhow::Error) -> String
used by HealthResponse; update the places that populate
HealthResponse.storage.error and the Redis-related health error to use the
sanitized message while logging the original error.
- Around line 69-87: The API docs above the handle_health function are out of
sync: update the example JSON in the doc comment to include the required
"storage" field (e.g., "storage":
{"backend":"Redis","ready":true,"checks":[],"details":{}}) and change the
status-code descriptions to reflect storage readiness (replace "Healthy (Redis
connected)" / "Unhealthy (Redis disconnected)" with "Healthy (storage ready)" /
"Unhealthy (storage not ready)"). Make these edits in the comment block
immediately preceding pub async fn handle_health(State(state): State<AppState>)
-> impl IntoResponse so the docs match the current response schema.
In `@crates/solver-storage/src/lib.rs`:
- Around line 395-424: redact_url_credentials currently only redacts when a
scheme ("scheme://") exists; update it to also redact schemeless userinfo by
detecting userinfo anywhere in the input. Specifically, in
redact_url_credentials: if the string contains an '@' and that '@' occurs before
the first '/' (or if no '/' exists), treat the substring before the '@' as
userinfo and replace it with "[REDACTED]" (preserving any scheme if present);
otherwise keep the existing logic (scheme-based redaction). This ensures inputs
like "user:pass@host:port" are redacted even without "scheme://".
- Around line 607-692: Tests that call StoreConfig::from_env mutate global
environment variables (STORAGE_BACKEND, REDIS_URL, STORAGE_PATH) without
restoring prior values, causing flakiness when run in parallel; wrap each
env-mutating test (e.g., test_store_config_from_env_redis_default,
test_store_config_from_env_redis_custom_url,
test_store_config_from_env_file_default_path,
test_store_config_from_env_file_custom_path, test_store_config_from_env_memory,
test_store_config_from_env_unsupported) with an env-guard that saves previous
values and restores them on drop or serialize the tests; implement a small test
helper (EnvGuard) used at the start of each test to snapshot and restore
STORAGE_BACKEND, REDIS_URL, STORAGE_PATH (or use an existing crate like temp_env
or add #[serial] via serial_test) and ensure every test that mutates env calls
the guard so StoreConfig::from_env reads deterministic values and no global
state leaks between tests.
shahnami
approved these changes
Feb 2, 2026
Collaborator
shahnami
left a comment
shahnami
left a comment
There was a problem hiding this comment.
I still think the whole "storage readiness" is a bit strange (I can't pinpoint it), but fine for now. LGTM, thanks for your hard work!
… multiple backends
… multiple backends
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/solver-demo/.env.example`:
- Line 19: The file ends without a trailing newline which fails dotenv-linter;
update the .env example by adding a single newline at end of file so the
JWT_SECRET=MySuperDuperSecureSecret123! line is terminated with a trailing
newline character (ensure the file ends with that newline).
🧹 Nitpick comments (1)
crates/solver-demo/.env.example (1)
19-19: Use a placeholder JWT secret to avoid accidental reuse in real deployments.Keeping a real-looking secret in
.env.examplecan lead to copy‑paste into production. Consider a clear placeholder and note that it must be replaced.📝 Proposed change
-JWT_SECRET=MySuperDuperSecureSecret123! +JWT_SECRET=change-me-in-prod
NicoMolinaOZ
approved these changes
Feb 2, 2026
pepebndc
approved these changes
Feb 3, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit
New Features
Chores
Tests
Bug Fixes
Documentation