Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces a complete authentication flow to the SPL API, including challenge and login endpoints. A new auth module provides utilities for token parsing and ephemeral authentication operations. Route definitions are updated to require auth headers for transfer and private-balance endpoints. Auth tokens are threaded through transaction building and balance fetching operations. New challenge and login endpoints are added and integrated into the API router. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/lib/auth.ts`:
- Around line 30-34: getChallenge and login are insecurely proxying to the PER
auth endpoint: stop concatenating strings onto config.ephemeralRpcUrl (use the
URL class or URL constructor to append path/query safely), do not blindly
forward the input.cluster upstream, add explicit headers including
'Content-Type: application/json' for JSON body requests, and always check fetch
response.ok (if false, propagate the upstream status and body instead of
returning HTTP 200). Update the calls in getChallenge and login (and any use of
resolveRpcConfig/ephemeralRpcUrl) to construct URLs safely, set proper headers,
and return upstream response.status and body when !response.ok.
In `@api/src/lib/solana.ts`:
- Around line 181-182: getEphemeralConnection currently concatenates authToken
onto config.ephemeralRpcUrl (causing tokens to be used as cache keys and
corrupting URLs with existing query params); change it to construct the URL via
the URL constructor and URL.searchParams to append token safely, and if
authToken is present do not use the cached getConnection path — instead create
and return an uncached connection (i.e., call the underlying connection creation
routine directly) so the connectionCache is not keyed by auth tokens; reference
getEphemeralConnection, config.ephemeralRpcUrl, authToken, URL.searchParams, and
the connectionCache/getConnection behavior when implementing the fix.
In `@api/src/routes/spl/spl.routes.ts`:
- Around line 196-223: The OpenAPI docs for challengeRoute and loginRoute
currently omit the validation error response branch; update both createRoute
definitions (challengeRoute and loginRoute) to include a 400 response describing
validation errors (e.g., jsonContent(errorResponseSchema, "Validation error") or
a more specific validation schema) so malformed pubkey/challenge/signature
inputs are documented in /doc alongside the existing 200/403/500 responses.
In `@api/src/routes/spl/spl.schemas.ts`:
- Around line 259-266: The authToken should not be part of balanceQuerySchema
(it leaks into GET query strings); remove authToken from balanceQuerySchema and
instead define a dedicated header-or-body schema for the private endpoint (e.g.,
create privateBalanceHeaderSchema with authToken: z.string() or a
privateBalanceRequestBody schema) and update the private-balance route to
validate/authenticate using that new schema (keep balanceQuerySchema used by
/v1/spl/balance without authToken). Ensure references to authToken in handlers
use the new header/body schema and not balanceQuerySchema.
🪄 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: 6471f08a-b46d-4a81-a101-c3bc40960f85
📒 Files selected for processing (7)
api/src/app.test.tsapi/src/lib/auth.tsapi/src/lib/solana.tsapi/src/routes/spl/spl.handlers.tsapi/src/routes/spl/spl.index.tsapi/src/routes/spl/spl.routes.tsapi/src/routes/spl/spl.schemas.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/lib/auth.ts`:
- Around line 41-47: parseAuthToken currently splits the Authorization header
but doesn't verify the scheme; update the function (parseAuthToken) to check
that the header starts with the "Bearer" scheme (case-insensitive) before
returning the token and return undefined for any other scheme or malformed
header; use the existing header lookup for "Authorization"/"authorization",
validate the prefix (e.g., split into two parts or use startsWith + safe
slicing) and ensure you guard against missing token part so only a valid Bearer
token is returned.
In `@api/src/routes/spl/spl.handlers.ts`:
- Line 26: Remove the unused import authTokenSchema from
api/src/routes/spl/spl.handlers.ts: locate the import list that currently
includes authTokenSchema and delete that identifier (or the entire import
statement if it only contains authTokenSchema) so the file no longer imports an
unused symbol; keep other imports intact and run the linter/TypeScript build to
confirm no remaining references to authTokenSchema in the spl.handlers module.
In `@api/src/routes/spl/spl.schemas.ts`:
- Around line 77-82: The example for the authorization field in authTokenSchema
is not in Bearer format and will cause parseAuthToken (which does
authToken.split(" ")[1]) to return undefined; update the example in
authTokenSchema to show the expected "Bearer <token>" format (e.g., "Bearer
1234567890") so it matches parseAuthToken's behavior and documentation for the
authorization header.
- Around line 242-245: Remove the unused authToken field from the
transferRequestSchema (the z.string().openapi(...) .optional() entry) so the
OpenAPI/request body matches actual behavior; the handler uses parseAuthToken on
request headers (see parseAuthToken usage in the transfer handler) to extract
authToken, so do not expect it in the body and update any related examples or
documentation metadata in transferRequestSchema accordingly.
🪄 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: 135c0c07-1ed9-4882-9bcf-5c50edfa24c1
📒 Files selected for processing (6)
api/src/app.test.tsapi/src/lib/auth.tsapi/src/lib/solana.tsapi/src/routes/spl/spl.handlers.tsapi/src/routes/spl/spl.routes.tsapi/src/routes/spl/spl.schemas.ts
* chore: docs * chore: docs * chore: docs * chore: simplify
* fix: comipation because of some deps using edition = 2024 in tomls * feat: support source_program verifiaction * fix: fmt * Delete yarn.lock --------- Co-authored-by: Gabriele Picco <piccogabriele@gmail.com>
* feat: add simpler transfer from base to delegated account * chore: refactor * chore: refactor * chore: refactor * chore: refactor * fix: workflow * chore: upgrade SDK in API * chore: revert workflow
…47) * chore: charge fee to prevent ata closing and draining of sponsor PDA * chore: refactor
* chore: refactor * Update e-token/src/processor/shuttle_close_schedule.rs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: refactor --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* feat: add user specified client-ref-id * chore: refactor * chore: use internal memo as external program require all accounts to be signers * chore: simplify internal memo
* feat: add auto queue topup * chore: refactor
* feat: improve api * chore: fix tests
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/src/routes/spl/spl.routes.ts (1)
137-150:⚠️ Potential issue | 🟡 MinorDocument the auth failure response on the protected SPL routes.
Both routes now require
authTokenSchema, but their response maps still only advertise200/400/422. Generated clients and/docconsumers will miss the runtime auth rejection branch. Please add the handler’s auth failure status here as well (401/403, whichever these handlers actually return).Also applies to: 168-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/routes/spl/spl.routes.ts` around lines 137 - 150, The route response map for transferRoute (created via createRoute) is missing the auth-failure response for the protected endpoints that use authTokenSchema; update the responses object on transferRoute (and the other protected SPL route created similarly) to include the actual auth failure status code your handlers return (401 or 403) with an appropriate jsonContent(errorResponseSchema, "Authentication error") entry so generated clients and /doc consumers see the runtime auth rejection branch; ensure you add the same response entry to the other SPL route defined with createRoute that also uses authTokenSchema.idl/ephemeral_spl_token.json (1)
1416-1601:⚠️ Potential issue | 🟠 MajorSync this internal-instruction section with the current dispatcher.
e-token/src/entrypoint.rsnow dispatches internalMARK_TRANSFER_QUEUE_REFILL_PENDING(204), but this IDL section still stops at 203. At the same time, the dispatcher no longer has an arm for internal 201 while this file still publishesundelegateWithdrawAndCloseShuttleEphemeralAta. That leaves generated bindings/docs out of sync with the on-chain ABI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idl/ephemeral_spl_token.json` around lines 1416 - 1601, The IDL's internal-instruction list is out of sync with the runtime dispatcher: remove the stale undelegateWithdrawAndCloseShuttleEphemeralAta entry (the one with discriminator 201) and add a new internal instruction entry for MARK_TRANSFER_QUEUE_REFILL_PENDING with discriminator [204], matching the dispatcher; ensure the new entry uses the same account and args shape as the on-chain handler for that opcode so generated bindings/docs align with the on-chain ABI (reference the existing nearby symbols transferLamportsPda (200), undelegateLamportsPda (202), and closeLamportsPdaIntent (203) to match formatting).
♻️ Duplicate comments (1)
api/src/lib/auth.ts (1)
44-53:⚠️ Potential issue | 🟡 MinorAccept case-insensitive Bearer schemes.
Line 50 still requires the exact
"Bearer"prefix. HTTP auth schemes are case-insensitive, so valid headers likeauthorization: bearer <token>will be dropped here and then misclassified downstream as missing auth.Suggested fix
export function parseAuthToken(headers: Record<string, string>): string | undefined { const authToken = headers["Authorization"] ?? headers["authorization"]; if (!authToken) { return undefined; } - const parts = authToken.split(" "); - if (parts.length !== 2 || parts[0] !== "Bearer") { + const parts = authToken.trim().split(/\s+/, 2); + if (parts.length !== 2 || parts[0].toLowerCase() !== "bearer" || parts[1].length === 0) { return undefined; } return parts[1]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/lib/auth.ts` around lines 44 - 53, The parseAuthToken function currently requires the exact "Bearer" scheme which is case-sensitive; update the validation in parseAuthToken to treat the scheme case-insensitively (e.g., compare parts[0].toLowerCase() to 'bearer') and be tolerant of extra whitespace (e.g., split on whitespace or trim parts) so headers like "authorization: bearer <token>" are accepted; ensure you still return the token portion (or join remaining parts if you split more broadly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/README.md`:
- Around line 533-534: Replace all example occurrences of the literal
initVaultIfMissing: true with the new default guidance (either remove the field
or set it to false) so the README examples match the updated statement that
initVaultIfMissing defaults to false; specifically update the MCP tool-call
example (the snippet referencing initVaultIfMissing in the MCP example) and the
POST /v1/spl/deposit example (the snippet containing initVaultIfMissing: true)
to either omit the key or set it to false and keep surrounding example text
consistent.
In `@api/src/env.ts`:
- Line 25: MOCK_PER currently uses z.boolean() which rejects string values from
Cloudflare Workers; update the schema for MOCK_PER to use z.preprocess to
normalize string inputs (like the preprocessing used in
optionalString/optionalUrl) so that string "true"/"false" (case-insensitive) are
converted to booleans before validation, then keep .optional().default(false);
target the MOCK_PER schema entry and wrap its current z.boolean() with a
preprocess function that lowercases string inputs and returns true/false
accordingly, leaving non-string inputs untouched.
In `@api/src/lib/solana.ts`:
- Around line 418-423: The code currently swallows failures from
getValidatorFromRpc by returning DEFAULT_FALLBACK_VALIDATOR, causing downstream
flows to silently target the hardcoded validator; change the catch so it does
not return the fallback — either remove the catch so the original error
propagates or throw a new Error with context (e.g., "failed to resolve validator
from RPC: <err>") so callers fail fast; if a fallback is ever desired make it
explicit via a config flag (e.g., useDefaultFallbackValidator) and only return
DEFAULT_FALLBACK_VALIDATOR when that flag is true. Ensure you modify the block
around getValidatorFromRpc and DEFAULT_FALLBACK_VALIDATOR and update callers to
handle propagated errors accordingly.
In `@api/src/routes/spl/spl.schemas.ts`:
- Around line 21-33: The function isNonNegativeBigIntString contains a redundant
try/catch because the /^\d+$/ regex already ensures the string is a valid
non-negative integer; simplify the function by removing the BigInt() call and
try/catch and return true when the regex test passes, otherwise false (update
isNonNegativeBigIntString accordingly to rely solely on the regex).
In `@e-token-api/src/state/transfer_queue.rs`:
- Around line 260-273: queue_allocate_group_id_from_data currently scans all
queue items for each candidate ID (using group_id_in_use), causing O(n ×
MAX_GROUP_ID) worst-case behavior; replace this linear search by tracking used
group IDs in a compact bitmap or free-list stored in the queue header and
consult that instead. Add a bitmap field to the header (e.g., used_group_bitmap)
and helper functions (set_group_bit, clear_group_bit, test_group_bit) and update
all places that add/remove queue items to flip the corresponding bit; then
change queue_allocate_group_id_from_data to iterate candidates by checking
test_group_bit and updating stored_next_group_id via set_stored_next_group_id
when an unused bit is found, removing the per-candidate linear scan of items and
eliminating group_id_in_use dependence for allocation.
In `@e-token/src/processor/lamports_pda.rs`:
- Around line 191-250: Ensure rent_pda_info is validated before embedding it
into close_handler_accounts: verify its PDA matches the canonical rent PDA
(e.g., compare against derive_rent_pda(...) or RENT_PDA constant and return
ProgramError::InvalidSeeds if mismatched), confirm its owner is the system
program (compare owner to system_program::id() and return
ProgramError::IncorrectProgramId or InvalidAccountData on mismatch), and ensure
rent_pda_info.data_len() == 0 (return ProgramError::InvalidAccountData if not).
Add these checks near where rent_pda_info is used (before building
close_handler_accounts / close_handler) so the scheduled close will not be
created with an invalid rent account.
In `@e-token/src/processor/process_transfer_queue_tick.rs`:
- Around line 69-71: The code currently returns early when
try_schedule_queue_refill(&tick_accounts, &queue_state)? returns true, which
stops executing ready transfers; change the control flow in
process_transfer_queue_tick so that scheduling a refill does not short-circuit
the rest of the function: remove the early return and instead allow the
execute-ready path to run regardless (you can set a local flag like
refill_scheduled = true for logging or later handling), and ensure the same fix
is applied to the similar block around the 147-190 area (where refill scheduling
currently returns) so ready transfers continue to be dispatched even when a
refill is scheduled.
In `@e-token/src/processor/transfer_queue_refill.rs`:
- Around line 226-265: Account 0 (rent_pda_info) is marked readonly_signer but
the callee treats it as payer and debits it, so change the CPI accounts
declaration to make rent_pda_info a writable signer: replace
InstructionAccount::readonly_signer(rent_pda_info.address()) with
InstructionAccount::writable_signer(rent_pda_info.address()) in the
sponsored_transfer_accounts array used to build sponsored_transfer_instruction;
keep the rest of the account_refs, rent_signer_seeds and
invoke_signed_with_bounds call unchanged so the PDA signer still signs the CPI.
In `@e-token/src/processor/undelegate_and_close_shuttle_to_owner.rs`:
- Around line 75-100: Before calling schedule_shuttle_close_after_undelegate,
add checks to reject readonly token accounts: verify
shuttle_wallet_ata_info.is_writable() and refund_token_info.is_writable() (the
destination token) and return an error if either is not writable; do this after
the validate_token_account calls and before
schedule_shuttle_close_after_undelegate so we never enqueue a
SETTLE_AND_CLOSE_SHUTTLE_INTENT that cannot later transfer or close funds.
---
Outside diff comments:
In `@api/src/routes/spl/spl.routes.ts`:
- Around line 137-150: The route response map for transferRoute (created via
createRoute) is missing the auth-failure response for the protected endpoints
that use authTokenSchema; update the responses object on transferRoute (and the
other protected SPL route created similarly) to include the actual auth failure
status code your handlers return (401 or 403) with an appropriate
jsonContent(errorResponseSchema, "Authentication error") entry so generated
clients and /doc consumers see the runtime auth rejection branch; ensure you add
the same response entry to the other SPL route defined with createRoute that
also uses authTokenSchema.
In `@idl/ephemeral_spl_token.json`:
- Around line 1416-1601: The IDL's internal-instruction list is out of sync with
the runtime dispatcher: remove the stale
undelegateWithdrawAndCloseShuttleEphemeralAta entry (the one with discriminator
201) and add a new internal instruction entry for
MARK_TRANSFER_QUEUE_REFILL_PENDING with discriminator [204], matching the
dispatcher; ensure the new entry uses the same account and args shape as the
on-chain handler for that opcode so generated bindings/docs align with the
on-chain ABI (reference the existing nearby symbols transferLamportsPda (200),
undelegateLamportsPda (202), and closeLamportsPdaIntent (203) to match
formatting).
---
Duplicate comments:
In `@api/src/lib/auth.ts`:
- Around line 44-53: The parseAuthToken function currently requires the exact
"Bearer" scheme which is case-sensitive; update the validation in parseAuthToken
to treat the scheme case-insensitively (e.g., compare parts[0].toLowerCase() to
'bearer') and be tolerant of extra whitespace (e.g., split on whitespace or trim
parts) so headers like "authorization: bearer <token>" are accepted; ensure you
still return the token portion (or join remaining parts if you split more
broadly).
🪄 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: 4ece99ed-6ebe-45b5-a4f7-c4bf1b16a1a4
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapi/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
README.mdapi/README.mdapi/package.jsonapi/src/app.test.tsapi/src/env.tsapi/src/lib/auth.tsapi/src/lib/solana.tsapi/src/routes/mcp.route.tsapi/src/routes/spl/spl.handlers.tsapi/src/routes/spl/spl.routes.tsapi/src/routes/spl/spl.schemas.tsdocs/docs.MDe-token-api/src/consts.rse-token-api/src/lib.rse-token-api/src/state/fees_pda.rse-token-api/src/state/mod.rse-token-api/src/state/transfer_queue.rse-token-api/src/state/transfer_queue_refill.rse-token/src/entrypoint.rse-token/src/processor/close_shuttle_ata_intent.rse-token/src/processor/deposit_and_delegate_shuttle_ephemeral_ata_with_merge.rse-token/src/processor/deposit_and_delegate_shuttle_ephemeral_ata_with_merge_and_private_transfer.rse-token/src/processor/deposit_and_queue_transfer.rse-token/src/processor/execute_ready_queued_transfer.rse-token/src/processor/fees_pda.rse-token/src/processor/initialize_transfer_queue.rse-token/src/processor/lamports_pda.rse-token/src/processor/mod.rse-token/src/processor/process_transfer_queue_tick.rse-token/src/processor/shuttle_close_schedule.rse-token/src/processor/transfer_queue_refill.rse-token/src/processor/undelegate_and_close_shuttle_ephemeral_ata.rse-token/src/processor/undelegate_and_close_shuttle_to_owner.rse-token/src/processor/undelegate_withdraw_and_close_shuttle_ephemeral_ata.rse-token/src/processor/withdraw_through_delegated_shuttle_with_merge.rse-token/tests/deposit_and_delegate_shuttle_ephemeral_ata_with_merge_and_private_transfer.rse-token/tests/deposit_and_queue_transfer.rse-token/tests/fees_pda.rse-token/tests/lamports_pda.rse-token/tests/transfer_queue_automation.rse-token/tests/withdraw_through_delegated_shuttle_with_merge.rsidl/ephemeral_spl_token.json
💤 Files with no reviewable changes (5)
- e-token-api/src/state/fees_pda.rs
- e-token/src/processor/undelegate_withdraw_and_close_shuttle_ephemeral_ata.rs
- e-token/src/processor/undelegate_and_close_shuttle_ephemeral_ata.rs
- e-token/tests/fees_pda.rs
- e-token/src/processor/fees_pda.rs
GabrielePicco
left a comment
There was a problem hiding this comment.
LGTM!
Left one comment to address or discuss
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/lib/auth.ts`:
- Around line 58-68: The upstream auth fetch calls in getChallenge (the fetch
assigned to challengeResponse) and the later PER call (the other bare fetch in
this file) must be wrapped with an AbortController timeout so stalled upstreams
fail fast; create/use a short timeout helper that instantiates an
AbortController, pass controller.signal into fetch, start a setTimeout that
calls controller.abort() after the timeout, and clearTimeout after fetch
completes (or on error) to avoid leaks; replace the direct await fetch(url)
calls in getChallenge and the other PER call with this pattern so the requests
produce a controlled abort error when the timeout elapses.
In `@api/src/routes/spl/spl.handlers.ts`:
- Around line 114-119: The handler challengeHandler currently logs incoming
query data (console.log(query)), which leaks caller wallet pubkeys and noisy
production logs; remove that debug statement and ensure no sensitive fields from
the query are logged, then return the response as before by calling
getChallenge(env, query) and c.json(response, 200); locate the console.log in
the challengeHandler for the challengeRoute and delete it (no other changes to
getChallenge or query handling required).
In `@api/src/routes/spl/spl.schemas.ts`:
- Around line 100-105: The current authTokenSchema is optional but
privateBalanceHandler requires the header; split it into two schemas:
requiredAuthTokenSchema (authorization: z.string() required with OpenAPI
example/description reflecting that private-balance requires the token) and
optionalAuthTokenSchema (authorization: z.string().optional() with a description
noting it may be used by transfer), then replace usages: update
privateBalanceRoute to use requiredAuthTokenSchema and transferRoute to use
optionalAuthTokenSchema; also update the description text in both schemas to be
accurate for each route.
🪄 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: 848fe11b-6ffe-4d62-aaf6-f88407e5967b
📒 Files selected for processing (6)
api/src/app.test.tsapi/src/env.tsapi/src/lib/auth.tsapi/src/lib/solana.tsapi/src/routes/spl/spl.handlers.tsapi/src/routes/spl/spl.schemas.ts
Adds route to get an auth token from a PER and use it when connecting
Summary by CodeRabbit
New Features
/v1/spl/challenge) and token issuance (/v1/spl/login)Tests