Conversation
|
I wonder why the @claude PR review action did not run here. |
There was a problem hiding this comment.
Pull request overview
Adds a new Solana “deposit-address” Anchor program that derives deterministic deposit PDAs and provides permissionless sweep instructions to forward funds into the relay depository, plus an owner-only whitelisted CPI “execute” escape hatch.
Changes:
- Introduces
deposit-addresson-chain program (config, whitelist management, sweep native/token, execute CPI) and supporting docs. - Adds a comprehensive Mocha/Chai test suite for deposit addresses and adjusts existing
relay-depositorytests for shared initialization. - Updates localnet program IDs/configuration (Anchor.toml) and adds documentation for architecture + doc generation.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/solana-vm/tests/relay-depository.ts | Adds “already initialized” guards to reduce inter-test dependency failures. |
| packages/solana-vm/tests/deposit-address.ts | New test suite covering deposit-address admin, whitelist, sweep, and execute flows. |
| packages/solana-vm/programs/relay-forwarder/src/lib.rs | Updates program ID used for localnet. |
| packages/solana-vm/programs/relay-depository/src/lib.rs | Updates program ID used for localnet. |
| packages/solana-vm/programs/deposit-address/src/lib.rs | New Anchor program implementing deterministic deposit PDAs + sweep/execute instructions. |
| packages/solana-vm/programs/deposit-address/Cargo.toml | New crate manifest for deposit-address program. |
| packages/solana-vm/programs/deposit-address/Xargo.toml | Adds Xargo config for the program build. |
| packages/solana-vm/docs/solana-deposit-address-plan.md | New planning/architecture document for the deposit-address program. |
| packages/solana-vm/README.md | Adds doc generation commands. |
| packages/solana-vm/Anchor.toml | Registers deposit_address and updates localnet IDs + test script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const owner = Keypair.fromSecretKey( | ||
| Buffer.from( | ||
| "5223911e0fbfb0b8d5880ebea5711d5d7754387950c08b52c0eaf127facebd455e28ef570e8aed9ecef8a89f5c1a90739080c05df9e9c8ca082376ef93a02b2e", | ||
| "hex" | ||
| ) | ||
| ); |
There was a problem hiding this comment.
A raw private key is committed into the repo as a hex literal. Even if intended for localnet only, this is risky and easy to accidentally reuse elsewhere. Prefer sourcing the key from a non-committed fixture (e.g. environment variable/Anchor wallet file loaded in CI) or generate a deterministic test keypair from a seed in code (not a real secret key), and ensure the on-chain AUTHORIZED_PUBKEY expectation is compatible with that approach.
| const owner = Keypair.fromSecretKey( | |
| Buffer.from( | |
| "5223911e0fbfb0b8d5880ebea5711d5d7754387950c08b52c0eaf127facebd455e28ef570e8aed9ecef8a89f5c1a90739080c05df9e9c8ca082376ef93a02b2e", | |
| "hex" | |
| ) | |
| ); | |
| const OWNER_SECRET_KEY_HEX = process.env.DEPOSIT_ADDRESS_TEST_SECRET_KEY; | |
| if (!OWNER_SECRET_KEY_HEX) { | |
| throw new Error( | |
| "Environment variable DEPOSIT_ADDRESS_TEST_SECRET_KEY must be set to the hex-encoded secret key for the authorized owner used in tests." | |
| ); | |
| } | |
| const owner = Keypair.fromSecretKey(Buffer.from(OWNER_SECRET_KEY_HEX, "hex")); |
|
|
||
| # Generate json docs | ||
| RUSTDOCFLAGS="-Z unstable-options --output-format json" \ | ||
| cargo doc --no-deps \ |
There was a problem hiding this comment.
The trailing \\ after cargo doc --no-deps makes the command invalid as written (it line-continues into a blank line). Remove the trailing backslash or add the intended continuation line so readers can copy/paste the command successfully.
| cargo doc --no-deps \ | |
| cargo doc --no-deps |
| // Only the deposit_address PDA is marked as signer (signed via invoke_signed) | ||
| let deposit_address_key = ctx.accounts.deposit_address.key(); | ||
| let account_metas: Vec<AccountMeta> = ctx | ||
| .remaining_accounts | ||
| .iter() | ||
| .map(|account| { | ||
| let is_signer = account.key() == deposit_address_key; | ||
| if account.is_writable { | ||
| AccountMeta::new(*account.key, is_signer) | ||
| } else { | ||
| AccountMeta::new_readonly(*account.key, is_signer) | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| let instruction = Instruction { | ||
| program_id: ctx.accounts.target_program.key(), | ||
| accounts: account_metas, | ||
| data: instruction_data.clone(), | ||
| }; | ||
|
|
||
| // Collect account infos for invoke_signed | ||
| let mut account_infos: Vec<AccountInfo<'info>> = ctx | ||
| .remaining_accounts | ||
| .iter() | ||
| .map(|a| a.to_account_info()) | ||
| .collect(); |
There was a problem hiding this comment.
execute currently relies on the caller to include the deposit_address in remaining_accounts (otherwise it won’t be present in the CPI instruction’s metas, and won’t be marked signer). If the intent is ‘execute from the deposit address’, consider enforcing that deposit_address is included (and exactly once), or automatically prepending it to account_metas/account_infos so the API is harder to misuse and behavior is consistent.
| // Only the deposit_address PDA is marked as signer (signed via invoke_signed) | |
| let deposit_address_key = ctx.accounts.deposit_address.key(); | |
| let account_metas: Vec<AccountMeta> = ctx | |
| .remaining_accounts | |
| .iter() | |
| .map(|account| { | |
| let is_signer = account.key() == deposit_address_key; | |
| if account.is_writable { | |
| AccountMeta::new(*account.key, is_signer) | |
| } else { | |
| AccountMeta::new_readonly(*account.key, is_signer) | |
| } | |
| }) | |
| .collect(); | |
| let instruction = Instruction { | |
| program_id: ctx.accounts.target_program.key(), | |
| accounts: account_metas, | |
| data: instruction_data.clone(), | |
| }; | |
| // Collect account infos for invoke_signed | |
| let mut account_infos: Vec<AccountInfo<'info>> = ctx | |
| .remaining_accounts | |
| .iter() | |
| .map(|a| a.to_account_info()) | |
| .collect(); | |
| // Ensure the deposit_address PDA is always included and marked as signer | |
| let deposit_address_info = ctx.accounts.deposit_address.to_account_info(); | |
| let deposit_address_key = deposit_address_info.key(); | |
| // Check whether deposit_address is already present in remaining accounts | |
| let deposit_in_remaining = ctx | |
| .remaining_accounts | |
| .iter() | |
| .any(|account| account.key() == deposit_address_key); | |
| // Start with capacity for all remaining accounts plus optional deposit_address | |
| let mut account_metas: Vec<AccountMeta> = | |
| Vec::with_capacity(ctx.remaining_accounts.len() + (!deposit_in_remaining as usize)); | |
| // If deposit_address is not provided by the caller, add it as a readonly signer | |
| if !deposit_in_remaining { | |
| account_metas.push(AccountMeta::new_readonly(deposit_address_key, true)); | |
| } | |
| // Map remaining accounts, marking deposit_address (if present) as signer | |
| account_metas.extend( | |
| ctx.remaining_accounts | |
| .iter() | |
| .map(|account| { | |
| let is_signer = account.key() == deposit_address_key; | |
| if account.is_writable { | |
| AccountMeta::new(*account.key, is_signer) | |
| } else { | |
| AccountMeta::new_readonly(*account.key, is_signer) | |
| } | |
| }), | |
| ); | |
| let instruction = Instruction { | |
| program_id: ctx.accounts.target_program.key(), | |
| accounts: account_metas, | |
| data: instruction_data.clone(), | |
| }; | |
| // Collect account infos for invoke_signed, ensuring deposit_address is included | |
| let mut account_infos: Vec<AccountInfo<'info>> = | |
| Vec::with_capacity(ctx.remaining_accounts.len() + 2); | |
| if !deposit_in_remaining { | |
| account_infos.push(deposit_address_info.clone()); | |
| } | |
| account_infos.extend( | |
| ctx.remaining_accounts | |
| .iter() | |
| .map(|a| a.to_account_info()), | |
| ); |
| /// Validates target_program is in the whitelist | ||
| #[account( | ||
| seeds = [ALLOWED_PROGRAM_SEED, target_program.key().as_ref()], | ||
| bump, | ||
| )] | ||
| pub allowed_program: Account<'info, AllowedProgram>, | ||
|
|
||
| /// CHECK: Target program for CPI, validated via allowed_program PDA and executable constraint | ||
| #[account(executable)] | ||
| pub target_program: UncheckedAccount<'info>, |
There was a problem hiding this comment.
The whitelist membership is enforced via PDA address derivation, but there’s no explicit constraint that allowed_program.program_id == target_program.key(). Adding that constraint makes the relationship self-documenting and guards against unexpected state corruption (e.g. if the account data were ever modified incorrectly).
| seeds = ["allowed_program", program_id] | ||
| ``` | ||
|
|
||
| **Why `depositor` is in the PDA seeds:** The deposit address contract cannot know who transferred funds into the PDA on-chain. But the depository contract requires the `depositor` when depositing. By including `depositor` in the PDA seeds, Anchor's seed validation enforces that the correct depositor is provided during sweep. Without this, the permissionless sweep caller could pass an arbitrary depositor. |
There was a problem hiding this comment.
Ha! Amazing. I like this approach. I wonder though if the solver "knows" the depositor's address when calling sweep_native or sweep_token?
There was a problem hiding this comment.
in the solver side, we can userequest.data.user or request.data.refundTo
| | `add_allowed_program` | owner | Add program to execute whitelist | | ||
| | `remove_allowed_program` | owner | Remove program from execute whitelist | | ||
| | `sweep_native` | permissionless | Sweep full SOL balance from deposit PDA to vault via CPI | | ||
| | `sweep_token` | permissionless | Sweep token balance from deposit PDA to vault via CPI, close ATA | |
There was a problem hiding this comment.
Any reason we need 2 functions? In the EVM version I think we will just have a single function and we will pass the token address for ERC20 or the 0x0 address for native.
There was a problem hiding this comment.
merged to the same sweep now
There was a problem hiding this comment.
It's important to update the plan first here @lljxx1 :)
There was a problem hiding this comment.
julien51
left a comment
There was a problem hiding this comment.
Plan needs a little bit of change please :) Ideally we only ask Claude to implement when the plan has been merged. I think we can even go as far as trying to automate the implementation via Github actions If a new plan is added.
|
|
||
| ## Key Design Decisions | ||
|
|
||
| 1. **PDA per (orderId, token, depositor)** — deterministic addresses computable off-chain before any deposit |
| 14. Successfully sweep SOL to vault via CPI (+ verify DepositEvent and SweepNativeEvent) | ||
| 15. Fails when balance is 0 | ||
| 16. Different IDs produce different deposit addresses | ||
| 17. Wrong depositor fails PDA seed validation (ConstraintSeeds) |
There was a problem hiding this comment.
Can you add a test where the the address has not been "deployed" and one where it hasbeen deployed already? Also, one where it's been deployed, sweeped once and then there is another deposit?
There was a problem hiding this comment.
25. Native lifecycle: deposit → sweep → deposit again → sweep again (PDA reusable after full drain)
26. Token lifecycle: deposit → sweep (ATA closed) → create ATA → deposit again → sweep again
More tests added! Solana PDA has no difference deployed and un-deployed. see https://github.com/relayprotocol/relay-depository/pull/38/changes#diff-4583b6c97405872b37a3254e757cd0e085c3d56f9eae83a9138581399dd4a81eR175
Address PR review feedback: - Merge sweep_native and sweep_token into unified sweep(id, mint) where mint=Pubkey::default() for native SOL (matching EVM address(0) pattern) - Token-specific accounts use Option<> following ExecuteTransfer pattern - Add MissingTokenAccounts error for token sweep without optional accounts - Add allowed_program.program_id == target_program.key() defense-in-depth - Add lifecycle tests (deposit→sweep→deposit→sweep) for native and token - Fix relay-depository tests to use this.skip() instead of return - Update plan with merged design, security checklist, and rustdoc standard Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Maybe it's a different repo than |
Summary
deposit-addressprogram that creates deterministic deposit addresses (PDAs) per order ID, token, and depositor— addresses are computable off-chain before any on-chain transaction
sweep_native/sweep_tokeninstructions forward deposited funds to the relay depository vault viaCPI, with depositor enforced through PDA seed validation
executeinstruction enables arbitrary CPI from deposit addresses for edge cases (stuck funds, airdrops),restricted to a dynamic program whitelist
set_owner,set_depository, and whitelist management (add/remove_allowed_program) for admin operationssolana-deposit-address-plan.mdHow to review
Start with the plan document for architecture context, then review
programs/deposit-address/src/lib.rs(single-fileAnchor program). Focus on PDA seed design (depositor inclusion), CPI safety in sweep/execute, access control
boundaries, and
has_oneconstraint validation.🤖 Generated with Claude Code