Make rewards-delegated-vrf and anchor-rock-paper-scissor work with SDK 'backward-compat` flag#82
Open
Make rewards-delegated-vrf and anchor-rock-paper-scissor work with SDK 'backward-compat` flag#82
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughAdds a new rewards-delegated-vrf Anchor program with on-chain state, instruction handlers, shared transfer logic (magic intents), VRF request/consume wiring, vendored ephemeral-vrf SDK (and proc-macro), workspace/tooling files, tests/helpers/fixtures, and switches ephemeral-rollups-sdk references in program manifests to a local path with adjusted features. ChangesRewards Delegated VRF
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/Cargo.toml`:
- Line 24: The Cargo.toml entry for the dependency named ephemeral-rollups-sdk
currently uses a machine-specific absolute path; replace this with a portable
solution by removing the path attribute and either specifying a published
version (e.g., ephemeral-rollups-sdk = "x.y.z" with the required features) or
revert to a local override via a project-level .cargo/config.toml
[patch.crates-io] override; update the dependency declaration in
anchor-rock-paper-scissor's Cargo.toml (and the same dependency in
rewards-delegated-vrf's Cargo.toml) so CI and other developers do not rely on an
absolute filesystem path.
In `@rewards-delegated-vrf/programs/rewards-delegated-vrf/Cargo.toml`:
- Line 24: The Cargo.toml currently pins ephemeral-rollups-sdk to a local
absolute path (ephemeral-rollups-sdk = { path = "/Users/..."}), which breaks CI;
change that dependency to a portable reference by replacing the path entry for
ephemeral-rollups-sdk with either a published crate version (e.g.,
ephemeral-rollups-sdk = "x.y.z") or a git-based reference (e.g.,
ephemeral-rollups-sdk = { git = "https://...", tag = "vX.Y.Z" }) and preserve
any needed features like ["anchor","backward-compat"]; make the same change in
the other occurrence for anchor-rock-paper-scissor so both Cargo.toml files use
a portable version/git dependency instead of the local path.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 348db2ce-a225-4e7c-8b3c-edf16195a680
⛔ Files ignored due to path filters (1)
anchor-rock-paper-scissor/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/Cargo.tomlrewards-delegated-vrf/programs/rewards-delegated-vrf/Cargo.toml
| [dependencies] | ||
| anchor-lang = { version = "0.32.1", features = ["init-if-needed"] } | ||
| ephemeral-rollups-sdk = { version = "0.11.0", features = ["anchor", "access-control"] } | ||
| ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor", "access-control", "backward-compat"] } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify committed Cargo manifests do not contain absolute filesystem path deps.
# Expected: no matches for path = "/..."
fd -HI 'Cargo.toml' | xargs rg -nP '^\s*[A-Za-z0-9_-]+\s*=\s*\{[^}]*\bpath\s*=\s*"/'
# Focus check for this dependency specifically.
fd -HI 'Cargo.toml' | xargs rg -nP '^\s*ephemeral-rollups-sdk\s*=\s*\{[^}]*\bpath\s*='Repository: magicblock-labs/magicblock-engine-examples
Length of output: 948
Remove machine-specific absolute path dependency; breaks CI and portability across the repository.
Line 24 contains path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", which is non-portable and will fail in CI and for other developers. This dependency is also present in rewards-delegated-vrf/programs/rewards-delegated-vrf/Cargo.toml. Use a published version number or a local .cargo/config.toml override instead of committing absolute paths.
Suggested fix
-ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor", "access-control", "backward-compat"] }
+ephemeral-rollups-sdk = { version = "0.11.1", features = ["anchor", "access-control", "backward-compat"] }📝 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
| ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor", "access-control", "backward-compat"] } | |
| ephemeral-rollups-sdk = { version = "0.11.1", features = ["anchor", "access-control", "backward-compat"] } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/Cargo.toml` at
line 24, The Cargo.toml entry for the dependency named ephemeral-rollups-sdk
currently uses a machine-specific absolute path; replace this with a portable
solution by removing the path attribute and either specifying a published
version (e.g., ephemeral-rollups-sdk = "x.y.z" with the required features) or
revert to a local override via a project-level .cargo/config.toml
[patch.crates-io] override; update the dependency declaration in
anchor-rock-paper-scissor's Cargo.toml (and the same dependency in
rewards-delegated-vrf's Cargo.toml) so CI and other developers do not rely on an
absolute filesystem path.
| anchor-lang = { version = "0.32.1", features = ["init-if-needed"] } | ||
| anchor-spl = { version = "0.32.1", features = ["idl-build", "metadata", "associated_token"] } | ||
| ephemeral-rollups-sdk = { version = "0.11.1" , features = ["anchor", "disable-realloc"] } | ||
| ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor","backward-compat"] } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "Cargo.toml" | grep -E "rewards-delegated-vrf.*Cargo.toml"Repository: magicblock-labs/magicblock-engine-examples
Length of output: 187
🏁 Script executed:
cat -n rewards-delegated-vrf/programs/rewards-delegated-vrf/Cargo.toml | head -40Repository: magicblock-labs/magicblock-engine-examples
Length of output: 1079
🏁 Script executed:
cd rewards-delegated-vrf/programs/rewards-delegated-vrf && git log --oneline -5 -- Cargo.toml 2>/dev/null || echo "Git info not available"Repository: magicblock-labs/magicblock-engine-examples
Length of output: 188
🏁 Script executed:
cat -n rewards-delegated-vrf/Cargo.toml | head -20Repository: magicblock-labs/magicblock-engine-examples
Length of output: 399
🏁 Script executed:
rg "ephemeral-rollups-sdk" --type tomlRepository: magicblock-labs/magicblock-engine-examples
Length of output: 10492
Fix absolute path dependency to use a portable version.
On line 24, the committed path dependency points to a local user directory and will break in CI and other environments. Replace with a version constraint:
Suggested fix
-ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor","backward-compat"] }
+ephemeral-rollups-sdk = { version = "0.11.1", features = ["anchor", "backward-compat"] }Note: The same issue exists in anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/Cargo.toml and should also be fixed.
📝 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
| ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor","backward-compat"] } | |
| ephemeral-rollups-sdk = { version = "0.11.1", features = ["anchor", "backward-compat"] } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rewards-delegated-vrf/programs/rewards-delegated-vrf/Cargo.toml` at line 24,
The Cargo.toml currently pins ephemeral-rollups-sdk to a local absolute path
(ephemeral-rollups-sdk = { path = "/Users/..."}), which breaks CI; change that
dependency to a portable reference by replacing the path entry for
ephemeral-rollups-sdk with either a published crate version (e.g.,
ephemeral-rollups-sdk = "x.y.z") or a git-based reference (e.g.,
ephemeral-rollups-sdk = { git = "https://...", tag = "vX.Y.Z" }) and preserve
any needed features like ["anchor","backward-compat"]; make the same change in
the other occurrence for anchor-rock-paper-scissor so both Cargo.toml files use
a portable version/git dependency instead of the local path.
…K 'backward-compat` flag
snawaz
force-pushed
the
snawaz/sdk-backward-compat
branch
from
0a9ddcd to
5d1bce9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 30
♻️ Duplicate comments (1)
rewards-delegated-vrf/programs/rewards-delegated-vrf/Cargo.toml (1)
24-24:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix absolute path dependency to use a portable version.
The committed
pathdependency points to a local user directory and will break in CI and other environments. Replace with a version constraint or git reference that includes thebackward-compatfeature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rewards-delegated-vrf/programs/rewards-delegated-vrf/Cargo.toml` at line 24, The Cargo.toml currently pins ephemeral-rollups-sdk using a local absolute path (the line containing ephemeral-rollups-sdk = { path = "...", features = ["anchor","backward-compat"] }), which is not portable; replace that path-based dependency with a remote dependency (either a crates.io version constraint like ephemeral-rollups-sdk = { version = "x.y.z", features = ["anchor","backward-compat"] } or a git reference like ephemeral-rollups-sdk = { git = "https://...", rev = "...", features = ["anchor","backward-compat"] }) ensuring the "backward-compat" and "anchor" features are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rewards-delegated-vrf-1.0/Anchor.toml`:
- Around line 18-22: Change the hardcoded devnet endpoints and Anchor default to
use configurable env vars so tests can run deterministically on localnet: update
Anchor.toml's cluster default from "devnet" to a local default (e.g., "localnet"
or remove default), and in the test setup in rewards-delegated-vrf.ts replace
the literal RPC and WS URLs ("https://devnet-as.magicblock.app/" and
"wss://devnet-as.magicblock.app/") with reads from environment variables (e.g.,
RPC_ENDPOINT and WS_ENDPOINT), defaulting to the local validator endpoints
(http://127.0.0.1:8899 and ws://127.0.0.1:8900 or similar) when the env vars are
not set; ensure any test helper or connection-creation function (the module that
constructs the Connection or WebSocket) uses these variables so tests can target
a local isolated cluster and the EPHEMERAL_WS_ENDPOINT override remains
supported.
In `@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/Cargo.toml`:
- Line 24: The dependency declaration for ephemeral-rollups-sdk currently
enables only the "anchor" feature; update the features list for the
ephemeral-rollups-sdk dependency (the features = [...] entry) to also include
"backward-compat" so the rewards program builds with SDK backward compatibility
enabled (i.e., change the features array on the ephemeral-rollups-sdk dependency
to include both "anchor" and "backward-compat").
In `@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/helpers.rs`:
- Around line 50-83: validate_reward_state currently checks logical invariants
but does not enforce the fixed-size caps that Reward::MAX_SIZE relies on; update
validate_reward_state to also validate the serialized-size-related limits (e.g.,
check reward.name.len() <= NAME_MAX_LEN or the constant used by Reward, verify
reward.reward_mints.len() <= MAX_REWARD_MINTS, and
reward.additional_pubkeys.len() <= MAX_ADDITIONAL_PUBKEYS) and return an
appropriate RewardError (e.g., InvalidRewardSize or a new error variant) with a
msg! on violation so writes cannot exceed the account size assumed by
Reward::MAX_SIZE; reference the Reward struct and Reward::MAX_SIZE and add the
checks near the other validations in validate_reward_state.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/add_reward.rs`:
- Around line 76-91: The branch that tops up an existing fungible reward should
also verify the provided mint matches the existing reward's mint: in the
(Some(reward_index), RewardType::SplToken | RewardType::SplToken2022) arm, after
fetching let existing_reward = &reward_list.rewards[reward_index]; add a
require! check that existing_reward.mint == mint (or the incoming mint variable)
and return an appropriate error (e.g., RewardError::MintMismatch) if they
differ, before computing redemptions_added and updating
reward_list.rewards[reward_index].redemption_limit.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/consume_random_reward.rs`:
- Around line 30-35: The random-range arithmetic must be done in u64 to avoid a
zero modulus when the range equals 2^32; change the computation so you cast the
RNG output (ephemeral_vrf_sdk::rnd::random_u32) to u64, compute range as u64
using reward_list.global_range_max and global_range_min, do the modulus with
that u64 range (e.g., (rnd_u32 as u64) % range), then add global_range_min as
u64 and only cast the final result back to u32 if the surrounding API requires
it; update the code around rnd::random_u32, range, and result to use u64 math to
prevent rnd % 0 panics.
- Around line 53-99: The code increments reward.redemption_count and mutates
reward.reward_mints before ensuring a transfer will occur; move the
redemption_count increment (and any mutation like reward.reward_mints.remove) to
after a successful execute_reward_transfer call so you only mark a reward
redeemed when the transfer returns Ok; specifically, in consume_random_reward
check transfer_lookup_table.lookup_accounts.is_empty first, pick (but do not
remove) the mint/index for NFTs (use a copy of the mint or index), call
execute_reward_transfer, and only upon Ok perform reward.redemption_count =
reward.redemption_count.saturating_add(1) and remove the selected mint from
reward.reward_mints if needed, keeping the break behavior unchanged.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/initialize_reward_distributor.rs`:
- Around line 20-22: The code currently builds all_admins by only filtering out
super_admin but does not remove duplicate Pubkeys from admins, which wastes
account space; change the construction of all_admins (used to set
reward_distributor.admins) to produce a deduplicated list while preserving
order: start with super_admin, then iterate admins and push each admin only if
it is not equal to super_admin and has not already been seen (use a HashSet/seen
set of Pubkey to track uniqueness). Ensure you reference and update the
variables all_admins, super_admin, admins, and reward_distributor.admins
accordingly so duplicates are never serialized.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/initialize_transfer_lookup_table.rs`:
- Line 15: The assignment table.lookup_accounts = lookup_accounts allows
unbounded input; add a size validation before assigning by checking
lookup_accounts.len() <= MAX_LOOKUP_ACCOUNTS (or compute max from the allocated
account space) and return a deterministic custom error (e.g.,
Error::LookupTableTooLarge) if exceeded; perform this check in the initialize
handler immediately before assigning to table.lookup_accounts and only assign
when the length is valid.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/remove_reward.rs`:
- Around line 96-99: The calculation for amount in remove_reward.rs is unsafe:
first validate redemption_amount and guard against overflow before computing
amount. For RewardType::LegacyNft and RewardType::ProgrammableNft, require
redemption_amount.is_none() or redemption_amount == Some(1) and return an error
if Some(0) or Some(n>1) to prevent zero or multi-NFT removals; for fungible
branches use checked_mul (or checked_mul on reward_amount and
redemption_amount.unwrap_or(1)) and return an error on overflow instead of
letting wrapping occur. Update the logic around RewardType, redemption_amount,
reward_amount and the computed amount to perform these validations and early
returns.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/set_admins.rs`:
- Around line 13-15: After prepending super_admin into unique_admins you must
validate the vector's length before assigning to reward_distributor.admins to
prevent serialization failures; check unique_admins.len() against the contract's
MAX_ADMINS constant (or define one if missing) and return an error (e.g.,
ProgramError::AccountDataTooSmall or a custom error) when it exceeds the limit,
then only set reward_distributor.admins = unique_admins when the size is within
bounds.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/set_whitelist.rs`:
- Line 9: Before writing into reward_distributor.whitelist, validate the
deduplicated whitelist length against the contract's max capacity: call
remove_duplicate_pubkeys(whitelist) into a temp variable, check its .len() <=
MAX_WHITELIST_LEN (or RewardDistributor::MAX_WHITELIST_LEN), and if it exceeds
return a custom program error (e.g., Error::WhitelistTooLarge). Do this check
inside the set_whitelist instruction handler (the function that currently
assigns reward_distributor.whitelist) and only assign
reward_distributor.whitelist = deduped_whitelist after the length guard passes.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/shared.rs`:
- Around line 33-205: The match currently treats RewardType::SplToken2022 as
unsupported; update the match in shared.rs so SplToken2022 is handled like
SplToken: either add RewardType::SplToken2022 to the first arm's pattern or add
a new arm that duplicates the SplToken logic (use
crate::instruction::TransferRewardSplToken for instruction_data, derive
source/destination with get_associated_token_address, build action_accounts,
create CallHandler and invoke via MagicIntentBundleBuilder with payer_seeds).
Ensure you reference RewardType::SplToken2022, TransferRewardSplToken,
get_associated_token_address, CallHandler and MagicIntentBundleBuilder so the
new arm mirrors the existing SplToken flow.
- Around line 26-31: Add a bounds check before indexing
transfer_lookup_table.lookup_accounts to avoid panics: verify
transfer_lookup_table.lookup_accounts.len() >= 6 at the start of the helper (the
block that assigns token_program, ata_program, system_program,
token_metadata_program, sysvar_instructions_program, auth_rule_program) and
return a recoverable program error (use an appropriate custom error or a
ProgramError like InvalidAccountData) if the length is too small; this ensures
the helper (in shared.rs where transfer_lookup_table.lookup_accounts is
accessed) fails gracefully instead of panicking when the lookup table is
partially initialized.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/transfer_reward_programmable_nft.rs`:
- Around line 8-10: The function transfer_reward_programmable_nft accepts an
amount parameter that is not enforced by the transfer CPI; add an explicit
runtime check at the start of transfer_reward_programmable_nft
(Context<TransferRewardProgrammableNft>) to reject any amount other than 1
(e.g., return a descriptive Anchor/Program error like "AmountMustBeOne") and
update any log messages to assume amount == 1; apply the same enforcement to the
other affected instruction implementations mentioned (lines 38-60) so NFT-only
transfers cannot be called with 0 or >1.
- Around line 26-36: The CPI is using the wrong program key when building the
CpiContext for Create: change the program passed to CpiContext::new from
ctx.accounts.token_program.key() to the associated token program key
(ctx.accounts.associated_token_program.key()) so that
create_idempotent(cpi_ata_ctx) targets the associated token program; adjust the
cpi_ata_program assignment used by CpiContext::new accordingly (symbols:
create_idempotent, cpi_ata_program, cpi_ata_ctx,
ctx.accounts.associated_token_program).
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/transfer_reward_spl_token.rs`:
- Around line 50-53: The multiplication by 10^decimals is done on u64 and can
overflow; compute the scaled transfer amount with checked arithmetic using a
wider type (e.g., cast amount to u128, compute 10u128.pow(decimals as u32) and
perform checked_mul/checked_pow), then ensure the result fits back into u64 (or
return a program error) before calling transfer_checked; update the code paths
around transfer_checked, cpi_transfer_ctx, amount and ctx.accounts.mint.decimals
to perform these checks and return a suitable error (or define an AmountOverflow
error) if any checked operation fails.
- Around line 34-49: The CPI contexts are being given Pubkeys instead of
AccountInfo and the ATA CPI should call the associated token program; change
cpi_ata_program to use ctx.accounts.associated_token_program.to_account_info()
when building the CpiContext passed to create_idempotent (so CpiContext::new
gets an AccountInfo), and change cpi_transfer_program to use
ctx.accounts.token_program.to_account_info() when building the
CpiContext::new_with_signer for the TransferChecked CPI; ensure the variables
used in create_idempotent, CpiContext::new, and CpiContext::new_with_signer
(cpi_ata_program, cpi_ata_ctx, cpi_transfer_program, cpi_transfer_ctx) are
AccountInfo values rather than Pubkey.
In `@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/lib.rs`:
- Around line 237-239: magic_fee_vault is currently accepted as an unchecked
mutable AccountInfo which allows callers to substitute an arbitrary vault;
compute and validate the expected PDA for magic_fee_vault from the delegation
record instead of accepting it blindly. Specifically, in the account validation
for handlers that use magic_fee_vault (including the RemoveReward flow and the
other commit flow), derive the expected PDA using the delegation_record (and the
same seeds used to create the vault), then assert the provided
magic_fee_vault.key equals the derived PDA and that its owner/program and token
account fields (mint/authority) match expectations before using it in the CPI;
update the account struct/handlers that reference magic_fee_vault to perform
this check so callers cannot forward an arbitrary writable account.
- Around line 211-222: The reward_list account in request_random_reward is not
seed-constrained to the provided reward_distributor, allowing mismatched
delegated lists; update the account attribute for reward_list in the
request_random_reward context to enforce a PDA derived from reward_distributor
(e.g., add #[account(seeds = [b"rewards_list",
reward_distributor.key().as_ref()], bump)] or the project’s canonical
rewards-list seed + reward_distributor.key()), so the runtime checks the list
belongs to that distributor; ensure the same PDA derivation is used where
consume_random_reward expects the PDA and keep delegation_record_reward_list's
address check unchanged.
- Around line 303-304: The account structs used by
transfer_reward_programmable_nft expose unchecked program/sysvar accounts; pin
token_metadata_program, sysvar_instruction_program, associated_token_program,
and auth_rule_program to their canonical IDs by adding explicit constraints on
those AccountInfo fields (or switching them to Program<'info> with explicit
checks) so CPI calls cannot be redirected: validate token_metadata_program ==
mpl_token_metadata::ID, sysvar_instruction_program == sysvar::instructions::ID,
associated_token_program == anchor_spl::associated_token::ID, and
auth_rule_program == mpl_auth_rules::ID (or otherwise verify the expected
pubkeys) before performing the CPI in transfer_reward_programmable_nft.
- Around line 186-189: The current check only ensures `authority` matches some
upgradeable program's upgrade authority but not that `program_data` is the
ProgramData account for THIS program; update the `program_data` account
constraint to also verify its key equals the ProgramData address derived for
this program (use the BPF loader helper to derive it from this program's id),
e.g. derive the expected ProgramData PDA from this program id (crate::ID) via
the bpf loader helper and add a constraint like `constraint = program_data.key()
== <derived_program_data_pubkey>` so `program_data` is guaranteed to belong to
this program before touching `transfer_lookup_table`.
In `@rewards-delegated-vrf-1.0/README.md`:
- Line 12: Update the README entry that currently shows "Rust | 1.85.0" to match
the toolchain version specified in rust-toolchain.toml (1.93.1); change the
displayed version to 1.93.1 and update any example commands in the README that
reference installing or setting the toolchain (e.g., rustup install / rustup
override commands) so they use 1.93.1 to keep the documentation consistent with
the rust-toolchain.toml configuration.
In `@rewards-delegated-vrf-1.0/tests/fixtures/user-keypair.json`:
- Around line 2-67: The committed fixture contains raw private key bytes in the
"secretKey" field which must be removed; replace the hard-coded secretKey array
in tests/fixtures/user-keypair.json with a non-sensitive placeholder (or remove
the file) and update tests to generate keys at runtime (e.g., use
Keypair.generate() or a helper like createTestKeypair()) or load from a local,
gitignored fixture or environment variable; ensure any code referencing the
"secretKey" in tests or helpers (search for "user-keypair.json" and uses of
"secretKey") is updated to accept runtime-provided keypairs instead of the
committed secret.
In `@rewards-delegated-vrf-1.0/tests/helpers.ts`:
- Around line 182-195: The localhost branch of getValidatorAccounts uses new
web3.PublicKey(...) but the file imports PublicKey directly; change the
construction to new PublicKey(...) (referencing the imported PublicKey) in the
getValidatorAccounts function and ensure the existing import for PublicKey from
'@solana/web3.js' is present so the localhost validator pubkey creation doesn't
throw a runtime error.
In `@rewards-delegated-vrf-1.0/tests/rewards-delegated-vrf.ts`:
- Line 31: The test suite is inadvertently limited by describe.only in the
"rewards-delegated-vrf" describe block; remove the .only so the suite reads
describe("rewards-delegated-vrf", ...) (or revert to the original describe call)
to ensure all tests run in CI and locally.
- Around line 622-675: The test currently swallows send errors and treats a
timed-out VRF callback as success; change the sendAndConfirm error handler on
providerEphemeralRollup.sendAndConfirm so it does not return null but instead
fails the test (rethrow or assert) when the transaction cannot be sent, and
modify the callback promise logic around callbackReceived/listener so a boolean
flag (e.g., callbackSeen) is set to true inside the onLogs handler when a valid
VRF callback is observed (check relevantLogs or logs.err), and replace the
timeout resolve with a reject (or assert failure) if callbackSeen is false after
30s; ensure you reference txHash, callbackReceived, listener/listenerRemoved,
and ephemeralProgram.provider.connection.onLogs when making these changes so the
test fails when VRF flow is broken.
In `@rewards-delegated-vrf-1.0/tests/setup.ts`:
- Around line 77-84: The final balance probe uses provider.connection.getBalance
outside any try/catch so a flaky RPC can throw; wrap the call that sets
finalBalance and the subsequent console.logs in a try/catch (targeting the
provider.connection.getBalance call and usage of finalBalance / LAMPORTS_PER_SOL
and user.publicKey) and on error log a non-fatal warning like "Could not fetch
final balance" (including the caught error) and continue without throwing;
preserve the existing zero-balance warning when finalBalance is obtained
successfully.
In `@rewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/anchor.rs`:
- Around line 11-15: The impl of anchor_lang::Id for VrfProgram unnecessarily
converts VRF_PROGRAM_ID to bytes then back to a Pubkey; update the id()
implementation in the VrfProgram impl to return crate::consts::VRF_PROGRAM_ID
directly (replace the
Pubkey::new_from_array(crate::consts::VRF_PROGRAM_ID.to_bytes()) expression with
crate::consts::VRF_PROGRAM_ID) since anchor_lang::prelude::Pubkey is the same
type here.
In `@rewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/rnd.rs`:
- Around line 31-43: random_u8_with_range can divide/modulo by zero when range
== 256 and also accepts invalid ranges where max_value < min_value; fix by
validating inputs at the top of random_u8_with_range: if max_value < min_value
return/err (or swap) to reject invalid ranges, and if (max_value - min_value +
1) == 256 (i.e., full byte range) short-circuit and return a raw byte from bytes
(e.g., bytes[31]) without doing `% range as u8`; keep the existing unbiased loop
and fallback logic for other ranges but avoid casting range to u8 when range ==
256.
In `@rewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/vrf-macro/src/lib.rs`:
- Around line 87-102: The macro-generated method invoke_signed_vrf references
self.oracle_queue but the macro doesn't validate or auto-inject an oracle_queue
field; update the macro to detect a missing oracle_queue at expansion time (same
style as the existing unnamed-fields enforcement/validation used for
program_identity, vrf_program, slot_hashes, system_program) and either (a) emit
a clear compile-time error when oracle_queue is not declared on the target
struct, or (b) auto-inject an oracle_queue account mapping like the other four
accounts (ensuring the correct mutability/readonly and PDA seeds if needed);
ensure the check/auto-injection logic is added where the macro collects and
validates required accounts so invoke_signed_vrf can safely call
self.oracle_queue.to_account_info().
---
Duplicate comments:
In `@rewards-delegated-vrf/programs/rewards-delegated-vrf/Cargo.toml`:
- Line 24: The Cargo.toml currently pins ephemeral-rollups-sdk using a local
absolute path (the line containing ephemeral-rollups-sdk = { path = "...",
features = ["anchor","backward-compat"] }), which is not portable; replace that
path-based dependency with a remote dependency (either a crates.io version
constraint like ephemeral-rollups-sdk = { version = "x.y.z", features =
["anchor","backward-compat"] } or a git reference like ephemeral-rollups-sdk = {
git = "https://...", rev = "...", features = ["anchor","backward-compat"] })
ensuring the "backward-compat" and "anchor" features are preserved.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b391658-4e48-4a36-9c4d-49c1b853bcb1
⛔ Files ignored due to path filters (4)
anchor-rock-paper-scissor/Cargo.lockis excluded by!**/*.lockrewards-delegated-vrf-1.0/Cargo.lockis excluded by!**/*.lockrewards-delegated-vrf-1.0/yarn.lockis excluded by!**/yarn.lock,!**/*.lockrewards-delegated-vrf/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/Cargo.tomlrewards-delegated-vrf-1.0/.gitignorerewards-delegated-vrf-1.0/.prettierignorerewards-delegated-vrf-1.0/Anchor.tomlrewards-delegated-vrf-1.0/Cargo.tomlrewards-delegated-vrf-1.0/README.mdrewards-delegated-vrf-1.0/package.jsonrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/Cargo.tomlrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/constants.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/errors.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/helpers.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/add_reward.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/consume_random_reward.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/delegate_reward_list.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/initialize_reward_distributor.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/initialize_transfer_lookup_table.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/mod.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/remove_reward.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/request_random_reward.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/set_admins.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/set_reward_list.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/set_whitelist.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/shared.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/transfer_reward_programmable_nft.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/transfer_reward_spl_token.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/undelegate_reward_list.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/update_reward.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/lib.rsrewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/state.rsrewards-delegated-vrf-1.0/rust-toolchain.tomlrewards-delegated-vrf-1.0/tests/constants.tsrewards-delegated-vrf-1.0/tests/fixtures/1001.jsonrewards-delegated-vrf-1.0/tests/fixtures/merch-collection.jsonrewards-delegated-vrf-1.0/tests/fixtures/nft-mints.jsonrewards-delegated-vrf-1.0/tests/fixtures/user-keypair.jsonrewards-delegated-vrf-1.0/tests/helpers.tsrewards-delegated-vrf-1.0/tests/pdas.tsrewards-delegated-vrf-1.0/tests/rewards-delegated-vrf.tsrewards-delegated-vrf-1.0/tests/setup.tsrewards-delegated-vrf-1.0/tsconfig.jsonrewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/Cargo.tomlrewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/anchor.rsrewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/consts.rsrewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/instructions.rsrewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/lib.rsrewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/pda.rsrewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/rnd.rsrewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/types.rsrewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/vrf-macro/Cargo.tomlrewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/vrf-macro/src/lib.rsrewards-delegated-vrf/programs/rewards-delegated-vrf/Cargo.toml
Comment on lines
+18
to
+22
| cluster = "devnet" | ||
| wallet = "~/.config/solana/id.json" | ||
|
|
||
| [scripts] | ||
| test = "yarn run ts-mocha -p ./tsconfig.json -t 120000 tests/**/*.ts" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify provider defaults and test command
sed -n '1,120p' rewards-delegated-vrf-1.0/Anchor.toml
# Check whether tests or helpers override provider/cluster explicitly
rg -n --type=ts 'AnchorProvider\.env|setProvider|ANCHOR_PROVIDER_URL|clusterApiUrl|devnet|localnet' rewards-delegated-vrf-1.0/tests rewards-delegated-vrf-1.0Repository: magicblock-labs/magicblock-engine-examples
Length of output: 1435
🏁 Script executed:
fd -t f '\.(ts|js)$' rewards-delegated-vrf-1.0/testsRepository: magicblock-labs/magicblock-engine-examples
Length of output: 312
🏁 Script executed:
cat -n rewards-delegated-vrf-1.0/tests/helpers.ts
cat -n rewards-delegated-vrf-1.0/tests/setup.ts
cat -n rewards-delegated-vrf-1.0/tests/constants.tsRepository: magicblock-labs/magicblock-engine-examples
Length of output: 13626
Hardcoded devnet infrastructure makes tests non-deterministic and stateful; requires code refactoring, not just configuration change
Line 18 sets the default cluster to devnet in Anchor.toml, and the test command (line 22) lacks local cluster enforcement. More critically, the test code itself hardcodes devnet endpoints: rewards-delegated-vrf.ts lines 38 and 41 use "https://devnet-as.magicblock.app/" and "wss://devnet-as.magicblock.app/" directly in the RPC configuration. While the WebSocket endpoint can be overridden via EPHEMERAL_WS_ENDPOINT environment variable, the RPC endpoint remains hardcoded, preventing tests from running against localnet without code changes. This couples tests to external devnet infrastructure and makes them flaky and stateful. Extract the cluster endpoint(s) to environment variables or a configuration file, and ensure tests default to a local/isolated cluster.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rewards-delegated-vrf-1.0/Anchor.toml` around lines 18 - 22, Change the
hardcoded devnet endpoints and Anchor default to use configurable env vars so
tests can run deterministically on localnet: update Anchor.toml's cluster
default from "devnet" to a local default (e.g., "localnet" or remove default),
and in the test setup in rewards-delegated-vrf.ts replace the literal RPC and WS
URLs ("https://devnet-as.magicblock.app/" and "wss://devnet-as.magicblock.app/")
with reads from environment variables (e.g., RPC_ENDPOINT and WS_ENDPOINT),
defaulting to the local validator endpoints (http://127.0.0.1:8899 and
ws://127.0.0.1:8900 or similar) when the env vars are not set; ensure any test
helper or connection-creation function (the module that constructs the
Connection or WebSocket) uses these variables so tests can target a local
isolated cluster and the EPHEMERAL_WS_ENDPOINT override remains supported.
| [dependencies] | ||
| anchor-lang = { version = "1.0.0", features = ["init-if-needed"] } | ||
| anchor-spl = { version = "1.0.0", features = ["idl-build", "metadata", "associated_token"] } | ||
| ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor"] } |
Contributor
There was a problem hiding this comment.
backward-compat feature is missing from the rewards program SDK dependency
Per the PR objective, this example should work with SDK backward compatibility enabled, but Line 24 only sets features = ["anchor"].
Suggested fix
-ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor"] }
+ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor", "backward-compat"] }📝 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
| ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor"] } | |
| ephemeral-rollups-sdk = { path = "/Users/snawaz/projects/mb/ephemeral-rollups-sdk/rust/sdk", features = ["anchor", "backward-compat"] } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/Cargo.toml` at line
24, The dependency declaration for ephemeral-rollups-sdk currently enables only
the "anchor" feature; update the features list for the ephemeral-rollups-sdk
dependency (the features = [...] entry) to also include "backward-compat" so the
rewards program builds with SDK backward compatibility enabled (i.e., change the
features array on the ephemeral-rollups-sdk dependency to include both "anchor"
and "backward-compat").
Comment on lines
+50
to
+83
| pub fn validate_reward_state(reward: &Reward) -> Result<()> { | ||
| // Check that draw_range_min <= draw_range_max | ||
| if reward.draw_range_min > reward.draw_range_max { | ||
| msg!( | ||
| "Reward '{}' has invalid draw range: min ({}) > max ({})", | ||
| reward.name, | ||
| reward.draw_range_min, | ||
| reward.draw_range_max | ||
| ); | ||
| return Err(RewardError::InvalidDrawRange.into()); | ||
| } | ||
|
|
||
| // Check that redemption_count <= redemption_limit | ||
| if reward.redemption_count > reward.redemption_limit { | ||
| msg!( | ||
| "Reward '{}' has invalid state: redemption_count ({}) > redemption_limit ({})", | ||
| reward.name, | ||
| reward.redemption_count, | ||
| reward.redemption_limit | ||
| ); | ||
| return Err(RewardError::InvalidRedemptionState.into()); | ||
| } | ||
|
|
||
| // Check that reward_amount is greater than 0 | ||
| if reward.reward_amount == 0 { | ||
| msg!( | ||
| "Reward '{}' has invalid reward_amount: must be greater than 0", | ||
| reward.name | ||
| ); | ||
| return Err(RewardError::InvalidRewardAmount.into()); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce serialized-size caps that Reward::MAX_SIZE assumes
validate_reward_state does not enforce the fixed limits implied by Reward::MAX_SIZE (name length, reward_mints count, additional_pubkeys count). Without these checks, valid writes can later fail due to account-size overflow.
Suggested fix
pub fn validate_reward_state(reward: &Reward) -> Result<()> {
+ const MAX_REWARD_NAME_LEN: usize = 46;
+ const MAX_REWARD_MINTS: usize = 25;
+ const MAX_ADDITIONAL_PUBKEYS: usize = 3;
+
+ require!(
+ reward.name.len() <= MAX_REWARD_NAME_LEN,
+ RewardError::RewardNameTooLong
+ );
+ require!(
+ reward.reward_mints.len() <= MAX_REWARD_MINTS,
+ RewardError::TooManyRewardMints
+ );
+ require!(
+ reward.additional_pubkeys.len() <= MAX_ADDITIONAL_PUBKEYS,
+ RewardError::TooManyAdditionalPubkeys
+ );
+
// Check that draw_range_min <= draw_range_max
if reward.draw_range_min > reward.draw_range_max {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/helpers.rs`
around lines 50 - 83, validate_reward_state currently checks logical invariants
but does not enforce the fixed-size caps that Reward::MAX_SIZE relies on; update
validate_reward_state to also validate the serialized-size-related limits (e.g.,
check reward.name.len() <= NAME_MAX_LEN or the constant used by Reward, verify
reward.reward_mints.len() <= MAX_REWARD_MINTS, and
reward.additional_pubkeys.len() <= MAX_ADDITIONAL_PUBKEYS) and return an
appropriate RewardError (e.g., InvalidRewardSize or a new error variant) with a
msg! on violation so writes cannot exceed the account size assumed by
Reward::MAX_SIZE; reference the Reward struct and Reward::MAX_SIZE and add the
checks near the other validations in validate_reward_state.
Comment on lines
+76
to
+91
| (Some(reward_index), RewardType::SplToken | RewardType::SplToken2022) => { | ||
| let existing_reward = &reward_list.rewards[reward_index]; | ||
| require!( | ||
| existing_reward.reward_type == detected_type, | ||
| RewardError::RewardTypeMismatch | ||
| ); | ||
|
|
||
| let redemptions_added = | ||
| redemption_limit.ok_or(RewardError::MissingRedemptionsAdded)?; | ||
| let old_limit = existing_reward.redemption_limit; | ||
| let updated_limit = old_limit | ||
| .checked_add(redemptions_added) | ||
| .ok_or(RewardError::ArithmeticOverflow)?; | ||
|
|
||
| // Existing fungible rewards only grow their redeemable inventory. | ||
| reward_list.rewards[reward_index].redemption_limit = updated_limit; |
Contributor
There was a problem hiding this comment.
Require the same mint when topping up an existing fungible reward.
This branch only checks reward_type. Reusing an existing reward name with a different SPL mint will still increase redemption_limit, but the reward continues to point at its original mint, which can leave the reward undercollateralized or misaccounted.
Suggested fix
(Some(reward_index), RewardType::SplToken | RewardType::SplToken2022) => {
let existing_reward = &reward_list.rewards[reward_index];
require!(
existing_reward.reward_type == detected_type,
RewardError::RewardTypeMismatch
);
+ require!(
+ existing_reward.reward_mints.first().copied() == Some(mint.key()),
+ RewardError::MintNotFoundInReward
+ );
let redemptions_added =
redemption_limit.ok_or(RewardError::MissingRedemptionsAdded)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/add_reward.rs`
around lines 76 - 91, The branch that tops up an existing fungible reward should
also verify the provided mint matches the existing reward's mint: in the
(Some(reward_index), RewardType::SplToken | RewardType::SplToken2022) arm, after
fetching let existing_reward = &reward_list.rewards[reward_index]; add a
require! check that existing_reward.mint == mint (or the incoming mint variable)
and return an appropriate error (e.g., RewardError::MintMismatch) if they
differ, before computing redemptions_added and updating
reward_list.rewards[reward_index].redemption_limit.
Comment on lines
+30
to
+35
| let rnd_u32 = ephemeral_vrf_sdk::rnd::random_u32(&randomness); | ||
| let range = (reward_list.global_range_max as u64) | ||
| .checked_sub(reward_list.global_range_min as u64) | ||
| .unwrap() | ||
| + 1; | ||
| let result = reward_list.global_range_min + (rnd_u32 % range as u32); |
Contributor
There was a problem hiding this comment.
Keep the random-range math in u64.
Line 35 casts a possible 2^32 range back to u32. If global_range_min == 0 and global_range_max == u32::MAX, that cast becomes 0 and rnd_u32 % 0 panics on a valid configuration.
Suggested fix
let rnd_u32 = ephemeral_vrf_sdk::rnd::random_u32(&randomness);
let range = (reward_list.global_range_max as u64)
.checked_sub(reward_list.global_range_min as u64)
.unwrap()
+ 1;
- let result = reward_list.global_range_min + (rnd_u32 % range as u32);
+ let offset = ((rnd_u32 as u64) % range) as u32;
+ let result = reward_list.global_range_min + offset;📝 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
| let rnd_u32 = ephemeral_vrf_sdk::rnd::random_u32(&randomness); | |
| let range = (reward_list.global_range_max as u64) | |
| .checked_sub(reward_list.global_range_min as u64) | |
| .unwrap() | |
| + 1; | |
| let result = reward_list.global_range_min + (rnd_u32 % range as u32); | |
| let rnd_u32 = ephemeral_vrf_sdk::rnd::random_u32(&randomness); | |
| let range = (reward_list.global_range_max as u64) | |
| .checked_sub(reward_list.global_range_min as u64) | |
| .unwrap() | |
| 1; | |
| let offset = ((rnd_u32 as u64) % range) as u32; | |
| let result = reward_list.global_range_min + offset; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@rewards-delegated-vrf-1.0/programs/rewards-delegated-vrf/src/instructions/consume_random_reward.rs`
around lines 30 - 35, The random-range arithmetic must be done in u64 to avoid a
zero modulus when the range equals 2^32; change the computation so you cast the
RNG output (ephemeral_vrf_sdk::rnd::random_u32) to u64, compute range as u64
using reward_list.global_range_max and global_range_min, do the modulus with
that u64 range (e.g., (rnd_u32 as u64) % range), then add global_range_min as
u64 and only cast the final result back to u32 if the surrounding API requires
it; update the code around rnd::random_u32, range, and result to use u64 math to
prevent rnd % 0 panics.
Comment on lines
+622
to
+675
| const txHash = await providerEphemeralRollup | ||
| .sendAndConfirm(tx, [wallet.payer], { skipPreflight: true }) | ||
| .catch((err) => { | ||
| console.log( | ||
| "Request Random Reward error (may fail if VRF not available):", | ||
| err.message | ||
| ); | ||
| return null; | ||
| }); | ||
|
|
||
| if (txHash) { | ||
| console.log("Request Random Reward txHash: ", txHash); | ||
| // Transaction succeeded, VRF callback will come asynchronously | ||
| let listener: number | null = null; | ||
| let listenerRemoved = false; | ||
| const callbackReceived = new Promise<void>((resolve) => { | ||
| const timeoutId = setTimeout(() => { | ||
| if (listener !== null && !listenerRemoved) { | ||
| ephemeralProgram.provider.connection.removeOnLogsListener(listener); | ||
| listenerRemoved = true; | ||
| } | ||
| console.log("VRF callback listener timeout (callback may still come)"); | ||
| resolve(); | ||
| }, 30000); // 30 second timeout | ||
|
|
||
| listener = ephemeralProgram.provider.connection.onLogs( | ||
| program.programId, | ||
| (logs) => { | ||
| try { | ||
| console.log("Program logs received:", logs.logs); | ||
| console.log("VRF callback signature:", logs.signature); | ||
| console.log("VRF callback status:", logs.err ? "Error" : "Success"); | ||
| console.log("VRF callback logs:"); | ||
| const relevantLogs = logs.logs.filter( | ||
| (log) => log.includes("Random result:") || log.includes("Won reward") || log.includes("exhausted") | ||
| ); | ||
| relevantLogs.forEach((log) => console.log(" " + log)); | ||
| if (listener !== null && !listenerRemoved) { | ||
| ephemeralProgram.provider.connection.removeOnLogsListener(listener); | ||
| listenerRemoved = true; | ||
| } | ||
| clearTimeout(timeoutId); | ||
| resolve(); | ||
| } catch (err) { | ||
| console.error("Error in log listener:", err); | ||
| } | ||
| }, | ||
| "confirmed" | ||
| ); | ||
| }); | ||
|
|
||
| // Wait for the callback (with timeout) | ||
| await callbackReceived; | ||
| } |
Contributor
There was a problem hiding this comment.
This test passes even when the delegated VRF flow is broken.
Line 624 converts request failures into null, and Lines 638-645 resolve the callback wait on timeout without any assertion. That means the main backward-compat path can fail and this spec still succeeds.
Suggested fix
- const txHash = await providerEphemeralRollup
+ const txHash = await providerEphemeralRollup
.sendAndConfirm(tx, [wallet.payer], { skipPreflight: true })
- .catch((err) => {
- console.log(
- "Request Random Reward error (may fail if VRF not available):",
- err.message
- );
- return null;
- });
+ .catch((err) => {
+ throw new Error(`Request Random Reward failed: ${err.message}`);
+ });
- if (txHash) {
- console.log("Request Random Reward txHash: ", txHash);
+ console.log("Request Random Reward txHash: ", txHash);
// Transaction succeeded, VRF callback will come asynchronously
let listener: number | null = null;
let listenerRemoved = false;
const callbackReceived = new Promise<void>((resolve, reject) => {
const timeoutId = setTimeout(() => {
if (listener !== null && !listenerRemoved) {
ephemeralProgram.provider.connection.removeOnLogsListener(listener);
listenerRemoved = true;
}
- console.log("VRF callback listener timeout (callback may still come)");
- resolve();
+ reject(new Error("Timed out waiting for VRF callback"));
}, 30000);
@@
- await callbackReceived;
- }
+ await callbackReceived;📝 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
| const txHash = await providerEphemeralRollup | |
| .sendAndConfirm(tx, [wallet.payer], { skipPreflight: true }) | |
| .catch((err) => { | |
| console.log( | |
| "Request Random Reward error (may fail if VRF not available):", | |
| err.message | |
| ); | |
| return null; | |
| }); | |
| if (txHash) { | |
| console.log("Request Random Reward txHash: ", txHash); | |
| // Transaction succeeded, VRF callback will come asynchronously | |
| let listener: number | null = null; | |
| let listenerRemoved = false; | |
| const callbackReceived = new Promise<void>((resolve) => { | |
| const timeoutId = setTimeout(() => { | |
| if (listener !== null && !listenerRemoved) { | |
| ephemeralProgram.provider.connection.removeOnLogsListener(listener); | |
| listenerRemoved = true; | |
| } | |
| console.log("VRF callback listener timeout (callback may still come)"); | |
| resolve(); | |
| }, 30000); // 30 second timeout | |
| listener = ephemeralProgram.provider.connection.onLogs( | |
| program.programId, | |
| (logs) => { | |
| try { | |
| console.log("Program logs received:", logs.logs); | |
| console.log("VRF callback signature:", logs.signature); | |
| console.log("VRF callback status:", logs.err ? "Error" : "Success"); | |
| console.log("VRF callback logs:"); | |
| const relevantLogs = logs.logs.filter( | |
| (log) => log.includes("Random result:") || log.includes("Won reward") || log.includes("exhausted") | |
| ); | |
| relevantLogs.forEach((log) => console.log(" " + log)); | |
| if (listener !== null && !listenerRemoved) { | |
| ephemeralProgram.provider.connection.removeOnLogsListener(listener); | |
| listenerRemoved = true; | |
| } | |
| clearTimeout(timeoutId); | |
| resolve(); | |
| } catch (err) { | |
| console.error("Error in log listener:", err); | |
| } | |
| }, | |
| "confirmed" | |
| ); | |
| }); | |
| // Wait for the callback (with timeout) | |
| await callbackReceived; | |
| } | |
| const txHash = await providerEphemeralRollup | |
| .sendAndConfirm(tx, [wallet.payer], { skipPreflight: true }) | |
| .catch((err) => { | |
| throw new Error(`Request Random Reward failed: ${err.message}`); | |
| }); | |
| console.log("Request Random Reward txHash: ", txHash); | |
| // Transaction succeeded, VRF callback will come asynchronously | |
| let listener: number | null = null; | |
| let listenerRemoved = false; | |
| const callbackReceived = new Promise<void>((resolve, reject) => { | |
| const timeoutId = setTimeout(() => { | |
| if (listener !== null && !listenerRemoved) { | |
| ephemeralProgram.provider.connection.removeOnLogsListener(listener); | |
| listenerRemoved = true; | |
| } | |
| reject(new Error("Timed out waiting for VRF callback")); | |
| }, 30000); // 30 second timeout | |
| listener = ephemeralProgram.provider.connection.onLogs( | |
| program.programId, | |
| (logs) => { | |
| try { | |
| console.log("Program logs received:", logs.logs); | |
| console.log("VRF callback signature:", logs.signature); | |
| console.log("VRF callback status:", logs.err ? "Error" : "Success"); | |
| console.log("VRF callback logs:"); | |
| const relevantLogs = logs.logs.filter( | |
| (log) => log.includes("Random result:") || log.includes("Won reward") || log.includes("exhausted") | |
| ); | |
| relevantLogs.forEach((log) => console.log(" " + log)); | |
| if (listener !== null && !listenerRemoved) { | |
| ephemeralProgram.provider.connection.removeOnLogsListener(listener); | |
| listenerRemoved = true; | |
| } | |
| clearTimeout(timeoutId); | |
| resolve(); | |
| } catch (err) { | |
| console.error("Error in log listener:", err); | |
| } | |
| }, | |
| "confirmed" | |
| ); | |
| }); | |
| // Wait for the callback (with timeout) | |
| await callbackReceived; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rewards-delegated-vrf-1.0/tests/rewards-delegated-vrf.ts` around lines 622 -
675, The test currently swallows send errors and treats a timed-out VRF callback
as success; change the sendAndConfirm error handler on
providerEphemeralRollup.sendAndConfirm so it does not return null but instead
fails the test (rethrow or assert) when the transaction cannot be sent, and
modify the callback promise logic around callbackReceived/listener so a boolean
flag (e.g., callbackSeen) is set to true inside the onLogs handler when a valid
VRF callback is observed (check relevantLogs or logs.err), and replace the
timeout resolve with a reject (or assert failure) if callbackSeen is false after
30s; ensure you reference txHash, callbackReceived, listener/listenerRemoved,
and ephemeralProgram.provider.connection.onLogs when making these changes so the
test fails when VRF flow is broken.
Comment on lines
+77
to
+84
| // Log final user balance | ||
| const finalBalance = await provider.connection.getBalance(user.publicKey); | ||
| console.log("User balance:", finalBalance / LAMPORTS_PER_SOL, "SOL\n"); | ||
|
|
||
| if (finalBalance === 0) { | ||
| console.log("WARNING: User has 0 SOL. Tests may fail. Fund the account manually:"); | ||
| console.log(` solana airdrop 2 ${user.publicKey.toString()}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the final balance probe non-fatal.
The helper catches RPC failures above, but Line 78 does another getBalance outside any try block. A flaky or unsupported RPC can still throw here after you've already taken the fallback path.
Suggested fix
- const finalBalance = await provider.connection.getBalance(user.publicKey);
- console.log("User balance:", finalBalance / LAMPORTS_PER_SOL, "SOL\n");
-
- if (finalBalance === 0) {
- console.log("WARNING: User has 0 SOL. Tests may fail. Fund the account manually:");
- console.log(` solana airdrop 2 ${user.publicKey.toString()}`);
- }
+ try {
+ const finalBalance = await provider.connection.getBalance(user.publicKey);
+ console.log("User balance:", finalBalance / LAMPORTS_PER_SOL, "SOL\n");
+
+ if (finalBalance === 0) {
+ console.log("WARNING: User has 0 SOL. Tests may fail. Fund the account manually:");
+ console.log(` solana airdrop 2 ${user.publicKey.toString()}`);
+ }
+ } catch (e) {
+ console.log("Final balance check failed:", e instanceof Error ? e.message : e);
+ }📝 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
| // Log final user balance | |
| const finalBalance = await provider.connection.getBalance(user.publicKey); | |
| console.log("User balance:", finalBalance / LAMPORTS_PER_SOL, "SOL\n"); | |
| if (finalBalance === 0) { | |
| console.log("WARNING: User has 0 SOL. Tests may fail. Fund the account manually:"); | |
| console.log(` solana airdrop 2 ${user.publicKey.toString()}`); | |
| } | |
| // Log final user balance | |
| try { | |
| const finalBalance = await provider.connection.getBalance(user.publicKey); | |
| console.log("User balance:", finalBalance / LAMPORTS_PER_SOL, "SOL\n"); | |
| if (finalBalance === 0) { | |
| console.log("WARNING: User has 0 SOL. Tests may fail. Fund the account manually:"); | |
| console.log(` solana airdrop 2 ${user.publicKey.toString()}`); | |
| } | |
| } catch (e) { | |
| console.log("Final balance check failed:", e instanceof Error ? e.message : e); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rewards-delegated-vrf-1.0/tests/setup.ts` around lines 77 - 84, The final
balance probe uses provider.connection.getBalance outside any try/catch so a
flaky RPC can throw; wrap the call that sets finalBalance and the subsequent
console.logs in a try/catch (targeting the provider.connection.getBalance call
and usage of finalBalance / LAMPORTS_PER_SOL and user.publicKey) and on error
log a non-fatal warning like "Could not fetch final balance" (including the
caught error) and continue without throwing; preserve the existing zero-balance
warning when finalBalance is obtained successfully.
Comment on lines
+11
to
+15
| impl anchor_lang::Id for VrfProgram { | ||
| fn id() -> anchor_lang::prelude::Pubkey { | ||
| anchor_lang::prelude::Pubkey::new_from_array(crate::consts::VRF_PROGRAM_ID.to_bytes()) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Cargo.toml(s) for solana_program/anchor-lang version pins around the new program/SDK.
fd -t f Cargo.toml | xargs rg -nP '^(solana-program|anchor-lang|ephemeral-vrf-sdk)\s*=' -C1Repository: magicblock-labs/magicblock-engine-examples
Length of output: 7759
Simplify: remove the unnecessary to_bytes()/new_from_array round-trip.
Both the vendored SDK and consuming program use identical versions: anchor-lang 1.0.0 and solana-program 3.0.0. In this setup, anchor_lang::prelude::Pubkey is a direct re-export of solana_program::pubkey::Pubkey, so the byte conversion is a no-op. Simplify to crate::consts::VRF_PROGRAM_ID.
♻️ Proposed simplification
impl anchor_lang::Id for VrfProgram {
fn id() -> anchor_lang::prelude::Pubkey {
- anchor_lang::prelude::Pubkey::new_from_array(crate::consts::VRF_PROGRAM_ID.to_bytes())
+ crate::consts::VRF_PROGRAM_ID
}
}📝 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
| impl anchor_lang::Id for VrfProgram { | |
| fn id() -> anchor_lang::prelude::Pubkey { | |
| anchor_lang::prelude::Pubkey::new_from_array(crate::consts::VRF_PROGRAM_ID.to_bytes()) | |
| } | |
| } | |
| impl anchor_lang::Id for VrfProgram { | |
| fn id() -> anchor_lang::prelude::Pubkey { | |
| crate::consts::VRF_PROGRAM_ID | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/anchor.rs` around
lines 11 - 15, The impl of anchor_lang::Id for VrfProgram unnecessarily converts
VRF_PROGRAM_ID to bytes then back to a Pubkey; update the id() implementation in
the VrfProgram impl to return crate::consts::VRF_PROGRAM_ID directly (replace
the Pubkey::new_from_array(crate::consts::VRF_PROGRAM_ID.to_bytes()) expression
with crate::consts::VRF_PROGRAM_ID) since anchor_lang::prelude::Pubkey is the
same type here.
Comment on lines
+31
to
+43
| pub fn random_u8_with_range(bytes: &[u8; 32], min_value: u8, max_value: u8) -> u8 { | ||
| let range = (max_value - min_value + 1) as u16; | ||
| let threshold = (256 / range * range) as u8; | ||
|
|
||
| // Try to find a byte that, when mapped, gives an unbiased result | ||
| for &b in bytes.iter().rev() { | ||
| if b < threshold { | ||
| return min_value + (b % range as u8); | ||
| } | ||
| } | ||
| // Fallback (slight bias, but rare fallback case) | ||
| min_value + (bytes[31] % range as u8) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard invalid/full-range inputs to prevent modulo-by-zero panic
random_u8_with_range can panic at runtime: on Line 38/Line 42, range as u8 becomes 0 when min_value=0 and max_value=255 (range=256), so % 0 occurs. Also, max_value < min_value is unchecked.
Suggested fix
pub fn random_u8_with_range(bytes: &[u8; 32], min_value: u8, max_value: u8) -> u8 {
- let range = (max_value - min_value + 1) as u16;
- let threshold = (256 / range * range) as u8;
+ assert!(min_value <= max_value, "min_value must be <= max_value");
+ let range = (max_value as u16) - (min_value as u16) + 1;
+ if range == 256 {
+ return bytes[31];
+ }
+ let range_u8 = range as u8;
+ let threshold = (256 / range * range) as u16;
// Try to find a byte that, when mapped, gives an unbiased result
for &b in bytes.iter().rev() {
- if b < threshold {
- return min_value + (b % range as u8);
+ if (b as u16) < threshold {
+ return min_value + (b % range_u8);
}
}
// Fallback (slight bias, but rare fallback case)
- min_value + (bytes[31] % range as u8)
+ min_value + (bytes[31] % range_u8)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/src/rnd.rs` around lines
31 - 43, random_u8_with_range can divide/modulo by zero when range == 256 and
also accepts invalid ranges where max_value < min_value; fix by validating
inputs at the top of random_u8_with_range: if max_value < min_value return/err
(or swap) to reject invalid ranges, and if (max_value - min_value + 1) == 256
(i.e., full byte range) short-circuit and return a raw byte from bytes (e.g.,
bytes[31]) without doing `% range as u8`; keep the existing unbiased loop and
fallback logic for other ranges but avoid casting range to u8 when range == 256.
Comment on lines
+87
to
+102
| impl<'info> #struct_name<'info> { | ||
| fn invoke_signed_vrf<'a>(&self, payer: &'a AccountInfo<'info>, ix: &anchor_lang::solana_program::instruction::Instruction) -> anchor_lang::solana_program::entrypoint::ProgramResult { | ||
| let bump = Pubkey::try_find_program_address(&[ephemeral_vrf_sdk::consts::IDENTITY], &crate::ID).ok_or(anchor_lang::prelude::ProgramError::InvalidSeeds)?; | ||
| anchor_lang::solana_program::program::invoke_signed( | ||
| ix, | ||
| &[ | ||
| payer.clone(), | ||
| self.program_identity.to_account_info(), | ||
| self.oracle_queue.to_account_info(), | ||
| self.system_program.to_account_info(), | ||
| self.slot_hashes.to_account_info(), | ||
| ], | ||
| &[&[ephemeral_vrf_sdk::consts::IDENTITY, &[bump.1]]], | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every consumer of the #[vrf] attribute in this PR and confirm each declares `oracle_queue`.
rg -nP '#\[vrf\]' -C 30 | rg -nP '#\[vrf\]|oracle_queue|pub struct'Repository: magicblock-labs/magicblock-engine-examples
Length of output: 2685
🏁 Script executed:
cat -n rewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/vrf-macro/src/lib.rs | head -100Repository: magicblock-labs/magicblock-engine-examples
Length of output: 4267
Macro requires undeclared oracle_queue field with no validation or auto-injection.
Line 95 references self.oracle_queue.to_account_info() unconditionally, but the macro neither auto-injects nor validates this field. Unlike the other four required accounts (program_identity, vrf_program, slot_hashes, system_program), which are tracked (lines 13–16), validated (lines 40–51), and auto-injected (lines 54–78), oracle_queue has no such safeguards.
If a downstream struct omits oracle_queue, the error will be error[E0609]: no field 'oracle_queue' on type ... pointing into macro-generated code—difficult to diagnose. Add an early validation check mirroring the existing unnamed-fields enforcement:
Proposed: detect missing `oracle_queue` at macro-expansion time
let mut has_program_identity = false;
let mut has_slot_hashes = false;
let mut has_vrf_program = false;
let mut has_system_program = false;
+ let mut has_oracle_queue = false;
for field in fields.iter() {
...
if field_name.eq("system_program") {
has_system_program = true;
}
+ if field_name.eq("oracle_queue") {
+ has_oracle_queue = true;
+ }
}
+
+ if !has_oracle_queue {
+ return syn::Error::new_spanned(
+ &input.ident,
+ "structs annotated with #[vrf] must declare an `oracle_queue` field; \
+ it is referenced by the generated `invoke_signed_vrf` method",
+ )
+ .to_compile_error()
+ .into();
+ }Alternatively, auto-inject oracle_queue like the other four fields, though the correct account constraints (mut/readonly, PDA seeds if applicable) may depend on your queue layout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rewards-delegated-vrf-1.0/vendor/ephemeral-vrf-sdk/vrf-macro/src/lib.rs`
around lines 87 - 102, The macro-generated method invoke_signed_vrf references
self.oracle_queue but the macro doesn't validate or auto-inject an oracle_queue
field; update the macro to detect a missing oracle_queue at expansion time (same
style as the existing unnamed-fields enforcement/validation used for
program_identity, vrf_program, slot_hashes, system_program) and either (a) emit
a clear compile-time error when oracle_queue is not declared on the target
struct, or (b) auto-inject an oracle_queue account mapping like the other four
accounts (ensuring the correct mutability/readonly and PDA seeds if needed);
ensure the check/auto-injection logic is added where the macro collects and
validates required accounts so invoke_signed_vrf can safely call
self.oracle_queue.to_account_info().
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.
Summary by CodeRabbit