feat(commit): add magic fee vault support with commit-based fee charging#1042
Merged
feat(commit): add magic fee vault support with commit-based fee charging#1042
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. |
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds delegated magic fee-vault support end-to-end. magicblock-api: startup ensures vault exists and delegated ownership, plus two new ApiError variants. Programs: new fee constants and per-account fee calculation APIs, vault PDA lookup/validation, delegated-payer charging, and fee-aware commit/intent processing paths. Utilities: wallet-to-vault charge helper and test-scoped instruction builders. Test harnesses and CPI APIs extended to accept an optional magic_fee_vault PDA and support per-account nonces. Cargo files updated to new revisions for delegation program and related SDKs. Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-integration/programs/schedulecommit-security/src/lib.rs (1)
83-87: 🧹 Nitpick | 🔵 TrivialExpose the vault option instead of hardcoding the legacy layout.
has_magic_vault/magic_fee_vaultdefine how the downstream instruction parses its accounts. Hardcodingfalse/Nonein every path means this harness cannot model charged-commit layouts; if a test includes a fee-vault account, it will be interpreted as the first committee PDA instead. Thread the optional vault throughScheduleCommitSecurityInstructionif these security scenarios are meant to cover the new fee-vault flow.Also applies to: 122-128, 147-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-integration/programs/schedulecommit-security/src/lib.rs` around lines 83 - 87, The harness is hardcoding legacy vault layout by always setting ProcessSchedulecommitCpiArgs.has_magic_vault = false and magic_fee_vault = None, so any test that supplies a fee-vault will be mis-parsed; update the ScheduleCommitSecurityInstruction payload to carry an optional vault flag/account, propagate that option into the code paths that construct ProcessSchedulecommitCpiArgs (instead of the literal false/None at the sites shown), and set has_magic_vault and magic_fee_vault based on the passed-through optional vault so the CPI instruction reflects the charged-commit layout when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/magicblock/src/magic_scheduled_base_intent.rs`:
- Around line 929-939: Replace the unsafe arithmetic in the
accounts.iter().try_fold closure: change the paid-commit check from "*nonce + 1
> ACTUAL_COMMIT_LIMIT" to "*nonce >= ACTUAL_COMMIT_LIMIT" (use the existing
commit_nonces lookup and ACTUAL_COMMIT_LIMIT constant) to avoid adding to a
possibly-maxed u64, and replace the unchecked fee accumulation "fee +
COMMIT_FEE_LAMPORTS" with a checked addition (e.g.,
fee.checked_add(COMMIT_FEE_LAMPORTS).ok_or(InstructionError::Custom(YOUR_OVERFLOW_ERR)))
so the closure returns an InstructionError on overflow instead of
wrapping/panicking; keep the existing MISSING_COMMIT_NONCE_ERR path for missing
nonces.
In
`@programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs`:
- Around line 1402-1446: The test seeds committee_at with ACTUAL_COMMIT_LIMIT
but calculate_commit_fee treats nonce >= ACTUAL_COMMIT_LIMIT as charged, so the
test will charge two fees; change the per_account entry for committee_at to use
ACTUAL_COMMIT_LIMIT - 1 (i.e., one less than the limit) so calculate_commit_fee
will treat committee_at as free. Update the per_account insert for committee_at
in the prepare_delegated_payer_transaction setup (referencing
ACTUAL_COMMIT_LIMIT, committee_at, committee_above,
prepare_delegated_payer_transaction, and process_instruction) accordingly.
- Around line 1289-1339: The test calls magic_fee_vault_pubkey() (which hits
validator_authority_id()) before the Validator is initialized, causing panics
when tests run in isolation; move the ensure_started_validator(&mut
account_data, Some(nonces)) call to happen before deriving the fee_vault_pubkey
(i.e., call ensure_started_validator early in
prepare_delegated_payer_transaction) so that
validator_authority_id()/magic_fee_vault_pubkey() run after the validator is
started and then compute fee_vault_pubkey safely.
In `@test-integration/programs/schedulecommit-security/src/lib.rs`:
- Around line 141-156: The invocation is missing the program account required by
the instruction: when create_schedule_commit_ix sets the instruction's
program_id to *magic_program.key, magic_program must be included in the accounts
slice passed to invoke; fix by inserting magic_program (the AccountInfo
referenced as magic_program) into the account_infos vector before calling invoke
(alongside payer, magic_context and pda_infos) so that the invoke call receives
the program account expected by create_schedule_commit_ix.
In `@test-integration/programs/schedulecommit/src/lib.rs`:
- Around line 686-691: Remove the unused construction of the account_infos
vector: the local variable account_infos (and its pushes of payer,
magic_context, optional magic_fee_vault, and extend from remaining) is never
read or returned, so delete the block that creates and populates account_infos
to eliminate dead code; ensure no other code expects that vector (search for
account_infos) and if a list of AccountInfo references is actually needed by a
call, replace the dead block with the correct call-site usage instead of just
removing logic.
- Around line 81-88: The struct ScheduleCommitCpiWithVaultArgs has a naming
mismatch: it defines pub has_vault while ScheduleCommitCpiArgs uses pub
has_magic_vault; make them consistent by renaming the field in
ScheduleCommitCpiWithVaultArgs to has_magic_vault (or rename the other to
has_vault) and update all references/usages, serialization/deserialization
(Borsh) expectations, and any tests or CPI calls that construct or pattern-match
on ScheduleCommitCpiWithVaultArgs to use the chosen name; ensure both structs
share the exact same field name and type (bool) to avoid runtime or
(de)serialization issues.
In `@test-integration/schedulecommit/test-scenarios/tests/03_commit_limit.rs`:
- Around line 215-218: Replace the local COMMIT_FEE_LAMPORTS constant in this
test with a direct import of COMMIT_FEE_LAMPORTS from the magicblock-program
crate (so the test uses the exported value), and leave ACTUAL_COMMIT_LIMIT
hardcoded because ACTUAL_COMMIT_LIMIT is declared pub(crate) in
magic_scheduled_base_intent.rs and cannot be imported; update the surrounding
comment to note that only COMMIT_FEE_LAMPORTS can be imported while
ACTUAL_COMMIT_LIMIT remains test-local.
In `@test-integration/schedulecommit/test-security/Cargo.toml`:
- Around line 18-19: The Cargo manifest currently lists the log crate under
[dependencies]; if this crate is only used by tests in this test-only package,
move the entry "log = \"0.4.29\"" from the [dependencies] section into a
[dev-dependencies] section so it is treated as a test-only dependency; update
Cargo.toml by removing the line from [dependencies] and adding it under
[dev-dependencies] (keeping the exact version string) to reflect that log is
used only for tests.
---
Outside diff comments:
In `@test-integration/programs/schedulecommit-security/src/lib.rs`:
- Around line 83-87: The harness is hardcoding legacy vault layout by always
setting ProcessSchedulecommitCpiArgs.has_magic_vault = false and magic_fee_vault
= None, so any test that supplies a fee-vault will be mis-parsed; update the
ScheduleCommitSecurityInstruction payload to carry an optional vault
flag/account, propagate that option into the code paths that construct
ProcessSchedulecommitCpiArgs (instead of the literal false/None at the sites
shown), and set has_magic_vault and magic_fee_vault based on the passed-through
optional vault so the CPI instruction reflects the charged-commit layout when
present.
🪄 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: 16b3d8aa-4012-4533-bb7d-9b9548996dd1
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (26)
Cargo.tomlmagicblock-api/Cargo.tomlmagicblock-api/src/errors.rsmagicblock-api/src/magic_validator.rsmagicblock-committor-service/src/tasks/mod.rsprograms/magicblock/src/magic_scheduled_base_intent.rsprograms/magicblock/src/schedule_transactions/mod.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_tests.rsprograms/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rsprograms/magicblock/src/test_utils/mod.rsprograms/magicblock/src/utils/instruction_utils.rsprograms/magicblock/src/utils/mod.rstest-integration/Cargo.tomltest-integration/programs/flexi-counter/src/processor.rstest-integration/programs/flexi-counter/src/processor/schedule_intent.rstest-integration/programs/schedulecommit-security/src/lib.rstest-integration/programs/schedulecommit/src/api.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/schedulecommit/client/src/schedule_commit_context.rstest-integration/schedulecommit/test-scenarios/Cargo.tomltest-integration/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/schedulecommit/test-scenarios/tests/03_commit_limit.rstest-integration/schedulecommit/test-security/Cargo.tomltest-integration/schedulecommit/test-security/tests/01_invocations.rs
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
Show resolved
Hide resolved
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
Outdated
Show resolved
Hide resolved
# Conflicts: # programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs (1)
1403-1436:⚠️ Potential issue | 🟠 MajorReverse the order: call
ensure_started_validator()beforemagic_fee_vault_pubkey().The
magic_fee_vault_pubkey()function callsvalidator_authority_id(), which panics with "Validator authority needs to be set on startup" if the validator has not been initialized. Currently at line 1403,magic_fee_vault_pubkey()is called beforeensure_started_validator()at line 1436, causing tests to panic when run in isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs` around lines 1403 - 1436, Call ensure_started_validator(...) before calling magic_fee_vault_pubkey() to ensure validator_authority_id() is initialized; currently magic_fee_vault_pubkey() (which calls validator_authority_id()) runs first and panics when the validator hasn’t been started. Move the ensure_started_validator(&mut account_data, Some(nonces)) call to precede the call to magic_fee_vault_pubkey(), keeping the same arguments and using the existing ensure_started_validator and magic_fee_vault_pubkey symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs`:
- Around line 1483-1488: The assertion duplicating the check used in the find()
call is redundant: remove the assert_eq!(vault.lamports(), COMMIT_FEE_LAMPORTS);
line (or replace it with a short comment) since the .expect("fee vault should
have COMMIT_FEE_LAMPORTS") on the find already guarantees that condition; locate
the usage of vault, accounts, and COMMIT_FEE_LAMPORTS in
process_schedule_commit_tests.rs and drop the redundant assertion to simplify
the test.
- Around line 1507-1512: The inline comments for committee_above and
committee_at are incorrect: per_account currently maps committee_above ->
ACTUAL_COMMIT_LIMIT (which is at the limit and thus charged by the logic) and
committee_at -> ACTUAL_COMMIT_LIMIT - 1 (below the limit and free). Update the
comments next to the Pubkey declarations or the per_account inserts to reflect
those actual values (e.g., mark committee_above as "nonce == limit → charged"
and committee_at as "nonce < limit → free") or alternatively swap the assigned
values if you intended the original comments to be true; adjust only the
comments or the assignments for committee_above, committee_at, and the
per_account.insert calls to keep code and comments consistent.
In
`@programs/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rs`:
- Around line 163-165: The fee calculation uses a single pre-bundle nonce
snapshot (fetch_current_commit_nonces) and get_all_committed_accounts can
contain duplicate pubkeys, allowing undercharging; update calculate_fee (or the
internal calculate_commit_fee it calls) to account for repeated commits by
either deduplicating the list returned by get_all_committed_accounts before
passing to fetch_current_commit_nonces, or (preferably) by iterating the
accounts and maintaining a local mutable nonce map that increments the pubkey's
nonce on each seen occurrence and uses that advanced nonce when checking against
ACTUAL_COMMIT_LIMIT, ensuring duplicate pubkeys are charged as multiple commits;
adjust the logic in calculate_fee / calculate_commit_fee accordingly and keep
fetch_current_commit_nonces as a snapshot-only function.
---
Duplicate comments:
In
`@programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs`:
- Around line 1403-1436: Call ensure_started_validator(...) before calling
magic_fee_vault_pubkey() to ensure validator_authority_id() is initialized;
currently magic_fee_vault_pubkey() (which calls validator_authority_id()) runs
first and panics when the validator hasn’t been started. Move the
ensure_started_validator(&mut account_data, Some(nonces)) call to precede the
call to magic_fee_vault_pubkey(), keeping the same arguments and using the
existing ensure_started_validator and magic_fee_vault_pubkey symbols.
🪄 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: 53a95ce9-0c71-437a-b08f-0ce8cca5ae87
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
magicblock-api/src/magic_validator.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_tests.rsprograms/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rs
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
Outdated
Show resolved
Hide resolved
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
Outdated
Show resolved
Hide resolved
programs/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/magicblock/src/magic_scheduled_base_intent.rs`:
- Around line 36-38: The doc comment for COMMIT_FEE_LAMPORTS is inaccurate:
update the comment to reflect that the fee is charged only when
calculate_commit_fee() deems nonce >= ACTUAL_COMMIT_LIMIT (i.e., after the
sponsored commit threshold such as the 25 sponsored commits), not simply “after
the first” commit; reference COMMIT_FEE_LAMPORTS and calculate_commit_fee() and
mention the ACTUAL_COMMIT_LIMIT / 25-sponsored-commits behavior in the updated
comment.
In `@test-integration/programs/schedulecommit/src/api.rs`:
- Around line 237-265: The account meta for the order_book in
schedule_commit_with_vault_and_order_book_instruction is created as readonly but
the doc comment and ScheduleCommitWithVaultAndOrderBookCpi expect it to be
writable; change the AccountMeta for order_book from a readonly meta to a
writable one (use the writable constructor for AccountMeta for the symbol
order_book in the function
schedule_commit_with_vault_and_order_book_instruction) and ensure the doc
comment and any references in ScheduleCommitWithVaultAndOrderBookCpi remain
consistent with the writable access.
In `@test-integration/schedulecommit/test-scenarios/tests/03_commit_limit.rs`:
- Around line 516-601: The test only asserts the committee commit but never
verifies the post-commit UpdateOrderBook action; after calling
verify::fetch_and_verify_commit_result_from_logs(ctx, sig) and
assert_committee_was_committed(...), fetch the order_book_pda account from the
chain (use ctx.fetch_chain_account(order_book_pda) or an equivalent helper),
deserialize or inspect its state to confirm the expected update (e.g., expected
escrow_index/fields changed), or alternatively assert that the commit logs
returned by fetch_and_verify_commit_result_from_logs contain an
"UpdateOrderBook" entry; ensure the check references order_book_pda,
ScheduleCommitWithOrderBookArgs.with_actions, and the sig used when verifying
logs.
🪄 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: 43e0fd4b-0d87-4951-b98a-9a039a874516
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
programs/magicblock/src/magic_scheduled_base_intent.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit_tests.rstest-integration/programs/schedulecommit/src/api.rstest-integration/programs/schedulecommit/src/lib.rstest-integration/schedulecommit/test-scenarios/tests/03_commit_limit.rstest-integration/schedulecommit/test-security/Cargo.toml
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test-integration/schedulecommit/test-scenarios/tests/03_commit_limit.rs (1)
604-636: 🧹 Nitpick | 🔵 TrivialThe test does not verify the
UpdateOrderBookaction was executed.The test asserts commit success and fee deductions but never confirms the post-commit action actually updated the order book state. Consider fetching
order_book_pdaafter the commit and asserting the expected state change, or verifying anUpdateOrderBooklog entry in the transaction logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-integration/schedulecommit/test-scenarios/tests/03_commit_limit.rs` around lines 604 - 636, The test currently checks commit success and fee transfers (using verify::fetch_and_verify_commit_result_from_logs and assert_committee_was_committed) but never asserts the post-commit side-effect; fetch the order book PDA (order_book_pda) or inspect the transaction logs for an UpdateOrderBook entry after the commit and assert the expected state change (e.g., updated bids/asks or a version/timestamp field), or explicitly verify an "UpdateOrderBook" log message in the result returned by verify::fetch_and_verify_commit_result_from_logs; add this check alongside the existing fee and vault assertions to ensure the action was executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test-integration/schedulecommit/test-scenarios/tests/03_commit_limit.rs`:
- Around line 604-636: The test currently checks commit success and fee
transfers (using verify::fetch_and_verify_commit_result_from_logs and
assert_committee_was_committed) but never asserts the post-commit side-effect;
fetch the order book PDA (order_book_pda) or inspect the transaction logs for an
UpdateOrderBook entry after the commit and assert the expected state change
(e.g., updated bids/asks or a version/timestamp field), or explicitly verify an
"UpdateOrderBook" log message in the result returned by
verify::fetch_and_verify_commit_result_from_logs; add this check alongside the
existing fee and vault assertions to ensure the action was executed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 183b5cbd-c935-4f9f-86eb-6d2d5a5cc660
📒 Files selected for processing (1)
test-integration/schedulecommit/test-scenarios/tests/03_commit_limit.rs
# Conflicts: # Cargo.lock # Cargo.toml # magicblock-committor-service/src/tasks/mod.rs # test-integration/Cargo.lock # test-integration/Cargo.toml # test-integration/schedulecommit/elfs/dlp.so
taco-paco
force-pushed
the
feat/magic-fee-vault
branch
from
2360171 to
d99c719
Compare
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
2 participants
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
magic fee vaultPDA. 25 commits are sponsored in order to avoid double charging with dlp.ensure_magic_fee_vault_on_chainto validator startup (magicblock-api): checks vault and delegation record on chain, initializes and/or delegates as needed, exits process on failure03_commit_limit.rs: missing vault errors, within-limit free, over-limit charged, order-book actionCompatibility
Testing
programs/magicblock/src/magic_scheduled_base_intent.rs: unit tests forcalculate_commit_feeboundariestest-integration/schedulecommit/test-scenarios/tests/03_commit_limit.rs: integration tests covering vault absent error, within-limit free commits, over-limit fee charging and vault credit, and schedule-commit-with-order-book actionChecklist
Summary by CodeRabbit
New Features
Improvements
Tests