Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThis pull request introduces a new Suggested reviewers
✨ 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.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs (1)
246-255: 🧹 Nitpick | 🔵 TrivialAdd at least one
FetchCloner::new(..., Some(risk_service))test path.These updates correctly match the new signature, but all touched call sites keep
risk_serviceasNone, so this file still doesn’t exercise AML-gated behavior directly.Also applies to: 1763-1772, 1837-1846, 1910-1919
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs` around lines 246 - 255, Add a test path that passes Some(risk_service) into FetchCloner::new so AML-gated behavior is exercised: create a lightweight mock or stub that implements the same trait/interface the production Risk service uses (e.g., a MockRiskService or TestRiskService), instantiate it and pass Some(mock_risk_service) into FetchCloner::new in at least one of the tests that currently pass None, then assert the expected AML-related behavior (e.g., transactions blocked/filtered or specific error/results) to prove the gate works; apply the same change to the other FetchCloner::new call sites in this test file that still use None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config.example.toml`:
- Line 272: Replace the misleading literal value for the config key api-key with
a clearly non-sensitive placeholder or comment it out so users won't mistakenly
paste real credentials; locate the api-key entry (symbol "api-key") in the
example config and either prefix it with a comment marker or change the value to
an obvious placeholder like "<REPLACE_WITH_YOUR_API_KEY>" or
"YOUR_API_KEY_HERE".
- Around line 259-261: The env var prefixes in the [chainlink.risk] section are
incorrect: replace occurrences of MBV_CHAINLINK__RANGE_RISK__* with
MBV_CHAINLINK__RISK__* (e.g., update the comment lines that currently reference
MBV_CHAINLINK__RANGE_RISK__ENABLED and the other entries in the same section) so
they match the TOML section name and the test expectations; apply the same
replacement to all other env var comments in that section (the subsequent
entries referenced in the comment block).
In `@magicblock-aml/src/lib.rs`:
- Around line 167-179: The is_high_risk logic in assessment_from_value is
inverted: change the comparison so addresses with risk_score >=
self.risk_score_threshold are marked high risk. Update the construction of
AddressRiskAssessment in assessment_from_value to set is_high_risk based on
self.risk_score_threshold <= score (i.e. score meets or exceeds the threshold)
so it matches the threshold semantics and the behavior used elsewhere (e.g., the
earlier cache/write logic).
- Around line 73-79: The CREATE TABLE for address_risk_cache is invalid because
the PRIMARY KEY references network which is not declared; fix by making the
schema and all SQL usages consistent: add a network TEXT NOT NULL column to the
address_risk_cache definition (alongside address, risk_score, fetched_at_unix_s)
and keep PRIMARY KEY (network, address), then update the related
INSERT/UPSERT/ON CONFLICT statements (the SQL at the locations around lines
127-130 and 155-161) to target the same conflict key (network, address);
alternatively, if you intend a Solana-only cache, remove network from the
PRIMARY KEY and change those ON CONFLICT targets to just address—ensure
address_risk_cache, the columns (address, network, risk_score,
fetched_at_unix_s), and the ON CONFLICT keys are all consistent.
- Around line 45-52: The SQLite operations in read_cache and write_cache are
blocking and currently run on the async executor inside check_address; change
those DB accesses to run on a blocking thread: make read_cache and write_cache
async helpers that call tokio::task::spawn_blocking (or
tokio::runtime::Handle::spawn_blocking) and perform all rusqlite queries inside
the spawned closure. Update RiskService so the Connection is not locked on the
async runtime (replace the parking_lot::Mutex<Connection> usage by either
storing a DB path/handle and opening/using the Connection inside spawn_blocking,
or keep the Connection behind a sync-only lock used only inside the blocking
closure), and call await on the spawn_blocking join handle from check_address;
ensure function signatures for read_cache and write_cache are updated
accordingly and reference these symbols (RiskService, check_address, read_cache,
write_cache).
- Around line 74-76: The CREATE TABLE schema for address_risk_cache uses
risk_score REAL but the code writes and reads u64, so update the SQL in the
create statement to declare risk_score INTEGER NOT NULL; ensure any
insert/update that uses params![address, assessment.risk_score, ...] keeps
passing an integer type and the retrieval that does let risk_score: u64 =
row.get(0)? will then match the INTEGER column type; adjust the CREATE TABLE
string surrounding address_risk_cache and verify any related queries reference
the same column type.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 530-545: The extraction of signer addresses from
delegation_actions is verbose; simplify the nested filter_map/if by using
filter_map with a conditional expression so it directly returns the pubkey when
meta.is_signer is true. Replace the inner closure in the flat_map over
delegation_actions.iter() (which currently uses
instruction.accounts.iter().filter_map(|meta| { if meta.is_signer {
Some(meta.pubkey) } else { None } })) with a concise form such as
instruction.accounts.iter().filter_map(|meta| meta.is_signer.then(||
meta.pubkey)) and keep collecting into the BTreeSet as before.
- Around line 522-559: The validation currently calls risk_service.check_address
sequentially inside validate_post_delegation_action_addresses which can be slow;
change it to run the checks concurrently by collecting the unique addresses
(from the addresses BTreeSet) into a Vec and then spawning async tasks (e.g.,
with futures::future::try_join_all or a bounded stream like
futures::stream::FuturesUnordered with a concurrency limit) to call
risk_service.check_address(&addr.to_string()).await for each address, then
inspect the returned assessments and return the same
ChainlinkError::RiskyDelegationActionAddress if any assessment.is_high_risk
(preserving address and risk_score) otherwise return Ok(()). Ensure you await
the combined future and propagate the first error or convert high-risk
assessments into an Err as before.
In `@magicblock-chainlink/src/chainlink/mod.rs`:
- Around line 142-146: The call to RiskService::try_from_config is passing
&ledger_path which produces a &&Path unnecessarily; change the invocation to
pass ledger_path directly (i.e.,
RiskService::try_from_config(&chainlink_config.risk, ledger_path)?) so you pass
a &Path instead of a double reference, then keep the subsequent .map(Arc::new)
unchanged; locate this in the block constructing risk_service that references
chainlink_config.risk, ledger_path, and RiskService::try_from_config.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs`:
- Around line 246-255: Add a test path that passes Some(risk_service) into
FetchCloner::new so AML-gated behavior is exercised: create a lightweight mock
or stub that implements the same trait/interface the production Risk service
uses (e.g., a MockRiskService or TestRiskService), instantiate it and pass
Some(mock_risk_service) into FetchCloner::new in at least one of the tests that
currently pass None, then assert the expected AML-related behavior (e.g.,
transactions blocked/filtered or specific error/results) to prove the gate
works; apply the same change to the other FetchCloner::new call sites in this
test file that still use None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d681ffca-6dbe-4551-950e-dcbac7e31975
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlconfig.example.tomlmagicblock-aml/Cargo.tomlmagicblock-aml/src/lib.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/Cargo.tomlmagicblock-chainlink/src/chainlink/errors.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/tests/utils/test_context.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/config/mod.rsmagicblock-config/src/consts.rsmagicblock-config/src/tests.rstest-integration/test-chainlink/src/ixtest_context.rstest-integration/test-chainlink/src/test_context.rs
| # Enables/disables Range risk checks. | ||
| # Default: false | ||
| # Env: MBV_CHAINLINK__RANGE_RISK__ENABLED |
There was a problem hiding this comment.
Environment variable prefix in comments is incorrect.
The comments reference MBV_CHAINLINK__RANGE_RISK__* but based on the TOML section name [chainlink.risk] and the test assertions in magicblock-config/src/tests.rs (which use MBV_CHAINLINK__RISK__*), the correct prefix should be MBV_CHAINLINK__RISK__*.
📝 Proposed fix
[chainlink.risk]
# Enables/disables Range risk checks.
# Default: false
-# Env: MBV_CHAINLINK__RANGE_RISK__ENABLED
+# Env: MBV_CHAINLINK__RISK__ENABLED
enabled = falseApply the same fix to all other env var references in this section (lines 266, 271, 276, 281, 286).
📝 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.
| # Enables/disables Range risk checks. | |
| # Default: false | |
| # Env: MBV_CHAINLINK__RANGE_RISK__ENABLED | |
| # Enables/disables Range risk checks. | |
| # Default: false | |
| # Env: MBV_CHAINLINK__RISK__ENABLED |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config.example.toml` around lines 259 - 261, The env var prefixes in the
[chainlink.risk] section are incorrect: replace occurrences of
MBV_CHAINLINK__RANGE_RISK__* with MBV_CHAINLINK__RISK__* (e.g., update the
comment lines that currently reference MBV_CHAINLINK__RANGE_RISK__ENABLED and
the other entries in the same section) so they match the TOML section name and
the test expectations; apply the same replacement to all other env var comments
in that section (the subsequent entries referenced in the comment block).
| pub struct RiskService { | ||
| client: Client, | ||
| cache: Mutex<Connection>, | ||
| base_url: String, | ||
| api_key: String, | ||
| cache_ttl: Duration, | ||
| risk_score_threshold: u64, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "lib.rs" | grep magicblockRepository: magicblock-labs/magicblock-validator
Length of output: 932
🏁 Script executed:
head -n 170 magicblock-aml/src/lib.rs | tail -n 130Repository: magicblock-labs/magicblock-validator
Length of output: 3903
🏁 Script executed:
cat -n magicblock-aml/src/lib.rs | sed -n '40,175p'Repository: magicblock-labs/magicblock-validator
Length of output: 5024
🏁 Script executed:
rg "spawn_blocking" magicblock-aml/src/lib.rsRepository: magicblock-labs/magicblock-validator
Length of output: 62
Move the SQLite cache work off the async executor.
check_address() is on the async path, but read_cache and write_cache perform blocking rusqlite I/O operations (lines 126-132 and 154-163) under a parking_lot::Mutex on the runtime thread. This blocks the executor and can stall Tokio workers under load. Move DB access behind spawn_blocking or use an async SQLite wrapper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-aml/src/lib.rs` around lines 45 - 52, The SQLite operations in
read_cache and write_cache are blocking and currently run on the async executor
inside check_address; change those DB accesses to run on a blocking thread: make
read_cache and write_cache async helpers that call tokio::task::spawn_blocking
(or tokio::runtime::Handle::spawn_blocking) and perform all rusqlite queries
inside the spawned closure. Update RiskService so the Connection is not locked
on the async runtime (replace the parking_lot::Mutex<Connection> usage by either
storing a DB path/handle and opening/using the Connection inside spawn_blocking,
or keep the Connection behind a sync-only lock used only inside the blocking
closure), and call await on the spawn_blocking join handle from check_address;
ensure function signatures for read_cache and write_cache are updated
accordingly and reference these symbols (RiskService, check_address, read_cache,
write_cache).
| conn.execute_batch( | ||
| "CREATE TABLE IF NOT EXISTS address_risk_cache ( | ||
| address TEXT NOT NULL, | ||
| risk_score REAL, | ||
| fetched_at_unix_s INTEGER NOT NULL, | ||
| PRIMARY KEY (network, address) | ||
| )", |
There was a problem hiding this comment.
Make the cache key/schema internally consistent.
Line 78 references network in the primary key, but the table never declares that column, so CREATE TABLE fails when risk checks are enabled. If network is intentional, Line 130 and Lines 159-161 also need to key by it; otherwise make address the only primary key/conflict target.
Possible minimal fix for a Solana-only cache
conn.execute_batch(
"CREATE TABLE IF NOT EXISTS address_risk_cache (
address TEXT NOT NULL,
risk_score REAL,
fetched_at_unix_s INTEGER NOT NULL,
- PRIMARY KEY (network, address)
+ PRIMARY KEY (address)
)",
)?;Also applies to: 127-130, 155-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-aml/src/lib.rs` around lines 73 - 79, The CREATE TABLE for
address_risk_cache is invalid because the PRIMARY KEY references network which
is not declared; fix by making the schema and all SQL usages consistent: add a
network TEXT NOT NULL column to the address_risk_cache definition (alongside
address, risk_score, fetched_at_unix_s) and keep PRIMARY KEY (network, address),
then update the related INSERT/UPSERT/ON CONFLICT statements (the SQL at the
locations around lines 127-130 and 155-161) to target the same conflict key
(network, address); alternatively, if you intend a Solana-only cache, remove
network from the PRIMARY KEY and change those ON CONFLICT targets to just
address—ensure address_risk_cache, the columns (address, network, risk_score,
fetched_at_unix_s), and the ON CONFLICT keys are all consistent.
| "CREATE TABLE IF NOT EXISTS address_risk_cache ( | ||
| address TEXT NOT NULL, | ||
| risk_score REAL, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd magicblock-aml && cat -n src/lib.rs | sed -n '70,80p'Repository: magicblock-labs/magicblock-validator
Length of output: 495
🏁 Script executed:
cd magicblock-aml && cat -n src/lib.rs | sed -n '138,150p'Repository: magicblock-labs/magicblock-validator
Length of output: 492
🏁 Script executed:
cd magicblock-aml && wc -l src/lib.rsRepository: magicblock-labs/magicblock-validator
Length of output: 94
🏁 Script executed:
cd magicblock-aml && rg -n 'unwrap\(\)|expect\(' src/lib.rsRepository: magicblock-labs/magicblock-validator
Length of output: 62
🏁 Script executed:
cd magicblock-aml && sed -n '125,150p' src/lib.rsRepository: magicblock-labs/magicblock-validator
Length of output: 846
🏁 Script executed:
cd magicblock-aml && sed -n '149,170p' src/lib.rsRepository: magicblock-labs/magicblock-validator
Length of output: 785
🏁 Script executed:
cd magicblock-aml && sed -n '145,180p' src/lib.rsRepository: magicblock-labs/magicblock-validator
Length of output: 1119
Change risk_score column from REAL to INTEGER NOT NULL in the schema.
The code writes u64 values (line 155: params![address, assessment.risk_score, ...]) to a REAL column (line 76) but attempts to read them back as u64 (line 142: let risk_score: u64 = row.get(0)?). This type mismatch will cause InvalidColumnType errors at runtime on cache hits, since rusqlite strictly validates column type compatibility.
Schema fix
- risk_score REAL,
+ risk_score INTEGER NOT NULL,📝 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.
| "CREATE TABLE IF NOT EXISTS address_risk_cache ( | |
| address TEXT NOT NULL, | |
| risk_score REAL, | |
| "CREATE TABLE IF NOT EXISTS address_risk_cache ( | |
| address TEXT NOT NULL, | |
| risk_score INTEGER NOT NULL, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-aml/src/lib.rs` around lines 74 - 76, The CREATE TABLE schema for
address_risk_cache uses risk_score REAL but the code writes and reads u64, so
update the SQL in the create statement to declare risk_score INTEGER NOT NULL;
ensure any insert/update that uses params![address, assessment.risk_score, ...]
keeps passing an integer type and the retrieval that does let risk_score: u64 =
row.get(0)? will then match the INTEGER column type; adjust the CREATE TABLE
string surrounding address_risk_cache and verify any related queries reference
the same column type.
| fn assessment_from_value( | ||
| &self, | ||
| value: &Value, | ||
| ) -> RiskResult<AddressRiskAssessment> { | ||
| let score = value | ||
| .get("riskScore") | ||
| .and_then(|v| v.as_u64()) | ||
| .ok_or(RiskError::RiskScoreNotFound)?; | ||
|
|
||
| Ok(AddressRiskAssessment { | ||
| is_high_risk: self.risk_score_threshold >= score, | ||
| risk_score: score, | ||
| }) |
There was a problem hiding this comment.
Invert the high-risk comparison here.
risk-score-threshold is documented as the cutoff for high-risk addresses, but this code marks scores below the threshold as risky. That also disagrees with Line 144, so the same address can flip classification after its first cache write.
Suggested fix
Ok(AddressRiskAssessment {
- is_high_risk: self.risk_score_threshold >= score,
+ is_high_risk: score >= self.risk_score_threshold,
risk_score: score,
})📝 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.
| fn assessment_from_value( | |
| &self, | |
| value: &Value, | |
| ) -> RiskResult<AddressRiskAssessment> { | |
| let score = value | |
| .get("riskScore") | |
| .and_then(|v| v.as_u64()) | |
| .ok_or(RiskError::RiskScoreNotFound)?; | |
| Ok(AddressRiskAssessment { | |
| is_high_risk: self.risk_score_threshold >= score, | |
| risk_score: score, | |
| }) | |
| fn assessment_from_value( | |
| &self, | |
| value: &Value, | |
| ) -> RiskResult<AddressRiskAssessment> { | |
| let score = value | |
| .get("riskScore") | |
| .and_then(|v| v.as_u64()) | |
| .ok_or(RiskError::RiskScoreNotFound)?; | |
| Ok(AddressRiskAssessment { | |
| is_high_risk: score >= self.risk_score_threshold, | |
| risk_score: score, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-aml/src/lib.rs` around lines 167 - 179, The is_high_risk logic in
assessment_from_value is inverted: change the comparison so addresses with
risk_score >= self.risk_score_threshold are marked high risk. Update the
construction of AddressRiskAssessment in assessment_from_value to set
is_high_risk based on self.risk_score_threshold <= score (i.e. score meets or
exceeds the threshold) so it matches the threshold semantics and the behavior
used elsewhere (e.g., the earlier cache/write logic).
| async fn validate_post_delegation_action_addresses( | ||
| &self, | ||
| delegation_actions: &DelegationActions, | ||
| ) -> ChainlinkResult<()> { | ||
| let Some(risk_service) = self.risk_service.as_ref() else { | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| let addresses = delegation_actions | ||
| .iter() | ||
| .flat_map(|instruction| { | ||
| instruction | ||
| .accounts | ||
| .iter() | ||
| .filter_map(|meta| { | ||
| if meta.is_signer { | ||
| Some(meta.pubkey) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| }) | ||
| .collect::<BTreeSet<_>>(); | ||
|
|
||
| for address in addresses { | ||
| let assessment = | ||
| risk_service.check_address(&address.to_string()).await?; | ||
| if assessment.is_high_risk { | ||
| return Err(ChainlinkError::RiskyDelegationActionAddress { | ||
| address, | ||
| risk_score: assessment.risk_score, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider parallelizing address risk checks for better performance.
The current implementation checks addresses sequentially. If there are many signer addresses and the risk service has network latency, this could become a bottleneck. Consider using futures::future::try_join_all or similar for parallel checks with a concurrency limit.
That said, the current implementation is correct and simpler. Given that delegation actions typically have few signers, this may be acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs` around lines 522 -
559, The validation currently calls risk_service.check_address sequentially
inside validate_post_delegation_action_addresses which can be slow; change it to
run the checks concurrently by collecting the unique addresses (from the
addresses BTreeSet) into a Vec and then spawning async tasks (e.g., with
futures::future::try_join_all or a bounded stream like
futures::stream::FuturesUnordered with a concurrency limit) to call
risk_service.check_address(&addr.to_string()).await for each address, then
inspect the returned assessments and return the same
ChainlinkError::RiskyDelegationActionAddress if any assessment.is_high_risk
(preserving address and risk_score) otherwise return Ok(()). Ensure you await
the combined future and propagate the first error or convert high-risk
assessments into an Err as before.
| let addresses = delegation_actions | ||
| .iter() | ||
| .flat_map(|instruction| { | ||
| instruction | ||
| .accounts | ||
| .iter() | ||
| .filter_map(|meta| { | ||
| if meta.is_signer { | ||
| Some(meta.pubkey) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| }) | ||
| .collect::<BTreeSet<_>>(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting addresses more concisely.
The nested filter_map with explicit if can be simplified.
📝 Suggested simplification
let addresses = delegation_actions
.iter()
.flat_map(|instruction| {
- instruction
- .accounts
- .iter()
- .filter_map(|meta| {
- if meta.is_signer {
- Some(meta.pubkey)
- } else {
- None
- }
- })
- .collect::<Vec<_>>()
+ instruction.accounts.iter().filter(|meta| meta.is_signer).map(|meta| meta.pubkey)
})
.collect::<BTreeSet<_>>();📝 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.
| let addresses = delegation_actions | |
| .iter() | |
| .flat_map(|instruction| { | |
| instruction | |
| .accounts | |
| .iter() | |
| .filter_map(|meta| { | |
| if meta.is_signer { | |
| Some(meta.pubkey) | |
| } else { | |
| None | |
| } | |
| }) | |
| .collect::<Vec<_>>() | |
| }) | |
| .collect::<BTreeSet<_>>(); | |
| let addresses = delegation_actions | |
| .iter() | |
| .flat_map(|instruction| { | |
| instruction.accounts.iter().filter(|meta| meta.is_signer).map(|meta| meta.pubkey) | |
| }) | |
| .collect::<BTreeSet<_>>(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs` around lines 530 -
545, The extraction of signer addresses from delegation_actions is verbose;
simplify the nested filter_map/if by using filter_map with a conditional
expression so it directly returns the pubkey when meta.is_signer is true.
Replace the inner closure in the flat_map over delegation_actions.iter() (which
currently uses instruction.accounts.iter().filter_map(|meta| { if meta.is_signer
{ Some(meta.pubkey) } else { None } })) with a concise form such as
instruction.accounts.iter().filter_map(|meta| meta.is_signer.then(||
meta.pubkey)) and keep collecting into the BTreeSet as before.
| let risk_service = RiskService::try_from_config( | ||
| &chainlink_config.risk, | ||
| &ledger_path, | ||
| )? | ||
| .map(Arc::new); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Redundant reference.
ledger_path is already &Path, so &ledger_path creates &&Path. While this works due to auto-deref, it's cleaner to pass ledger_path directly.
📝 Suggested fix
let risk_service = RiskService::try_from_config(
&chainlink_config.risk,
- &ledger_path,
+ ledger_path,
)?
.map(Arc::new);📝 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.
| let risk_service = RiskService::try_from_config( | |
| &chainlink_config.risk, | |
| &ledger_path, | |
| )? | |
| .map(Arc::new); | |
| let risk_service = RiskService::try_from_config( | |
| &chainlink_config.risk, | |
| ledger_path, | |
| )? | |
| .map(Arc::new); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-chainlink/src/chainlink/mod.rs` around lines 142 - 146, The call
to RiskService::try_from_config is passing &ledger_path which produces a &&Path
unnecessarily; change the invocation to pass ledger_path directly (i.e.,
RiskService::try_from_config(&chainlink_config.risk, ledger_path)?) so you pass
a &Path instead of a double reference, then keep the subsequent .map(Arc::new)
unchanged; locate this in the block constructing risk_service that references
chainlink_config.risk, ledger_path, and RiskService::try_from_config.
1 participant
Summary
Compatibility
No breaking change, but new optional config fields:
Summary by CodeRabbit
Release Notes
New Features
Configuration
[chainlink.risk]configuration block with toggles for risk checks, API endpoints, caching parameters, and risk thresholds.