Feat/153 chunked proof upload#156
Conversation
ChunkOutOfBounds, PayloadAlreadyFinalized, PayloadNotReady, ProofIncomplete — used by the init/write/finalize instruction flow for multi-tx proof delivery. Refs #153
Three new fields support the chunked proof upload flow: expected_proof_len declares total size at init, high_water_mark tracks the furthest byte written, finalized gates settlement. Refs #153
…ctions Three new instructions implement chunked proof upload: init allocates the PDA with expected_proof_len, write copies chunks with bounds and high-water-mark tracking, finalize sets metadata and flips the finalized flag. Existing set_hook_payload sets finalized=true for backward compatibility. Settlement now requires finalized=true. Refs #153
Cover finalize input validation (zero nullifier, zero amount) and high-water-mark tracking (sequential writes, overwrites, gaps, partial-write-blocks-finalize). Refs #153
Exports buildInitHookPayloadIx, buildWriteChunkIx, buildFinalizeHookPayloadIx instruction builders plus an uploadProofChunked orchestrator that batches 2 writes per tx. CHUNK_SIZE=900 keeps each tx within Solana's 1,232-byte limit. Refs #153
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR introduces a multi-step hook payload lifecycle for transfer hooks, replacing a single consolidated handler with three sequential steps: initialization, incremental proof writing, and finalization. Backend error variants and data structures expand to track proof state. The SDK gains chunked upload utilities and instruction builders to support this workflow. ChangesChunked Proof Upload Lifecycle
Sequence DiagramsequenceDiagram
actor Client as Client/SDK
participant Init as init_hook_payload<br/>(Handler)
participant Write as write_hook_proof<br/>(Handler)
participant Finalize as finalize_hook_payload<br/>(Handler)
participant Settle as Settlement Flow
Client->>Init: Init w/ expected_proof_len
activate Init
Init->>Init: Allocate proof buffer,<br/>set high_water_mark=0,<br/>finalized=false
Init-->>Client: InitSignature
deactivate Init
loop For each chunk (1..N)
Client->>Write: Write chunk @ offset
activate Write
Write->>Write: Bounds check,<br/>append to proof_and_witness,<br/>update high_water_mark
Write-->>Client: ChunkSignature
deactivate Write
end
Client->>Finalize: Finalize w/ metadata
activate Finalize
Finalize->>Finalize: Verify complete proof<br/>(high_water_mark ==<br/>expected_proof_len)
Finalize->>Finalize: Set nullifier_hash, mint,<br/>recipient, amount,<br/>epoch, light_args,<br/>finalized=true
Finalize-->>Client: FinalizeSignature
deactivate Finalize
Client->>Settle: Execute settlement
activate Settle
Settle->>Settle: Check: payload.finalized?
alt Finalized
Settle->>Settle: Process settlement
Settle-->>Client: Success
else Not Finalized
Settle-->>Client: PayloadNotReady Error
end
deactivate Settle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 6
🤖 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 `@backend/programs/zksettle/src/instructions/transfer_hook/handlers.rs`:
- Around line 82-102: The handler write_hook_proof_handler currently lets
clients write out-of-order chunks because it sets payload.high_water_mark =
max(...), so finalize can be bypassed; enforce sequential writes by requiring
the incoming offset equals the current payload.high_water_mark before copying
(return an error like ZkSettleError::ChunkOutOfOrder if not), then perform the
copy and set payload.high_water_mark = end_u32 (no max), which ensures
contiguous writes; update any callers/tests that assume out-of-order or
overlapping writes accordingly or implement a bitmap/contiguous-range tracker if
parallel/non-sequential uploads are needed.
In `@backend/programs/zksettle/src/instructions/transfer_hook/types.rs`:
- Around line 72-74: The finalize logic is wrong: checking only high_water_mark
== expected_proof_len allows gaps because write() may update high_water_mark
from non-sequential writes; modify the implementation so finalize verifies full
coverage instead of just the max end offset—either enforce sequential writes in
the write handler (reject writes whose start != current high_water_mark) or
maintain written-coverage (e.g., a bitmap/interval set) and have finalize check
that coverage spans [0, expected_proof_len); update references to
expected_proof_len, high_water_mark, finalized, the finalize handler and the
write handler (and adjust the gap_write_inflates_water_mark test accordingly) so
finalize only succeeds when every byte in [0, expected_proof_len) has been
written.
In `@sdk/src/wrap/index.ts`:
- Around line 162-178: The loop stride uses chunkSize derived from
opts.chunkSize without validation, which can produce non-terminating or
incorrect loops; validate and normalize chunkSize before the loop (e.g., read
opts.chunkSize into chunkSize, if it's undefined use CHUNK_SIZE, otherwise
coerce to a positive integer via Math.floor and ensure > 0), and if invalid
throw or fallback to CHUNK_SIZE; update the code around the chunkSize assignment
(symbols: chunkSize, CHUNK_SIZE, opts.chunkSize, proofBytes.length, and the for
loop using offset += chunkSize) so the for loop always advances by a positive
integer stride.
- Around line 70-85: The program client is being constructed with
makeProgram(connection) which ignores the caller-supplied programId, so builders
like buildInitHookPayloadIx, buildWriteChunkIx, buildFinalizeHookPayloadIx and
uploadProofChunked silently use the IDL's embedded address or hardcoded
ZKSETTLE_PROGRAM_ID; update makeProgram to accept a programId (e.g.,
makeProgram(connection, programId)) and use it when constructing the Anchor
Program, then call makeProgram(connection, programId) from
buildInitHookPayloadIx (and the other builders) and add a programId
parameter/override to uploadProofChunked (or alternatively remove the programId
parameters from the builders if you choose to enforce a single fixed ID); ensure
references to ZKSETTLE_PROGRAM_ID remain only as a default fallback and that
PDAs and Program use the same programId consistently.
- Line 68: The CHUNK_SIZE (export const CHUNK_SIZE) is too large for batched
writeHookProof instructions and causes transactions to exceed Solana's
~1232-byte limit; either reduce CHUNK_SIZE or disable batching. Fix by updating
CHUNK_SIZE to a safe value (e.g., ~600 bytes so that 2 * (16 + CHUNK_SIZE) <
1232, where 16 accounts for the 8-byte discriminator + 4-byte offset + 4-byte
length prefix) or change WRITES_PER_TX to 1 to send one writeHookProof per
transaction; ensure you modify the CHUNK_SIZE constant and/or the WRITES_PER_TX
usage where writeHookProof(offset, chunk) batching is assembled so instruction
sizes no longer exceed the limit.
- Around line 157-218: The uploadProofChunked flow (uploadProofChunked)
currently uses signAndSend which returns only a signature (initSignature /
chunkSignatures / finalizeSignature) and can cause races between
buildInitHookPayloadIx, buildWriteChunkIx and buildFinalizeHookPayloadIx; modify
the function to explicitly wait for on-chain confirmation before moving to the
next phase: after obtaining initSignature call the RPC confirmation (e.g.,
connection.confirmTransaction or equivalent) and only then build and send
writeIxs; after each chunk tx (each sig pushed to chunkSignatures) confirm that
signature before proceeding to the next batch; finally confirm all write
signatures before calling buildFinalizeHookPayloadIx and before sending
finalizeSignature; keep using opts.connection and the existing signAndSend but
add these confirmTransaction waits to ensure correct ordering and avoid changing
external signAndSend type.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5deca45-c98a-4ef9-b358-e85e5da40f07
📒 Files selected for processing (10)
backend/programs/zksettle/src/error.rsbackend/programs/zksettle/src/instructions/transfer_hook/handlers.rsbackend/programs/zksettle/src/instructions/transfer_hook/mod.rsbackend/programs/zksettle/src/instructions/transfer_hook/settlement.rsbackend/programs/zksettle/src/instructions/transfer_hook/tests.rsbackend/programs/zksettle/src/instructions/transfer_hook/types.rsbackend/programs/zksettle/src/lib.rssdk/src/index.tssdk/src/types.tssdk/src/wrap/index.ts
| function makeProgram(connection: Connection): Program { | ||
| const dummyWallet = new Wallet(Keypair.generate()); | ||
| const provider = new AnchorProvider(connection, dummyWallet, {}); | ||
| return new Program(idl as any, provider); | ||
| } | ||
|
|
||
| export async function buildInitHookPayloadIx( | ||
| wallet: PublicKey, | ||
| proofLen: number, | ||
| connection: Connection, | ||
| programId = ZKSETTLE_PROGRAM_ID, | ||
| program?: Program, | ||
| ): Promise<TransactionInstruction> { | ||
| const [issuerPda] = findIssuerPda(wallet, programId); | ||
| const [hookPayloadPda] = findHookPayloadPda(wallet, programId); | ||
| const prog = program ?? makeProgram(connection); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @coral-xyz/anchor0.31.1 TypeScript, when constructingnew Program(idl, provider), is the program ID always taken from the IDL's embedded address/metadata?
💡 Result:
Yes—when constructing a Program client in Anchor TS using the 0.31.x style new Program(idl, provider), the program’s on-chain address is taken from the IDL’s top-level address field (not from metadata.address). This is because the newer Anchor TS API removed the separate programId constructor argument as redundant, since address is now required in the IDL. [1] Evidence: - Anchor’s TypeScript client IDL shape includes address: string as “The on-chain program address.” [2] - The Anchor TS PR notes that removing the programId constructor parameter is based on the fact that the address field is now required, and that the constructor will “use the program id from the IDL.” [1] So, in @coral-xyz/anchor 0.31.1 specifically, new Program(idl, provider) expects the program ID to come from idl.address. Note: Older/legacy IDL variants and conversions discuss metadata.address specifically, but that’s about legacy IDL formats and conversion requirements; the current required field for the TS Program constructor is the top-level address. [3][4]
Citations:
- 1: ts: Remove
programIdparameter of theProgramconstructor otter-sec/anchor#2864 - 2: https://solana-foundation-anchor.mintlify.app/api/typescript/idl
- 3: https://blog.chalda.cz/posts/anchor-idl/
- 4: cli: Add
--program-idoption toidl convertcommand otter-sec/anchor#3309
🏁 Script executed:
# First, let's look at the file to understand its structure and find the functions mentioned
head -n 200 sdk/src/wrap/index.tsRepository: yuribodo/zksettle
Length of output: 5689
🏁 Script executed:
# Search for uploadProofChunked and ZKSETTLE_PROGRAM_ID usage
rg "uploadProofChunked|ZKSETTLE_PROGRAM_ID" sdk/src/wrap/index.ts -A 3 -B 2Repository: yuribodo/zksettle
Length of output: 1460
🏁 Script executed:
# Search for PDA derivation functions (findIssuerPda, findHookPayloadPda) to see how they use programId
rg "findIssuerPda|findHookPayloadPda" sdk/src/ -A 5 -B 2Repository: yuribodo/zksettle
Length of output: 5311
🏁 Script executed:
# Check the function signatures around the mentioned line numbers to understand the full context
sed -n '1,200p' sdk/src/wrap/index.ts | cat -nRepository: yuribodo/zksettle
Length of output: 7089
Thread program ID consistently through chunked upload APIs, or remove the false override parameters.
PDAs are correctly derived from the caller-supplied programId (or default), but the Anchor Program client is constructed from the IDL's embedded address via new Program(idl, provider), which ignores the programId parameter entirely. This means:
buildInitHookPayloadIx,buildWriteChunkIx, andbuildFinalizeHookPayloadIxacceptprogramIdbut silently ignore it when constructing the program viamakeProgram(connection).uploadProofChunkedhardcodesZKSETTLE_PROGRAM_IDwith no override option, so callers cannot target an alternative deployment.
Either pass programId to makeProgram() (and extend it to accept and apply a program ID), ensure the IDL's embedded address matches deployment expectations, or remove the redundant programId parameter from the builders to avoid false configurability.
🤖 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 `@sdk/src/wrap/index.ts` around lines 70 - 85, The program client is being
constructed with makeProgram(connection) which ignores the caller-supplied
programId, so builders like buildInitHookPayloadIx, buildWriteChunkIx,
buildFinalizeHookPayloadIx and uploadProofChunked silently use the IDL's
embedded address or hardcoded ZKSETTLE_PROGRAM_ID; update makeProgram to accept
a programId (e.g., makeProgram(connection, programId)) and use it when
constructing the Anchor Program, then call makeProgram(connection, programId)
from buildInitHookPayloadIx (and the other builders) and add a programId
parameter/override to uploadProofChunked (or alternatively remove the programId
parameters from the builders if you choose to enforce a single fixed ID); ensure
references to ZKSETTLE_PROGRAM_ID remain only as a default fallback and that
PDAs and Program use the same programId consistently.
| export async function uploadProofChunked( | ||
| opts: ChunkedUploadOptions, | ||
| signAndSend: (tx: Transaction) => Promise<string>, | ||
| ): Promise<ChunkedUploadResult> { | ||
| const programId = ZKSETTLE_PROGRAM_ID; | ||
| const chunkSize = opts.chunkSize ?? CHUNK_SIZE; | ||
| const proofBytes = opts.proof; | ||
| const program = makeProgram(opts.connection); | ||
|
|
||
| const initIx = await buildInitHookPayloadIx( | ||
| opts.wallet, | ||
| proofBytes.length, | ||
| opts.connection, | ||
| programId, | ||
| program, | ||
| ); | ||
| const initSignature = await signAndSend(new Transaction().add(initIx)); | ||
|
|
||
| const writeIxs: TransactionInstruction[] = []; | ||
| for (let offset = 0; offset < proofBytes.length; offset += chunkSize) { | ||
| const end = Math.min(offset + chunkSize, proofBytes.length); | ||
| const chunk = proofBytes.slice(offset, end); | ||
| writeIxs.push( | ||
| await buildWriteChunkIx( | ||
| opts.wallet, | ||
| offset, | ||
| chunk, | ||
| opts.connection, | ||
| programId, | ||
| program, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| const WRITES_PER_TX = 2; | ||
| const chunkSignatures: string[] = []; | ||
| for (let i = 0; i < writeIxs.length; i += WRITES_PER_TX) { | ||
| const tx = new Transaction(); | ||
| for (const ix of writeIxs.slice(i, i + WRITES_PER_TX)) { | ||
| tx.add(ix); | ||
| } | ||
| const sig = await signAndSend(tx); | ||
| chunkSignatures.push(sig); | ||
| } | ||
|
|
||
| const epoch = | ||
| opts.transferContext.epoch ?? Math.floor(Date.now() / 1000 / 86400); | ||
| const finalizeIx = await buildFinalizeHookPayloadIx( | ||
| opts.wallet, | ||
| { | ||
| nullifierHash: opts.nullifierHash, | ||
| mint: opts.transferContext.mint, | ||
| epoch, | ||
| recipient: opts.transferContext.recipient, | ||
| amount: opts.transferContext.amount, | ||
| lightArgs: opts.lightArgs, | ||
| }, | ||
| opts.connection, | ||
| programId, | ||
| program, | ||
| ); | ||
| const finalizeSignature = await signAndSend(new Transaction().add(finalizeIx)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd sdk && find . -name "*.ts" -path "*/wrap/*" | head -20Repository: yuribodo/zksettle
Length of output: 99
🏁 Script executed:
# Let me look at the full context around the function
cd sdk && sed -n '140,230p' src/wrap/index.tsRepository: yuribodo/zksettle
Length of output: 2291
🏁 Script executed:
# Find where signAndSend type is defined
cd sdk && rg -A 5 "signAndSend.*function\|signAndSend.*=>" src/wrap/index.ts | head -40Repository: yuribodo/zksettle
Length of output: 43
🏁 Script executed:
# Look for the type definition of ChunkedUploadOptions
cd sdk && rg -B 5 -A 10 "ChunkedUploadOptions" src/wrap/index.ts | head -60Repository: yuribodo/zksettle
Length of output: 1037
🏁 Script executed:
# Check the build functions to see if they have dependencies on prior phases
cd sdk && rg -A 20 "buildInitHookPayloadIx|buildWriteChunkIx|buildFinalizeHookPayloadIx" src/wrap/index.ts | head -100Repository: yuribodo/zksettle
Length of output: 2841
🏁 Script executed:
cd sdk && rg -B 3 -A 3 "uploadProofChunked" src/ --include="*.ts" | head -80Repository: yuribodo/zksettle
Length of output: 503
🏁 Script executed:
# Check the full ChunkedUploadOptions type definition
cd sdk && rg -B 5 -A 15 "interface ChunkedUploadOptions\|type ChunkedUploadOptions" src/Repository: yuribodo/zksettle
Length of output: 43
🏁 Script executed:
# Look for any documentation or comments around signAndSend
cd sdk && rg -B 10 "signAndSend.*=" src/wrap/index.ts | head -60Repository: yuribodo/zksettle
Length of output: 317
🏁 Script executed:
# Check if there's a pattern elsewhere for confirming transactions
cd sdk && rg "confirmTransaction\|signAndConfirm" src/ --include="*.ts" | head -30Repository: yuribodo/zksettle
Length of output: 503
🏁 Script executed:
cd sdk && rg "uploadProofChunked" src/ -A 3 -B 3 | head -100Repository: yuribodo/zksettle
Length of output: 725
🏁 Script executed:
# Look for ChunkedUploadOptions definition
cd sdk && find . -name "*.ts" -type f | xargs grep -l "ChunkedUploadOptions" | head -5Repository: yuribodo/zksettle
Length of output: 111
🏁 Script executed:
# Check types file
cd sdk && cat src/types.ts | head -150Repository: yuribodo/zksettle
Length of output: 2808
🏁 Script executed:
# Search for confirmTransaction or similar confirmation patterns
cd sdk && rg "confirmTransaction|getSignatureStatus" src/ -A 2 | head -80Repository: yuribodo/zksettle
Length of output: 43
Require confirmation between init, write, and finalize phases.
This workflow is stateful on-chain: initHookPayload creates the hookPayloadPda account, writeHookProof writes to it, and finalizeHookPayload reads from it. However, signAndSend only returns a signature string with no guarantee of on-chain confirmation. A caller can provide a sendTransaction-style function that returns immediately after broadcasting, allowing phase 2 writes and phase 3 finalization to execute before phase 1 is confirmed. This creates race conditions that cause silent failures and flaky behavior under load.
The API should either:
- Change
signAndSendtype to require "send and confirm" semantics, or - Add explicit
confirmTransactioncalls within the helper before proceeding to the next phase.
🤖 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 `@sdk/src/wrap/index.ts` around lines 157 - 218, The uploadProofChunked flow
(uploadProofChunked) currently uses signAndSend which returns only a signature
(initSignature / chunkSignatures / finalizeSignature) and can cause races
between buildInitHookPayloadIx, buildWriteChunkIx and
buildFinalizeHookPayloadIx; modify the function to explicitly wait for on-chain
confirmation before moving to the next phase: after obtaining initSignature call
the RPC confirmation (e.g., connection.confirmTransaction or equivalent) and
only then build and send writeIxs; after each chunk tx (each sig pushed to
chunkSignatures) confirm that signature before proceeding to the next batch;
finally confirm all write signatures before calling buildFinalizeHookPayloadIx
and before sending finalizeSignature; keep using opts.connection and the
existing signAndSend but add these confirmTransaction waits to ensure correct
ordering and avoid changing external signAndSend type.
SetHookPayload was identical to InitHookPayload (both init the PDA). WriteHookProof was identical to FinalizeHookPayload (both mutate non-finalized payload with issuer check). Consolidate into two shared structs: InitHookPayload and ModifyHookPayload.
Reject writes where offset != high_water_mark to prevent gaps that would let finalize pass with unwritten proof regions.
900-byte chunks exceed the ~1232-byte Solana txn limit when batching 2 writeHookProof IXs. Co-locate CHUNK_SIZE and WRITES_PER_TX with sizing rationale. Guard against zero or fractional chunkSize causing infinite loops.
|
1 participant
Summary by CodeRabbit