feat: Handle post delegation actions: detect actions, parse and execute on the ER#999
Open
feat: Handle post delegation actions: detect actions, parse and execute on the ER#999
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
📝 WalkthroughWalkthroughThreads delegation actions through the chainlink/cloner flow: adds a new DelegationActions type and a 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 105-107: The dependency entry for magicblock-delegation-program
currently uses a local path ("path = \"../delegation-program\"") which will
break CI and isolated checkouts; change it back to a VCS-based dependency
(restore the previous git + rev or git + tag/branch) or vendor/subtree the
delegation-program into this repo and update the Cargo.toml entry accordingly so
CI can fetch it; locate and update the magicblock-delegation-program dependency
line in Cargo.toml to use the git URL and fixed rev (or replace the path with
the vendored crate name) instead of the local path.
In `@magicblock-account-cloner/src/lib.rs`:
- Around line 165-167: Before extending ixs with request.delegation_actions,
compute the serialized size that would result (e.g., temporarily clone ixs,
extend with request.delegation_actions.clone(), serialize a Transaction built
with those instructions or compute serialized instruction bytes) and if it
exceeds Solana's 1232-byte limit return an error or split the instructions into
a separate transaction; specifically update the block that currently does
ixs.extend(request.delegation_actions.clone()) to perform this size check prior
to calling Transaction::new_with_payer or send_transaction and either reject the
request with a clear error or chunk delegation_actions into additional
transactions so Transaction::new_with_payer/send_transaction never receives an
oversized transaction.
- Around line 165-167: The code currently clones the entire Vec with
request.delegation_actions.clone() before extending ixs, which causes an
unnecessary allocation; change the extension to use
ixs.extend_from_slice(&request.delegation_actions) (or
ixs.extend(request.delegation_actions.iter().cloned()) if types differ) so you
avoid the intermediate Vec allocation—update the block that checks
request.delegation_actions and replace the clone-based extend with
extend_from_slice referencing ixs and request.delegation_actions.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs`:
- Around line 301-308: The empty layout-check using
dlp::state::DelegationRecord::size_with_discriminator() followed by an if/else
that only contains comments is dead code and should be fixed: either remove the
entire if/else block to avoid misleading readers, or implement the intended
per-layout behavior by making parse_delegation_actions conditional (call
parse_delegation_actions only when delegation_record_data.data().len() >
delegation_record_size to handle the DelegateWithActions layout); locate the
check around delegation_record_size, delegation_record_data, and the
parse_delegation_actions call and apply one of these two fixes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlmagicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/errors.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/cloner/mod.rstest-integration/test-chainlink/src/ixtest_context.rs
Comment on lines
+105
to
107
| magicblock-delegation-program = { path = "../delegation-program", default-features = false, features = [ | ||
| "sdk", | ||
| ] } |
Contributor
There was a problem hiding this comment.
Local path dependency ../delegation-program will break CI and isolated checkouts.
path = "../delegation-program" references a sibling directory outside this repository. Any developer or CI runner without that exact directory layout will get a build failure. The previous git-based reference (with rev) was portable. This should either stay as a git dependency (possibly pointing to a local branch) or the delegation-program should be vendored/subtree'd into this repo before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` around lines 105 - 107, The dependency entry for
magicblock-delegation-program currently uses a local path ("path =
\"../delegation-program\"") which will break CI and isolated checkouts; change
it back to a VCS-based dependency (restore the previous git + rev or git +
tag/branch) or vendor/subtree the delegation-program into this repo and update
the Cargo.toml entry accordingly so CI can fetch it; locate and update the
magicblock-delegation-program dependency line in Cargo.toml to use the git URL
and fixed rev (or replace the path with the vendored crate name) instead of the
local path.
Comment on lines
+165
to
+167
| if !request.delegation_actions.is_empty() { | ||
| ixs.extend(request.delegation_actions.clone()); | ||
| } |
Contributor
There was a problem hiding this comment.
No transaction size guard before appending delegation_actions.
Appending arbitrary instructions from delegation_actions to the existing ixs slice can push the serialized Transaction past Solana's 1232-byte limit, which will cause a runtime send failure. There is no size check before Transaction::new_with_payer or send_transaction.
Consider computing the serialized size of ixs after extension and returning an error (or chunking into a separate transaction) when the limit would be exceeded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-account-cloner/src/lib.rs` around lines 165 - 167, Before
extending ixs with request.delegation_actions, compute the serialized size that
would result (e.g., temporarily clone ixs, extend with
request.delegation_actions.clone(), serialize a Transaction built with those
instructions or compute serialized instruction bytes) and if it exceeds Solana's
1232-byte limit return an error or split the instructions into a separate
transaction; specifically update the block that currently does
ixs.extend(request.delegation_actions.clone()) to perform this size check prior
to calling Transaction::new_with_payer or send_transaction and either reject the
request with a clear error or chunk delegation_actions into additional
transactions so Transaction::new_with_payer/send_transaction never receives an
oversized transaction.
🧹 Nitpick | 🔵 Trivial
Unnecessary full-Vec clone; prefer extend_from_slice.
request.delegation_actions.clone() allocates a full Vec copy before iterating. Since request is already a reference, extend_from_slice avoids the intermediate allocation.
♻️ Proposed refactor
- if !request.delegation_actions.is_empty() {
- ixs.extend(request.delegation_actions.clone());
- }
+ ixs.extend_from_slice(&request.delegation_actions);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if !request.delegation_actions.is_empty() { | |
| ixs.extend(request.delegation_actions.clone()); | |
| } | |
| ixs.extend_from_slice(&request.delegation_actions); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-account-cloner/src/lib.rs` around lines 165 - 167, The code
currently clones the entire Vec with request.delegation_actions.clone() before
extending ixs, which causes an unnecessary allocation; change the extension to
use ixs.extend_from_slice(&request.delegation_actions) (or
ixs.extend(request.delegation_actions.iter().cloned()) if types differ) so you
avoid the intermediate Vec allocation—update the block that checks
request.delegation_actions and replace the clone-based extend with
extend_from_slice referencing ixs and request.delegation_actions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 462-473: The length-check using
DelegationRecord::size_with_discriminator() and its empty if/else branches is
dead code; remove the whole if/else block (the delegation_record_size
calculation and the conditional that checks account.data().len() ==
delegation_record_size) and leave the existing return tuple
(Some(account.into_account_shared_data()), None, vec![]) directly; if per-layout
behavior is needed later, reintroduce a clearly separated match or functions
around DelegationRecord layout handling instead of empty branches.
- Line 451: Replace the use of unwrap_or(vec![]) with the idiomatic
unwrap_or_default() on delegation_actions; locate the expression
delegation_actions.unwrap_or(vec![]) (inside the call building whatever uses
delegation_actions) and change it to delegation_actions.unwrap_or_default() so
the code uses Vec's Default implementation instead of constructing an empty vec
manually.
- Around line 337-341: The function's return signature currently spells out the
third return type as Vec<solana_instruction::Instruction>; replace that explicit
type with the existing alias DelegationActions so the signature returns
(Option<AccountSharedData>, Option<DelegationRecord>, DelegationActions)
instead, updating any matching use sites or pattern matches if needed to
preserve names and types (refer to the function whose signature ends with the
three-tuple return and the DelegationActions alias already imported).
In `@magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs`:
- Line 345: Replace the explicit vec![] fallback with unwrap_or_default() on the
Option variable (delegation_actions) so it mirrors the other call in mod.rs;
locate the usage of delegation_actions.unwrap_or(vec![]) in pipeline.rs and
change it to delegation_actions.unwrap_or_default(), making sure
delegation_actions is an Option<Vec<...>> (Vec implements Default) so the
compiler accepts the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs
snawaz
force-pushed
the
snawaz/post-delegation-actions
branch
from
874475d to
74a0319
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs (1)
279-280:⚠️ Potential issue | 🟠 MajorCancel all fetched delegation-record subscriptions on parse failure
Line 309 aborts on parse error, but the cancellation set is built from
record_subs, which is populated incrementally in the loop. If failure happens early, some already-fetched delegation-record subscriptions are missed and remain active.💡 Proposed fix
@@ - let mut record_subs = Vec::with_capacity(accounts_fully_resolved.len()); + let all_record_subs: HashSet<Pubkey> = accounts_fully_resolved + .iter() + .map(|a| a.companion_pubkey) + .collect(); + let mut record_subs = Vec::with_capacity(all_record_subs.len()); @@ - new_subs: pubkeys + new_subs: pubkeys .iter() .cloned() - .chain(record_subs.iter().cloned()) + .chain(all_record_subs.iter().cloned()) .collect(),Also applies to: 286-293, 309-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs` around lines 279 - 280, The loop builds record_subs incrementally but on parse failure (the early abort around the parse error) subscriptions already pushed to record_subs remain active; modify the error path so that before returning/aborting you iterate over the existing record_subs and cancel/unsubscribe each subscription (the vector named record_subs), ensuring any active delegation-record subscriptions are closed for accounts already processed (accounts_to_clone / the loop that populates record_subs). In short: on parse error, drain record_subs and cancel each subscription (or move subscription registration into a scope that guarantees cleanup) so no partial subscriptions are left running.magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (1)
122-123:⚠️ Potential issue | 🟠 MajorPropagate parsed delegation actions into ATA clone requests
Line 130 parses
delegation_actionsbut discards them, and Line 151 always sendsvec![]. This drops post-delegation actions for the ATA-projection path.💡 Proposed fix
@@ - let mut delegated_to_other = None; + let mut delegated_to_other = None; + let mut delegation_actions = vec![]; @@ - let (deleg_record, _delegation_actions) = deleg; + let (deleg_record, maybe_delegation_actions) = deleg; + delegation_actions = maybe_delegation_actions.unwrap_or_default(); delegated_to_other = delegation::get_delegated_to_other(this, &deleg_record); commit_frequency_ms = Some(deleg_record.commit_frequency_ms); @@ accounts_to_clone.push(AccountCloneRequest { pubkey: ata_pubkey, account: account_to_clone, commit_frequency_ms, - delegation_actions: vec![], + delegation_actions, delegated_to_other, });Also applies to: 130-133, 147-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs` around lines 122 - 123, The parsed delegation actions returned by delegation::fetch_and_parse_delegation_record (stored as delegation_actions) are currently discarded and an empty vec![] is sent in the ATA clone request; change the code that builds the ATA clone request to forward delegation_actions instead of vec![] so post-delegation actions are preserved. Locate where delegation::fetch_and_parse_delegation_record is called and the subsequent ATA clone request construction (the spot currently passing vec![]) and replace that empty vector with the parsed delegation_actions variable so the cloned-ATA path executes the delegated actions.
♻️ Duplicate comments (2)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
451-451: 🧹 Nitpick | 🔵 TrivialUse
unwrap_or_default()for delegation-actions fallbackLine 451 should use
unwrap_or_default().🤖 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` at line 451, Replace the explicit vec![] fallback on delegation_actions with the standard unwrap_or_default() to avoid allocating an empty Vec manually; locate the call site where delegation_actions.unwrap_or(vec![]) is used (in the construct that passes delegation_actions into the action list) and change it to delegation_actions.unwrap_or_default() so the Option<Vec<_>> uses its Default implementation.magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs (1)
345-345: 🧹 Nitpick | 🔵 TrivialUse
unwrap_or_default()for the empty-actions fallbackLine 345 can use the idiomatic default form.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs` at line 345, Replace the manual empty vector fallback on delegation_actions with the idiomatic default helper: update the call site using delegation_actions.unwrap_or(vec![]) to use delegation_actions.unwrap_or_default() so it returns an empty Vec when None; locate the usage in the pipeline code where delegation_actions is passed (the expression currently written as delegation_actions.unwrap_or(vec![])) and change it to use unwrap_or_default on that Option<Vec<...>>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/delegation.rs`:
- Around line 25-27: Update the parser doc comment (the one starting "Parses a
delegation record from account data bytes.") to document that the function can
return not only InvalidDelegationRecord but also
ChainlinkError::InvalidDelegationActions; explicitly mention both error variants
and the conditions under which InvalidDelegationActions is produced so readers
know the parser may fail for invalid delegation actions as well as malformed
records.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 497-500: The ATA subscription-update path is discarding parsed
post-delegation actions by returning an empty vec; update the code where
maybe_project_ata_from_subscription_update(pubkey, account).await is handled so
you return the parsed actions from that call instead of vec![] (i.e., propagate
the actions tuple element produced by maybe_project_ata_from_subscription_update
into the returned value), and apply the same fix to the other similar branches
(the sections around the other occurrences that currently return vec![] for
projected ATA updates) so projected ATA updates can carry post-delegation
actions.
---
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs`:
- Around line 122-123: The parsed delegation actions returned by
delegation::fetch_and_parse_delegation_record (stored as delegation_actions) are
currently discarded and an empty vec![] is sent in the ATA clone request; change
the code that builds the ATA clone request to forward delegation_actions instead
of vec![] so post-delegation actions are preserved. Locate where
delegation::fetch_and_parse_delegation_record is called and the subsequent ATA
clone request construction (the spot currently passing vec![]) and replace that
empty vector with the parsed delegation_actions variable so the cloned-ATA path
executes the delegated actions.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs`:
- Around line 279-280: The loop builds record_subs incrementally but on parse
failure (the early abort around the parse error) subscriptions already pushed to
record_subs remain active; modify the error path so that before
returning/aborting you iterate over the existing record_subs and
cancel/unsubscribe each subscription (the vector named record_subs), ensuring
any active delegation-record subscriptions are closed for accounts already
processed (accounts_to_clone / the loop that populates record_subs). In short:
on parse error, drain record_subs and cancel each subscription (or move
subscription registration into a scope that guarantees cleanup) so no partial
subscriptions are left running.
---
Duplicate comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Line 451: Replace the explicit vec![] fallback on delegation_actions with the
standard unwrap_or_default() to avoid allocating an empty Vec manually; locate
the call site where delegation_actions.unwrap_or(vec![]) is used (in the
construct that passes delegation_actions into the action list) and change it to
delegation_actions.unwrap_or_default() so the Option<Vec<_>> uses its Default
implementation.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs`:
- Line 345: Replace the manual empty vector fallback on delegation_actions with
the idiomatic default helper: update the call site using
delegation_actions.unwrap_or(vec![]) to use
delegation_actions.unwrap_or_default() so it returns an empty Vec when None;
locate the usage in the pipeline code where delegation_actions is passed (the
expression currently written as delegation_actions.unwrap_or(vec![])) and change
it to use unwrap_or_default on that Option<Vec<...>>.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rs
Comment on lines
+25
to
+27
| /// Parses a delegation record from account data bytes. | ||
| /// Returns the parsed DelegationRecord, or InvalidDelegationRecord error | ||
| /// if parsing fails. |
Contributor
There was a problem hiding this comment.
Update the parser doc comment to include InvalidDelegationActions
The comment currently documents only InvalidDelegationRecord, but this function can now also fail with ChainlinkError::InvalidDelegationActions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/delegation.rs` around lines
25 - 27, Update the parser doc comment (the one starting "Parses a delegation
record from account data bytes.") to document that the function can return not
only InvalidDelegationRecord but also ChainlinkError::InvalidDelegationActions;
explicitly mention both error variants and the conditions under which
InvalidDelegationActions is produced so readers know the parser may fail for
invalid delegation actions as well as malformed records.
…ord return actions
snawaz
force-pushed
the
snawaz/post-delegation-actions
branch
from
74a0319 to
9e2b9e9
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs (1)
122-133:⚠️ Potential issue | 🟠 MajorDelegation actions are dropped in ATA projection flow.
At Line 130 actions are ignored, and Line 151 always sends default actions. This loses post-delegation actions for ATA clones resolved via eATA projection.
Proposed fix
let mut account_to_clone = ata_account.account_shared_data_cloned(); let mut commit_frequency_ms = None; let mut delegated_to_other = None; + let mut delegation_actions = DelegationActions::default(); @@ - if let Some(deleg) = delegation::fetch_and_parse_delegation_record( + if let Some((deleg_record, actions)) = delegation::fetch_and_parse_delegation_record( this, eata_pubkey, this.remote_account_provider.chain_slot(), fetch_origin, ) .await { - let (deleg_record, _delegation_actions) = deleg; + delegation_actions = actions.unwrap_or_default(); delegated_to_other = delegation::get_delegated_to_other(this, &deleg_record); commit_frequency_ms = Some(deleg_record.commit_frequency_ms); @@ accounts_to_clone.push(AccountCloneRequest { pubkey: ata_pubkey, account: account_to_clone, commit_frequency_ms, - delegation_actions: DelegationActions::default(), + delegation_actions, delegated_to_other, });Also applies to: 147-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs` around lines 122 - 133, The code drops delegation actions returned from delegation::fetch_and_parse_delegation_record (currently bound to _delegation_actions) and later always sends default actions; keep and thread the actual delegation_actions through the ATA projection flow: remove the underscore so you bind let (deleg_record, delegation_actions) = ..., store delegation_actions in a variable in scope (e.g., let mut resolved_delegation_actions = delegation_actions), and when building/sending the ATA clone actions (the place that currently uses default actions around commit/send) use resolved_delegation_actions if present instead of the hard-coded default; ensure the variable is propagated to where commit_frequency_ms and delegated_to_other are used.
♻️ Duplicate comments (2)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (1)
503-510:⚠️ Potential issue | 🟠 MajorATA subscription-update path still drops delegation actions.
Line 643 discards parsed actions and Line 509 returns defaults, so ATA updates resolved through this path lose post-delegation actions.
Proposed fix
- let (account, deleg_record) = self + let (account, deleg_record, delegation_actions) = self .maybe_project_ata_from_subscription_update(pubkey, account) .await; ( Some(account), deleg_record, - DelegationActions::default(), + delegation_actions, ) @@ async fn maybe_project_ata_from_subscription_update( &self, ata_pubkey: Pubkey, ata_account: AccountSharedData, - ) -> (AccountSharedData, Option<DelegationRecord>) { + ) -> (AccountSharedData, Option<DelegationRecord>, DelegationActions) { let Some(ata_info) = is_ata(&ata_pubkey, &ata_account) else { - return (ata_account, None); + return (ata_account, None, DelegationActions::default()); }; @@ let Some((eata_pubkey, _)) = try_derive_eata_address_and_bump(&ata_info.owner, &ata_info.mint) else { - return (ata_account, None); + return (ata_account, None, DelegationActions::default()); }; @@ let Some(eata_account) = eata_account else { - return (ata_account, None); + return (ata_account, None, DelegationActions::default()); }; @@ - let Some(deleg_record) = deleg_record else { - return (ata_account, None); + let Some((deleg_record, delegation_actions)) = deleg_record else { + return (ata_account, None, DelegationActions::default()); }; - let (deleg_record, _delegation_actions) = deleg_record; + let delegation_actions = delegation_actions.unwrap_or_default(); @@ if let Some(projected_ata) = self.maybe_project_delegated_ata_from_eata( &ata_account, &eata_account, &deleg_record, ) { - return (projected_ata, Some(deleg_record)); + return (projected_ata, Some(deleg_record), delegation_actions); } - (ata_account, Some(deleg_record)) + (ata_account, Some(deleg_record), delegation_actions) }Also applies to: 632-644
🤖 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 503 - 510, The ATA subscription-update path is discarding parsed DelegationActions by returning DelegationActions::default() after calling maybe_project_ata_from_subscription_update; change maybe_project_ata_from_subscription_update to return the parsed DelegationActions (e.g., (Option<Account>, DelegationRecord, DelegationActions)), update this call site to destructure and propagate that third value instead of DelegationActions::default(), and update any other callers of maybe_project_ata_from_subscription_update to accept the new triple return so post-delegation actions are preserved.magicblock-chainlink/src/chainlink/fetch_cloner/delegation.rs (1)
23-25:⚠️ Potential issue | 🟡 MinorUpdate parser docs to include
InvalidDelegationActions.Line 23-Line 25 still describe only
InvalidDelegationRecord, but this parser now also fails on invalid trailing action bytes viaChainlinkError::InvalidDelegationActions.Suggested doc update
-/// Parses a delegation record from account data bytes. -/// Returns the parsed DelegationRecord, or InvalidDelegationRecord error -/// if parsing fails. +/// Parses a delegation record from account data bytes. +/// Returns the parsed `DelegationRecord` plus optional `DelegationActions`. +/// Fails with `ChainlinkError::InvalidDelegationRecord` when the base record is malformed. +/// Fails with `ChainlinkError::InvalidDelegationActions` when trailing action bytes can't be decoded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-chainlink/src/chainlink/fetch_cloner/delegation.rs` around lines 23 - 25, Update the parser doc comment above the DelegationRecord parsing function to mention that it can return both ChainlinkError::InvalidDelegationRecord and ChainlinkError::InvalidDelegationActions when trailing action bytes are invalid; locate the comment block in delegation.rs associated with the DelegationRecord parser (the doc lines starting "Parses a delegation record from account data bytes.") and add a short sentence listing InvalidDelegationActions as an additional failure mode referencing ChainlinkError::InvalidDelegationActions.
🤖 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 `@magicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rs`:
- Around line 122-133: The code drops delegation actions returned from
delegation::fetch_and_parse_delegation_record (currently bound to
_delegation_actions) and later always sends default actions; keep and thread the
actual delegation_actions through the ATA projection flow: remove the underscore
so you bind let (deleg_record, delegation_actions) = ..., store
delegation_actions in a variable in scope (e.g., let mut
resolved_delegation_actions = delegation_actions), and when building/sending the
ATA clone actions (the place that currently uses default actions around
commit/send) use resolved_delegation_actions if present instead of the
hard-coded default; ensure the variable is propagated to where
commit_frequency_ms and delegated_to_other are used.
---
Duplicate comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/delegation.rs`:
- Around line 23-25: Update the parser doc comment above the DelegationRecord
parsing function to mention that it can return both
ChainlinkError::InvalidDelegationRecord and
ChainlinkError::InvalidDelegationActions when trailing action bytes are invalid;
locate the comment block in delegation.rs associated with the DelegationRecord
parser (the doc lines starting "Parses a delegation record from account data
bytes.") and add a short sentence listing InvalidDelegationActions as an
additional failure mode referencing ChainlinkError::InvalidDelegationActions.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 503-510: The ATA subscription-update path is discarding parsed
DelegationActions by returning DelegationActions::default() after calling
maybe_project_ata_from_subscription_update; change
maybe_project_ata_from_subscription_update to return the parsed
DelegationActions (e.g., (Option<Account>, DelegationRecord,
DelegationActions)), update this call site to destructure and propagate that
third value instead of DelegationActions::default(), and update any other
callers of maybe_project_ata_from_subscription_update to accept the new triple
return so post-delegation actions are preserved.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
magicblock-accounts/src/scheduled_commits_processor.rsmagicblock-chainlink/src/chainlink/fetch_cloner/ata_projection.rsmagicblock-chainlink/src/chainlink/fetch_cloner/delegation.rsmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/pipeline.rsmagicblock-chainlink/src/cloner/mod.rstest-integration/test-chainlink/src/ixtest_context.rs
snawaz
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR does the following: