fix: migrate integration tests to production dlp without unit_test_config#630
Conversation
📝 WalkthroughWalkthroughThe PR introduces optional validator propagation across delegation flows: adds Assessment against linked issues
Out-of-scope changes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3
♻️ Duplicate comments (4)
test-integration/test-ledger-restore/tests/06_delegated_account.rs (1)
1-2: Deprecation warning suppression.Same deprecation suppression pattern as noted in
test-integration/test-config/tests/auto_airdrop_feepayer.rs. See the comment on that file regarding verification of deprecated APIs.test-integration/test-runner/bin/run_tests.rs (1)
1-2: Deprecation warning suppression.Same deprecation suppression pattern. See the verification request in
test-integration/test-config/tests/auto_airdrop_feepayer.rs.test-integration/test-magicblock-api/tests/test_claim_fees.rs (1)
1-2: Deprecation warning suppression.Same deprecation suppression pattern. See the verification request in
test-integration/test-config/tests/auto_airdrop_feepayer.rs.test-integration/test-task-scheduler/src/lib.rs (1)
1-2: Deprecation warning suppression.Same deprecation suppression pattern. See the verification request in
test-integration/test-config/tests/auto_airdrop_feepayer.rs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
test-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (12)
test-integration/programs/flexi-counter/src/instruction.rs(1 hunks)test-integration/programs/flexi-counter/src/processor/call_handler.rs(2 hunks)test-integration/programs/schedulecommit/src/api.rs(2 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(2 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(1 hunks)test-integration/test-config/tests/auto_airdrop_feepayer.rs(1 hunks)test-integration/test-ledger-restore/tests/06_delegated_account.rs(1 hunks)test-integration/test-magicblock-api/tests/test_claim_fees.rs(1 hunks)test-integration/test-runner/bin/run_tests.rs(1 hunks)test-integration/test-schedule-intent/tests/test_schedule_intents.rs(3 hunks)test-integration/test-task-scheduler/src/lib.rs(1 hunks)test-integration/test-tools/src/loaded_accounts.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/programs/flexi-counter/src/instruction.rstest-integration/programs/flexi-counter/src/processor/call_handler.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-task-scheduler/src/lib.rstest-integration/test-config/tests/auto_airdrop_feepayer.rstest-integration/test-schedule-intent/tests/test_schedule_intents.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
test-integration/test-ledger-restore/tests/06_delegated_account.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-config/tests/auto_airdrop_feepayer.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
test-integration/test-task-scheduler/src/lib.rstest-integration/test-runner/bin/run_tests.rstest-integration/test-magicblock-api/tests/test_claim_fees.rs
🧬 Code graph analysis (2)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
test-integration/programs/schedulecommit/src/api.rs (2)
init_payer_escrow_with_validator(39-63)pda_and_bump(263-267)
test-integration/test-schedule-intent/tests/test_schedule_intents.rs (1)
test-integration/programs/flexi-counter/src/instruction.rs (1)
create_delegate_ix_with_validator(303-308)
🔇 Additional comments (7)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (2)
233-235: Early return on InvalidWritableAccount in simulation is appropriate.The early return when detecting
InvalidWritableAccountin the simulation result correctly handles the transient undelegation state. However, note that string matching on error messages can be fragile if error message formats change.
247-252: LGTM! Proper error handling for undelegated accounts.The structured error matching using
matches!correctly identifies theInvalidWritableAccounterror and returns early, preventing false test failures when accounts are in a transient undelegated state. This is an improvement over the string-based check in the simulation path above.test-integration/programs/flexi-counter/src/processor/call_handler.rs (1)
100-103: LGTM! Validator parameter properly threaded for future use.The addition of the
validatorparameter toprocess_redelegation_call_handlerand its propagation toDelegateConfigaligns with the PR's goal of supporting custom validator identities. While this function is currently dead code due to Solana's reentrancy restrictions, the change prepares the infrastructure for validator-aware delegation when reentrancy becomes possible.Also applies to: 130-133
test-integration/programs/flexi-counter/src/instruction.rs (1)
303-308: Transitional API: validator parameter currently unused.This new function accepts a
validatorparameter but doesn't utilize it—it simply delegates to the existingcreate_delegate_ix. This is a reasonable transitional step to establish the API signature while the underlying implementation is being updated. However, consider:
- Documenting when and how the
validatorparameter will be used- Adding a TODO or tracking issue reference for the full implementation
- Considering whether to mark this as
#[deprecated]once the parameter is actually utilized in a new versionIs there a tracking issue or timeline for when the
validatorparameter will be actively used rather than ignored?test-integration/test-config/tests/auto_airdrop_feepayer.rs (1)
1-2: The deprecated API suppression is justified and documented.The
#![allow(deprecated)]suppresses warnings fromLoadedAccounts::with_delegation_program_test_authority()(line 49), which is marked deprecated in test-integration/test-tools/src/loaded_accounts.rs. The deprecation is well-documented:
- Migration path: Use
new_with_new_validator_authority()instead- Reason: DLP now uses production build without unit_test_config (issue-602)
The current code uses the deprecated method intentionally for backward compatibility with existing test infrastructure. Consider updating to the new API when convenient, though the current approach is acceptable for test code during a transition period.
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
8-9: Validator-aware escrow wiring looks correctSwitching to
init_payer_escrow_with_validatorand derivingvalidator_identityfromcommon_ctx.ephem_validator_identity.as_ref().copied()cleanly threads the optional validator without changing behavior when it’sNone. No issues found with the new escrow flow.Also applies to: 198-207
test-integration/programs/schedulecommit/src/api.rs (1)
35-37: New escrow helper cleanly propagates optional validator
init_payer_escrow_with_validatorcentralizes the escrow setup whileinit_payer_escrowremains a backward-compatible wrapper passingNone. Wiring thevalidatorparameter straight intoDelegateArgsis consistent with the DLP API and matches the intent to move off hardcoded test authorities.Also applies to: 39-42, 52-58
test-integration/test-schedule-intent/tests/test_schedule_intents.rs
Outdated
Show resolved
Hide resolved
test-integration/test-schedule-intent/tests/test_schedule_intents.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test-integration/test-schedule-intent/tests/test_schedule_intents.rs (1)
350-351: Consistent validator-aware delegation usage.The update to
create_delegate_ix_with_validatorpassingctx.ephem_validator_identityis consistent with the new validator-aware delegation flow and matches the pattern established insetup_payer. This keeps all delegation call sites aligned with the PR objective.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test-integration/test-schedule-intent/tests/test_schedule_intents.rs(3 hunks)test-integration/test-tools/src/loaded_accounts.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-schedule-intent/tests/test_schedule_intents.rs
🧬 Code graph analysis (1)
test-integration/test-schedule-intent/tests/test_schedule_intents.rs (1)
test-integration/programs/flexi-counter/src/instruction.rs (4)
create_add_ix(226-238)create_delegate_ix_with_validator(303-308)create_init_ix(189-202)create_intent_ix(381-422)
🔇 Additional comments (2)
test-integration/test-schedule-intent/tests/test_schedule_intents.rs (2)
7-8: LGTM: Import updated for validator-aware delegation.The import change from
create_delegate_ixtocreate_delegate_ix_with_validatorcorrectly reflects the new validator-aware delegation API, aligning with the PR objective to pass explicit validator identities instead of relying on hardcoded test values.
291-311: Excellent fix: Index consistency issue resolved and validator identity properly propagated.The previous critical issue regarding index mismatch has been addressed. The code now consistently uses index
1for:
top_up_ephemeral_balance(line 296)delegate_ephemeral_balance(line 302)- PDA assertion (line 314)
The explicit validator identity propagation via
ctx.ephem_validator_identity(line 306) correctly implements the PR objective to move away from hardcoded test validator authorities. The two-step delegation (top-up followed by delegate) is sent atomically in a single transaction (line 310), preventing any intermediate failure states.
taco-paco
left a comment
taco-paco
left a comment
There was a problem hiding this comment.
Thank you for contribution! That looks good, but prior to merging we need to make sure that all tests are passing
AvhiMaz
force-pushed
the
fix/issue-602-production-dlp-validator-authority
branch
from
80532bd to
c121a47
Compare
fixed all the conflicts and ran the unit and integration tests locally. let’s see if it passes this time, hoping all tests pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test-integration/test-magicblock-api/tests/test_claim_fees.rs`:
- Around line 25-38: The three helpers validator_keypair(), validator_pubkey()
and validator_fees_vault() each call
LoadedAccounts::new_with_new_validator_authority(), producing different random
validator identities; make them use the same LoadedAccounts instance
instead—create one LoadedAccounts (e.g., in the test setup or as a local
variable in test_claim_fees_transaction) and either pass &LoadedAccounts into
these helper functions or replace the helpers with inline accesses that read
validator_authority_keypair(), validator_authority(), and validator_fees_vault()
from that single instance so the signing keypair and fees vault refer to the
same validator.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-integration/test-magicblock-api/tests/test_claim_fees.rs
🧰 Additional context used
📓 Path-based instructions (1)
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/test-magicblock-api/tests/test_claim_fees.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/test-magicblock-api/tests/test_claim_fees.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue `#579`: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
test-integration/test-magicblock-api/tests/test_claim_fees.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
test-integration/test-magicblock-api/tests/test_claim_fees.rs
🧬 Code graph analysis (1)
test-integration/test-magicblock-api/tests/test_claim_fees.rs (1)
test-integration/test-tools/src/loaded_accounts.rs (2)
new_with_new_validator_authority(42-50)validator_fees_vault(90-94)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
GabrielePicco
force-pushed
the
fix/issue-602-production-dlp-validator-authority
branch
from
42e597f to
c121a47
Compare
|
Hey @AvhiMaz, thanks a lot for the contribution, and sorry for the slow turnaround on this one. I’ve disabled the workflow that was failing due to not being a repo member and merged master. Let us know if you’re still interested in fixing the remaining integration tests to bring the PR to a mergeable state. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test-integration/schedulecommit/client/src/schedule_commit_context.rs`:
- Around line 243-249: The escrow_lamports_for_payer function currently calls
init_payer_escrow_with_validator passing
self.common_ctx.ephem_validator_identity which may be None; add an explicit
guard in escrow_lamports_for_payer to check that
self.common_ctx.ephem_validator_identity.is_some() (or match on it) and return
an Err with a clear message if missing, otherwise unwrap/copy the
validator_identity and call init_payer_escrow_with_validator as before
(reference symbols: escrow_lamports_for_payer,
common_ctx.ephem_validator_identity, init_payer_escrow_with_validator,
payer_ephem.pubkey()).
In
`@test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs`:
- Around line 433-436: Move the inline `use
solana_sdk::transaction::TransactionError;` out of the conditional and into the
surrounding scope (either the enclosing function or top of the file) so it's not
declared inside the if block; update the scope where `tx_err` is matched (the
`if matches!(tx_err, TransactionError::InvalidWritableAccount) { return; }`
check) to rely on the now-top-level import of `TransactionError` to improve
readability and conventional organization.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/deploy-testnet-by-pr-comment.yml.github/workflows/slack-notify.ymltest-integration/programs/flexi-counter/src/instruction.rstest-integration/programs/flexi-counter/src/processor/call_handler.rstest-integration/programs/schedulecommit/src/api.rstest-integration/schedulecommit/client/src/schedule_commit_context.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-config/tests/auto_airdrop_feepayer.rstest-integration/test-ledger-restore/tests/06_delegated_account.rstest-integration/test-magicblock-api/tests/test_claim_fees.rstest-integration/test-runner/bin/run_tests.rstest-integration/test-schedule-intent/tests/test_schedule_intents.rstest-integration/test-task-scheduler/src/lib.rstest-integration/test-tools/src/loaded_accounts.rs
🧰 Additional context used
📓 Path-based instructions (1)
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/test-magicblock-api/tests/test_claim_fees.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-runner/bin/run_tests.rstest-integration/test-schedule-intent/tests/test_schedule_intents.rstest-integration/test-task-scheduler/src/lib.rstest-integration/programs/flexi-counter/src/processor/call_handler.rstest-integration/programs/schedulecommit/src/api.rstest-integration/schedulecommit/client/src/schedule_commit_context.rstest-integration/test-tools/src/loaded_accounts.rstest-integration/programs/flexi-counter/src/instruction.rstest-integration/test-config/tests/auto_airdrop_feepayer.rstest-integration/test-ledger-restore/tests/06_delegated_account.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-magicblock-api/tests/test_claim_fees.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-schedule-intent/tests/test_schedule_intents.rstest-integration/programs/flexi-counter/src/processor/call_handler.rstest-integration/programs/flexi-counter/src/instruction.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
test-integration/test-magicblock-api/tests/test_claim_fees.rstest-integration/programs/flexi-counter/src/processor/call_handler.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
test-integration/test-magicblock-api/tests/test_claim_fees.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-schedule-intent/tests/test_schedule_intents.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/test-magicblock-api/tests/test_claim_fees.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/test-schedule-intent/tests/test_schedule_intents.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue `#579`: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
test-integration/test-magicblock-api/tests/test_claim_fees.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
test-integration/test-runner/bin/run_tests.rstest-integration/test-task-scheduler/src/lib.rs
📚 Learning: 2025-11-19T11:31:24.218Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs:85-114
Timestamp: 2025-11-19T11:31:24.218Z
Learning: In the magicblock-validator codebase, compressed delegation records are never subscribed to by design. Tests for compressed delegation flows should focus on verifying that the delegated account (e.g., counter PDA) is cloned as delegated and not subscribed, but do not need to check for subscription to delegation-record-like PDAs in the compressed path because subscriptions to compressed delegation records never occur.
Applied to files:
test-integration/test-schedule-intent/tests/test_schedule_intents.rs
📚 Learning: 2025-11-19T12:55:48.931Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-tools/src/validator.rs:193-197
Timestamp: 2025-11-19T12:55:48.931Z
Learning: In the magicblock-validator codebase, when constructing arguments for the light CLI test validator in start_light_validator_with_config (test-integration/test-tools/src/validator.rs), the shlex::split approach is necessary because the light CLI does not handle whitespaces properly in validator arguments. The string concatenation + shlex parsing pattern should not be refactored to direct argument construction in this specific case.
Applied to files:
test-integration/test-schedule-intent/tests/test_schedule_intents.rs
🧬 Code graph analysis (2)
test-integration/test-schedule-intent/tests/test_schedule_intents.rs (1)
test-integration/programs/flexi-counter/src/instruction.rs (1)
create_delegate_ix_with_validator(310-315)
test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
test-integration/programs/schedulecommit/src/api.rs (2)
init_order_book_instruction(51-69)init_payer_escrow_with_validator(96-120)magicblock-committor-service/src/tasks/task_strategist.rs (1)
ixs(313-317)
🔇 Additional comments (11)
test-integration/test-config/tests/auto_airdrop_feepayer.rs (1)
1-2: No issues identified.The
#![allow(deprecated)]attribute appropriately silences warnings for the use ofwith_delegation_program_test_authority()on line 50, which is marked deprecated as part of this PR's migration effort.test-integration/test-magicblock-api/tests/test_claim_fees.rs (1)
25-41: Previous review concern no longer applies.The past review flagged an issue with
new_with_new_validator_authority()generating different random identities per call. The current code useswith_delegation_program_test_authority()instead, which returns the same deterministic hardcoded test values on every call—so the validator keypair, pubkey, and fees vault will all be consistent.test-integration/test-runner/bin/run_tests.rs (1)
1-2: Appropriate suppression for deprecated usages.The file contains numerous calls to
with_delegation_program_test_authority()across various test suite setups. The crate-level suppression is the right approach rather than annotating each call site.test-integration/test-task-scheduler/src/lib.rs (1)
1-2: No issues identified.The deprecated attribute appropriately covers the usage on line 62.
test-integration/test-ledger-restore/tests/06_delegated_account.rs (1)
1-2: No issues identified.The deprecated attribute appropriately covers usages on lines 39 and 72.
.github/workflows/deploy-testnet-by-pr-comment.yml (1)
22-25: Confirm that OWNER/MEMBER-only gating won’t block intended deploy access.Line 22-25 and Line 79-80 now filter by
author_associationto OWNER/MEMBER, which excludes COLLABORATOR/CONTRIBUTOR. If collaborators are meant to get deploy links or trigger/deploy(even when listed inTEST_NODE_MANUAL_DEPLOY_AUTHORIZED_USERS), this will block them. Please verify policy intent; otherwise consider expanding the allowlist.🔧 Optional allowlist expansion
- contains(fromJSON('["OWNER","MEMBER"]'), github.event.pull_request.author_association) + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.pull_request.author_association) ... - contains(fromJSON('["OWNER","MEMBER"]'), github.event.comment.author_association) + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)Also applies to: 79-80
.github/workflows/slack-notify.yml (2)
13-15: Verify reviewer notifications are intentionally limited to OWNER/MEMBER.Line 13-15 now suppresses Slack notifications for PRs opened by COLLABORATOR/CONTRIBUTOR. If external collaborators should still trigger notifications, broaden the allowlist accordingly.
61-61: No review feedback for this change.test-integration/test-tools/src/loaded_accounts.rs (1)
52-60: No review feedback for this change.test-integration/programs/schedulecommit/src/api.rs (1)
92-119: No review feedback for this change.test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
8-8: No review feedback for this change.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| pub fn escrow_lamports_for_payer(&self) -> Result<Signature> { | ||
| let ixs = init_payer_escrow(self.payer_ephem.pubkey()); | ||
| let validator_identity = | ||
| self.common_ctx.ephem_validator_identity.as_ref().copied(); | ||
| let ixs = init_payer_escrow_with_validator( | ||
| self.payer_ephem.pubkey(), | ||
| validator_identity, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Guard against missing validator identity before escrow initialization.
Line 244-249 passes None when ephem_validator_identity is unset, which may lead to escrow delegation without a validator. If production DLP requires a validator, fail fast to make the error explicit.
✅ Suggested guard
- let validator_identity =
- self.common_ctx.ephem_validator_identity.as_ref().copied();
- let ixs = init_payer_escrow_with_validator(
- self.payer_ephem.pubkey(),
- validator_identity,
- );
+ let validator_identity = self
+ .common_ctx
+ .ephem_validator_identity
+ .context("Missing ephem validator identity for escrow init")?;
+ let ixs = init_payer_escrow_with_validator(
+ self.payer_ephem.pubkey(),
+ Some(validator_identity),
+ );📝 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.
| pub fn escrow_lamports_for_payer(&self) -> Result<Signature> { | |
| let ixs = init_payer_escrow(self.payer_ephem.pubkey()); | |
| let validator_identity = | |
| self.common_ctx.ephem_validator_identity.as_ref().copied(); | |
| let ixs = init_payer_escrow_with_validator( | |
| self.payer_ephem.pubkey(), | |
| validator_identity, | |
| ); | |
| pub fn escrow_lamports_for_payer(&self) -> Result<Signature> { | |
| let validator_identity = self | |
| .common_ctx | |
| .ephem_validator_identity | |
| .context("Missing ephem validator identity for escrow init")?; | |
| let ixs = init_payer_escrow_with_validator( | |
| self.payer_ephem.pubkey(), | |
| Some(validator_identity), | |
| ); |
🤖 Prompt for AI Agents
In `@test-integration/schedulecommit/client/src/schedule_commit_context.rs` around
lines 243 - 249, The escrow_lamports_for_payer function currently calls
init_payer_escrow_with_validator passing
self.common_ctx.ephem_validator_identity which may be None; add an explicit
guard in escrow_lamports_for_payer to check that
self.common_ctx.ephem_validator_identity.is_some() (or match on it) and return
an Err with a clear message if missing, otherwise unwrap/copy the
validator_identity and call init_payer_escrow_with_validator as before
(reference symbols: escrow_lamports_for_payer,
common_ctx.ephem_validator_identity, init_payer_escrow_with_validator,
payer_ephem.pubkey()).
| use solana_sdk::transaction::TransactionError; | ||
| if matches!(tx_err, TransactionError::InvalidWritableAccount) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider moving the use statement outside the conditional block.
The inline use statement works but is unconventional. Moving it to the function scope or top of the file would improve readability.
♻️ Suggested refactor
+use solana_sdk::transaction::TransactionError;
+
fn assert_cannot_increase_committee_count(
pda: Pubkey,
payer: &Keypair,
rpc_client: &RpcClient,
) {
// ... existing code ...
let (tx_result_err, tx_err) = extract_transaction_error(tx_res);
if let Some(tx_err) = tx_err {
- use solana_sdk::transaction::TransactionError;
if matches!(tx_err, TransactionError::InvalidWritableAccount) {
return;
}🤖 Prompt for AI Agents
In
`@test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs`
around lines 433 - 436, Move the inline `use
solana_sdk::transaction::TransactionError;` out of the conditional and into the
surrounding scope (either the enclosing function or top of the file) so it's not
declared inside the if block; update the scope where `tx_err` is matched (the
`if matches!(tx_err, TransactionError::InvalidWritableAccount) { return; }`
check) to rely on the now-top-level import of `TransactionError` to improve
readability and conventional organization.
…nfig (magicblock-labs#602) Replace the precompiled dlp.so binary with a production version compiled without the unit_test_config feature, which hardcoded test validator authorities. Update integration tests and test programs to accept and properly pass custom validator identities instead of relying on hardcoded test values. Fixes magicblock-labs#602 Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
AvhiMaz
force-pushed
the
fix/issue-602-production-dlp-validator-authority
branch
from
0fb1223 to
ed1441b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test-integration/programs/flexi-counter/src/instruction.rs`:
- Around line 310-315: The create_delegate_ix_with_validator function currently
ignores its _validator argument; update it so the validator is wired through
instead of discarded: modify or overload create_delegate_ix to accept an
Option<Pubkey> (or a Pubkey) and call that variant from
create_delegate_ix_with_validator with the provided validator (i.e. replace
create_delegate_ix(payer) with create_delegate_ix(payer, _validator) or
construct the Instruction using _validator directly), or if the validator truly
must be ignored, add a clear doc comment on create_delegate_ix_with_validator
explaining why the validator is unused and remove the leading underscore so the
API reflects intent; reference create_delegate_ix_with_validator and
create_delegate_ix when making the change.
♻️ Duplicate comments (2)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
433-436: MoveTransactionErrorimport out of the conditional.
Inlineuseinside a block is non-idiomatic; place it at function scope or file scope.♻️ Suggested refactor
+use solana_sdk::transaction::TransactionError; + fn assert_cannot_increase_committee_count( pda: Pubkey, payer: &Keypair, rpc_client: &RpcClient, ) { // ... let (tx_result_err, tx_err) = extract_transaction_error(tx_res); if let Some(tx_err) = tx_err { - use solana_sdk::transaction::TransactionError; if matches!(tx_err, TransactionError::InvalidWritableAccount) { return; }test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
244-249: Consider adding a guard for missing validator identity.The
fields()method at lines 345-349 calls.unwrap()onephem_validator_identity, indicating it's expected to be present during normal test operation. However,escrow_lamports_for_payersilently passesNonewhen the identity is missing.For consistency and clearer error messages, consider failing fast here:
Suggested change
pub fn escrow_lamports_for_payer(&self) -> Result<Signature> { - let validator_identity = - self.common_ctx.ephem_validator_identity.as_ref().copied(); + let validator_identity = self + .common_ctx + .ephem_validator_identity + .context("Missing ephem validator identity for escrow init")?; let ixs = init_payer_escrow_with_validator( self.payer_ephem.pubkey(), - validator_identity, + Some(validator_identity), );
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 0
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/flexi-counter/src/instruction.rs (1)
275-343: Reduce duplication in delegate instruction builders.
create_delegate_ix_with_commit_frequency_msandcreate_delegate_ix_with_validatorduplicate account meta and argument assembly. A small internal helper prevents drift and makes it easier to support both parameters consistently.♻️ Suggested refactor
+fn create_delegate_ix_internal( + payer: Pubkey, + commit_frequency_ms: u32, + validator: Option<Pubkey>, +) -> Instruction { + let program_id = &crate::id(); + let (pda, _) = FlexiCounter::pda(&payer); + + let delegate_accounts = DelegateAccounts::new(pda, *program_id); + let delegate_metas = DelegateAccountMetas::from(delegate_accounts); + let account_metas = vec![ + AccountMeta::new(payer, true), + delegate_metas.delegated_account, + delegate_metas.owner_program, + delegate_metas.delegate_buffer, + delegate_metas.delegation_record, + delegate_metas.delegation_metadata, + delegate_metas.delegation_program, + delegate_metas.system_program, + ]; + + let args = DelegateArgs { + valid_until: i64::MAX, + commit_frequency_ms, + validator, + }; + + Instruction::new_with_borsh( + *program_id, + &FlexiCounterInstruction::Delegate(args), + account_metas, + ) +} + pub fn create_delegate_ix(payer: Pubkey) -> Instruction { - create_delegate_ix_with_commit_frequency_ms(payer, 0) + create_delegate_ix_internal(payer, 0, None) } pub fn create_delegate_ix_with_commit_frequency_ms( payer: Pubkey, commit_frequency_ms: u32, ) -> Instruction { - let program_id = &crate::id(); - let (pda, _) = FlexiCounter::pda(&payer); - let delegate_accounts = DelegateAccounts::new(pda, *program_id); - let delegate_metas = DelegateAccountMetas::from(delegate_accounts); - let account_metas = vec![ - AccountMeta::new(payer, true), - delegate_metas.delegated_account, - delegate_metas.owner_program, - delegate_metas.delegate_buffer, - delegate_metas.delegation_record, - delegate_metas.delegation_metadata, - delegate_metas.delegation_program, - delegate_metas.system_program, - ]; - let args = DelegateArgs { - valid_until: i64::MAX, - commit_frequency_ms, - validator: None, - }; - Instruction::new_with_borsh( - *program_id, - &FlexiCounterInstruction::Delegate(args), - account_metas, - ) + create_delegate_ix_internal(payer, commit_frequency_ms, None) } pub fn create_delegate_ix_with_validator( payer: Pubkey, validator: Option<Pubkey>, ) -> Instruction { - let program_id = &crate::id(); - let (pda, _) = FlexiCounter::pda(&payer); - let commit_frequency_ms = 0; - let delegate_accounts = DelegateAccounts::new(pda, *program_id); - let delegate_metas = DelegateAccountMetas::from(delegate_accounts); - let account_metas = vec![ - AccountMeta::new(payer, true), - delegate_metas.delegated_account, - delegate_metas.owner_program, - delegate_metas.delegate_buffer, - delegate_metas.delegation_record, - delegate_metas.delegation_metadata, - delegate_metas.delegation_program, - delegate_metas.system_program, - ]; - let args = DelegateArgs { - valid_until: i64::MAX, - commit_frequency_ms, - validator, - }; - Instruction::new_with_borsh( - *program_id, - &FlexiCounterInstruction::Delegate(args), - account_metas, - ) + create_delegate_ix_internal(payer, 0, validator) }
3 participants
Replace the precompiled dlp.so binary with a production version compiled without
the unit_test_config feature, which hardcoded test validator authorities. Update
integration tests and test programs to accept and properly pass custom validator
identities instead of relying on hardcoded test values.
Fixes #602
Summary by CodeRabbit
New Features
Deprecations
Tests
✏️ Tip: You can customize this high-level summary in your review settings.