fix(cct-sdk): Prevent partial Solana CCT commits - #416
Open
mervin-link wants to merge 3 commits into
Open
Conversation
|
You must have Developer access to commit code to Chainlink Labs on Vercel. If you contact an administrator and receive Developer access, commit again to see your changes. Learn more: https://vercel.com/docs/accounts/team-members-and-roles/access-roles#team-level-roles |
CI Test Report❌ 3 of 2579 tests did not pass (pass 2570, fail 3, cancelled 0, skipped 6) in 5m 53s Failed suites: Failed testsSummaryCoverage report |
Collaborator
The Bug & The FixThe Bug
Concrete case: a token has wallet A as mint authority and wallet B as freeze authority. A calls The FixOne shared send routine (
Compute-budget vs program failure is decided on the structured Solana error, not on message text: function isComputeBudgetError(error: unknown): boolean {
if (!(error instanceof SendTransactionError)) return false
const structured = (error as { simulationError?: unknown }).simulationError
if (typeof structured === 'string') return COMPUTE_BUDGET_ERRORS.has(structured)
if (structured && typeof structured === 'object' && 'InstructionError' in structured) {
const detail = (structured as { InstructionError?: unknown }).InstructionError
return Array.isArray(detail) && typeof detail[1] === 'string' && COMPUTE_BUDGET_ERRORS.has(detail[1])
}
return false
}The split-vs-rethrow decision in the slice loop: try {
unitsConsumed = (await simulateTransaction(ctx, { payerKey, instructions: slice.instructions, ... })).unitsConsumed || 0
break
} catch (error) {
// split only on a compute-budget (resource) error; rethrow every program rejection
if (!requireSingleTransaction && isComputeBudgetError(error) && end - 1 > start) {
slice = getInstructionSlice(unsigned, start, --end)
continue
}
throw error
}Properties the Fix Guarantees
ProofP1 — The bug is real (devnet)The old truncating loop, given P2 — The fix prevents the partial commit at the send path (devnet)The same P3 —
|
Collaborator
Finding #1 — 🟡 Medium (DX): Partial-Failure State is Hard to Act OnOn a split failure, Impact: Callers receiving an error don't know whether partial state was committed and cannot reason about recovery (retry, rollback, etc.). Fix:
Example:
This change makes it clear to operators that prior slices succeeded and are permanent. |
Collaborator
Finding #2 — 🟡 Medium (DX): Authority Pre-Check Error Message Lacks ContextThe authority pre-check error does not name the current authority: "must match the token mint authority" does not say what the correct value is, and the already-revoked ( Impact: When validation fails, callers don't see the on-chain authority value and must make a separate query to understand what went wrong. Fix: Enhance error message with the actual value (location:
Alternatively: Put Example:
This reduces the round-trip to understand validation failures. |
Collaborator
Finding #3 — 🟡 Low: Missing @throws Documentation on execute()The class docs cover the atomicity guarantee, but the public Fix: Add
Example: /**
* Generate, sign, simulate, send, and confirm with the current authority wallet.
*
* @throws {CCTParamsInvalidError} If authority types are invalid or if authority does not match the executing wallet
* @throws {CCIPTokenDataParseError} If the mint data cannot be parsed
* @throws {CCTTxFailedError} If the transaction fails on-chain (e.g., wrong current authority)
* @throws {CCTTxNotConfirmedError} If the transaction is not confirmed in time
*/
override async execute(...)Finding #4 — Nits (Polish)(a) Rename the concept from "truncation" An oversized transaction hard-fails at serialize time, it is not silently truncated. "Size overflow" is more accurate. (b) Clarify the atomicity wiring The atomicity is wired by a bare positional return submit(..., /* requireSingleTransaction */ true)(c) Document ALT-only-on-main-slice behavior The lookup-tables-only-on-the-main-slice behavior silently drops ALTs on non-main slices. Safe today (no CCT op sets (d) Namespace committedHashes to avoid key collisions
|
Collaborator
Recommendation & Summary✅ APPROVE. The bug is reproduced (P1), the fix prevents it (P2), and both are proven on a local validator and on devnet. The remaining items are documentation/DX polish (findings above), not blockers. What ships:
Polish items (non-blocking):
Recommendation: Ship. Polish can follow in a focused commit if preferred. |
Collaborator
Author
@aelmanaa Addressed all comments. |
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
2 participants
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.
What
setTokenAuthorityatomicity and validate selected mint/freeze authorities before submissionCCTTxFailedError.context.committedHashesWhy
Notes
simulateAndSendTxshelper because shared Solana version treats every simulation failure as a size failure and splits instructions. The local path splits only after a deterministic size check, preserving CCT operation semantics without changing shared SDK behavior.