fix(cct-sdk): Harden Solana operation validation - #414
Merged
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❌ 1 of 2574 tests did not pass (pass 2567, fail 1, cancelled 0, skipped 6) in 5m 54s Failed suites: Failed testsSummaryCoverage report |
Collaborator
✅ APPROVE-WITH-NITS — 0 blockers, 0 correctness majorsSummary: Correct (mechanical refactor + new input validations are grounded on-chain), type-safe (tsc green), green (all non-metaplex tests pass). Extracts 3 reusable dedup validators ( Key Verification:
Live Devnet Proofs:
|
Collaborator
[COMMENT 1/4] 🔴 Major (Packaging) — "Refactor" commit bundles 5 behavior changesProblem: This single commit labeled "fix: refactor solana ops" quietly bundles multiple behavior changes, none flagged or separately justified:
Grounding: All are defensible (new checks mirror on-chain program behavior or established EVM policy), but they're behavior changes that deserve their own commits with rationales. Live-Proven Behavior Differences (real op code, OLD vs NEW):
Fix: Split the commit into:
Changelog action: If the Locations:
|
Collaborator
[COMMENT 2/4] 🔴 Major (DX) — Inconsistent error parameter naming in dedup validatorsProblem: The three new dedup validators blame errors differently, making some duplicates hard to identify: Current Behavior:
Live Example (Proven Above): Fix: Update // validate.ts:98 — validateUniquePublicKeys
export function validateUniquePublicKeys(
operation: string,
param: string,
publicKeys: PublicKey[],
): void {
const seen = new Set<string>()
for (const [i, pk] of publicKeys.entries()) {
const key = pk.toBase58()
if (seen.has(key)) {
throw new CCTParamsInvalidError(
operation,
`${param}[${i}]`, // ← ADD: name the offending index
'must not contain duplicate addresses'
)
}
seen.add(key)
}
}Cost: Updates to 3 test assertions (replace bare param with indexed param or
Recommendation: Apply the fix and update test assertions in the same commit (comment 1: as part of the dedup-addition split). Locations:
|
Collaborator
[COMMENT 3/4] 🟡 Low (DX) — Wording inconsistency in dedup error messagesProblem: The three dedup validators use inconsistent nouns for what they're checking: validateUniqueChainSelectors → "must not contain duplicate chain selectors"
validateUniquePublicKeys → "must not contain duplicate addresses"
validateUniqueHexBytes → "must not contain duplicate hex values" // ← too genericWhen Impact: Minimal — the message is technically correct (they are hex values), but less actionable than peer validators. Fix: Update the message to name the domain concept: // validate.ts:130
export function validateUniqueHexBytes(operation: string, param: string, values: Buffer[]): void {
if (new Set(values.map((value) => value.toString('hex'))).size !== values.length) {
throw new CCTParamsInvalidError(
operation,
param,
// ← e.g., if param='remotePoolAddresses', this becomes "must not contain duplicate remote pool addresses"
`must not contain duplicate ${param.includes('Pool') ? 'remote pool addresses' : 'addresses'}`
)
}
}OR (simpler — let the caller provide context in the error): Simply update the call sites to pre-pend the semantic noun: // edit-chain-remote-config.ts — pass clearer context
validateUniqueHexBytes(this.name, 'remotePoolAddresses (hex)', params.remotePoolAddresses)Then the message becomes: "remotePoolAddresses (hex): must not contain duplicate hex values" — still not perfect, but the caller knows what was validated. Recommendation: Apply the simpler approach (let validators keep generic messages, but rename the param passed to them if needed for clarity). This avoids hardcoding business logic into Location: validate.ts:130 (validateUniqueHexBytes message and/or call sites in edit-chain-remote-config.ts) |
Collaborator
[COMMENT 4/4] 🔵 Nits — JSDoc and documentation gaps(a) validateUniqueChainSelectors silently skips non-bigint entriesCurrent JSDoc: /**
* Asserts bigint chain selectors do not contain duplicates.
* @throws CCTParamsInvalidError if a chain selector is duplicated.
*/
export function validateUniqueChainSelectors(...): voidProblem: The function iterates with Fix: Add a one-line JSDoc note: /**
* Asserts bigint chain selectors do not contain duplicates.
* @remarks Silently ignores non-bigint entries; relies on downstream `validateBigInt` for type safety.
* @throws CCTParamsInvalidError if a chain selector is duplicated.
*/Location: validate.ts:106 (b) Ops don't document the new dedup rejectionsThe new dedup validators aren't surfaced in op Fix: Add one-line // token-pool/operations/deploy-token-pool.ts
/**
* Initializes a Solana token pool, optionally configuring an allowlist.
* @remarks The allowlist must not contain duplicate addresses.
*/
export class DeployTokenPool extends SolanaOperation { ... }
// token-pool/operations/apply-chain-updates.ts
/**
* Applies chain configuration updates to a Solana token pool.
* @remarks Chain selectors to add and remove must not contain duplicates.
*/
export class ApplyChainUpdates extends SolanaOperation { ... }Locations:
(c) DeployToken preMint documentation incompleteCurrent JSDoc: /** Initial supply in base units. Requires preMintRecipient. */
preMint?: bigintProblem: The new validation adds a Fix: Update the field JSDoc: /**
* Initial supply in base units, between 1 and 2^64 - 1 (u64 maximum).
* Requires preMintRecipient if provided.
*/
preMint?: bigintLocation: deploy-token.ts (BaseDeployTokenParams.preMint field JSDoc) These are all documentation-only polish — the code is correct, but these will help future maintainers and SDK users. |
apedrob
approved these changes
Sep 9, 2026
mervin-link
changed the title
mervin-link
force-pushed
the
fix/refactor-ops
branch
from
September 9, 2026 17:20
df60f9a to
65bf531
Compare
Collaborator
Author
@aelmanaa Addressed all your comments. |
aelmanaa
approved these changes
Sep 9, 2026
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
3 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
DeployToken.preMintto u64 maximum.@localnerve/image-size@2.1.2Why